IOPort/Windows: Fix wrong break condition handling #262
Conversation
Ignore break conditions completely (like BreakBehaviour=Ignore), rather than treating them as errors without further handling them in a useful way.
| if (estatus > 0 && inerrmsg) { | ||
| if (estatus & CE_BREAK) { sprintf(errmsg, "IOPort: Break condition on receive line detected.\n"); strcat(inerrmsg, errmsg); } |
There was a problem hiding this comment.
We can leave this line for future use.
| @@ -369,7 +369,8 @@ PsychError IOPORTOpenSerialPort(void) | |||
| "or on OS/X and Linux in 'Cooked' processing mode as line delimiter. A setting of -1 will try to disable the line terminator.\n\n" | |||
| "DTR=os default -- Setting for 'Data Terminal Ready' pin: 0 or 1.\n\n" | |||
| "RTS=os default -- Setting for 'Request To Send' pin: 0 or 1.\n\n" | |||
| "BreakBehaviour=Ignore -- Behaviour if a 'Break Condition' is detected on the line: Ignore, Flush, Zero. On Windows, this setting is ignored.\n\n" | |||
| "BreakBehaviour=Ignore -- Behaviour if a 'Break Condition' is detected on the line: Ignore, Flush, Zero. On Windows, this " | |||
There was a problem hiding this comment.
Hmm, thinking further about this: I think we should not "Ignore" the setting, instead we should validate the parameter and error abort with a "Setting not supported on Windows" error message or such, if a setting other than Ignore is specified.
This way the default or explicit setting of Ignore will do the right thing - and compatible to other OS'es, but users get a heads up if they try to ask for a behaviour that is fine on Linux/macOS, but unsupported on Windows. I think that would be the most sane approach, and the one I should have implemented in the first place.
PsychSerialWindowsGlue.c already has the commented out if-else statements, so one just has to uncomment and update it accordingly, and change the last sentence of the help text here to "On Windows, only 'Ignore' is allowed."
|
So some more suggestions for a cleaner solution, the one that I should have done in the first place. You can make them and force-push to update this pull request, or I can do them, up to you. I'll probably do another PTB bugfix release very soon, probably already tomorrow or at the weekend. If you get to it by early noon tomorrow, great. Otherwise I may do it. |
|
Okay. I hope the 2nd commit is what you had in mind. |
Fixup help string formatting.
Fixup for consistency with Unix error message format.
…r settings. As GitHub user @qx1147 found out during a code review, there is a flaw in the handling of serial port options, where default serial port options may override user provided configuration options, so the wrong settings are silently applied! This only happens if the user script provides different settings in IOPort('OpenSerialPort', ..., settings), ie. on initial port open. Calls to IOPort('ConfigureSerialPort', ..., settings) are not affected. Patch this up by reordering the if-else ladders for Unix and Windows: Default settings must come as the last else-if-else block before the error handling for invalid parameters. This way, user provided non-default settings for serial port parameters take precedence over the built in defaults, as intended. Luckily (dumb luck!), only a few options were mishandled, and most of these are rarely changed by user scripts, specifically: ProcessingMode=Cooked would get ignored, and raw mode used instead. Rarely used on Linux/macOS and not supported on Windows anyways. Low impact. BreakBehaviour=Ignore would be used instead of Flush or Zero. Not a problem on Windows, as that is the only supported option. The fact that this went unnoticed on Linux/macOS suggests not much use of Flush or Zero by users scripts. No use by PTB itself. Low expected impact. Note that the implementation of 'Flush' on the Unixes is of questionable use, as Flush would not only flush read/write buffers, but also send a SIGINT, which may end in unexpected ways on Unix, as we don't handle SIGINT specifically. StopBits=2 would get ignored and the default of 1 stop bits would be used. Unclear how many devices want 2 stop bits, but I haven't ever seen one, so probably low expected impact. DataBits=16 would get ignored on MS-Windows only and replaced by 8 bits, whereas other settings are fine, and 16 bits is unsuported on Linux/macOS, so probably rarely if ever used. Low expected impact. FlowControl=None will be used instead of hardware or software flow control. This can be a real bummer with potential real impact, as some devices do support or recommend active flow control! In the case of PTB, the CedrusResponseBox() driver for Cedrus response box devices would have liked that setting. Audit of Psychtoolbox internal user of IOPort, and of the demos, only shows one bad case where things went sideways: CedrusResponseBox.m This driver requested hardware flow control, but silently got instead NO flow control! This is interesting, because during my testing I found communication with Cedrus boxes always rather unreliable, and now I have to wonder if this was because of the accidental lack of hardware flow control? I can't find out, as I lost access to Cedrus devices long ago. This whole parameter handling is somewhat fragile, and could do with an improved implementation, but due to the severe lack of financial funding for PTB, this is not an option in the foreseeable future. Not even code review or testing of 3rd party contributions, if there were any. Luckily this part of the driver is mostly static since years, so I guess we can drag our feet longer and sit it out. Another problem pointed out by @qx1147 is indeed that wrong options, e.g., due to typos in users experiment scripts, would not lead to an error abort or warning, but would get silently ignored in many cases. Not great at all, but lack of funding will leave this problems also unsolved in the near- to midterm, possibly forever. Life sucks... See also PR #262 for discussion. Thanks to @qx1147 for catching this! Work time spent on IOPort improvements: 5 hours, resulting in loss of at least 1000 Euros of billable time to the project!
|
Yeah, probably not the worst idea to only address what would be considered a bug. Unfortunately, the more robust and user-friendly we make the configuration string processing (e.g., by making it case-insensitive, ignoring white spaces, and catching unprocessed, i.e., misspelled parameters), the higher is the risk of breaking old user scripts by then silently obeying potentially wrong configuration settings which have been ignored before ("wrong" in the sense of making the serial communication not work anymore). Therefore, "Stick to the shit!" might even be the most sensible choice here. |
|
Yes. If you want to send a pull request to my github with some help text
improvements, I'll pull it. On vacation from now on though, it may take a
while, depending on mood.
In principle we have PsychDefaultSetup(featureLevel); for this, so user
scripts can declare how "modern" they are and save themselves a bit of
boilerplate code. Right now:
0 = Old style. Just execute AssertOpenGL.
1 = Cross platform aware: Also execute KbName('UnifyKeyNames');
2 = Modern color aware: Also expect colors in normalized 0-1 range instead
of 0-255 or other custom ranges.
One could add 3 for dealing with such things. In theory. Difficult to say
what settings one could lump together under that.
Right now, many PTB demos and tests do not even use the "new" style of
things, although the "new" style exists since probably 10 years now,
because there is not ever enough time for such maintenance of old demos...
Merry x-mas and happy new year!
-mario
…On Mon, Dec 4, 2023 at 11:22 AM qx1147 ***@***.***> wrote:
Yeah, probably not the worst idea to only address what would be considered
a bug. Unfortunately, the more robust and user-friendly we make the
configuration string processing (e.g., by making it case-insensitive,
ignoring white spaces, and catching unprocessed, i.e., misspelled
parameters), the higher is the risk of breaking old user scripts by then
silently obeying potentially wrong configuration settings which have been
ignored before ("wrong" in the sense of making the serial communication not
work anymore). Therefore, "Stick to the shit!" might even be the most
sensible choice here.
Maybe a warning in the help text for 'OpenSerialPort' and
'ConfigureSerialPort' would be helpful, at a prominent place, that the
configuration string is absolutely case-sensitive and does not tolerate
white-spaces around the "=" character, and that consistency checking is
weak.
—
Reply to this email directly, view it on GitHub
<#262 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIXMNQCITZBOWZE5AYIHPLYHWP4XAVCNFSM6AAAAABABBJULSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZYGI2DEOJUGY>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Ignore break conditions completely (like BreakBehaviour=Ignore), rather than treating them as errors without further handling them in a useful way. As discussed in Pull request #821, this commit will change IOPort's behaviour in a somewhat incompatible way, mostly because the user script won't have any possibility anymore to find out about break conditions.
According to the help text of
IOPort('OpenSerialPort',...)for theBreakBehavioursetting, the setting is ignored for Windows. But it was not further specified what kind of break behaviour was actually implemented. The implementation so far treated a break condition as an error, which caused data to be discarded when the condition was detected during a read operation. Moreover, the user script was notified about the error through a return parameter of the IOPort's read or write function. Beyond that, no further actions were taken like, for example, preserving data that was received before the break condition and re-starting normal data reception at the end of the break condition. However, implementing such handling would be very difficult if not impossible without low-level OS support. So, ignoring the break condition altogether seems to be the better solution and is consistent with theBreakBehaviour=Ignoresetting, which is the default for other OSs as well.