search

kleinerm / Psychtoolbox-3

This is kleinerm's git repository for development of Psychtoolbox-3. Regular end users should stay away from it, unless instructed by him otherwise, and use the official Psychtoolbox-3 GitHub page or distribution system for production releases.
104 stars 304 forks source link

IOPort/Windows: Fix wrong break condition handling #262

Closed qx1147 closed 9 months ago

qx1147 commented 9 months ago

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 the BreakBehaviour setting, 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 the BreakBehaviour=Ignore setting, which is the default for other OSs as well.

kleinerm commented 9 months ago

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.

qx1147 commented 9 months ago

Okay. I hope the 2nd commit is what you had in mind. The unusual way of how I check for the Ignore value points at a general flaw in the configuration string handling. This is a different topic, but most user errors will currently go undetected if the default setting string contains a setting. Moreover, the default settings might take precedence no matter what, unless I am missing something. Check what happens to a 'StopBits=2' user setting. I thought about fixing it, but it is not so simple without breaking compatibility.

qx1147 commented 9 months ago

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.

kleinerm commented 9 months ago

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 https://github.com/kleinerm/Psychtoolbox-3/pull/262#issuecomment-1838242946, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIXMNQCITZBOWZE5AYIHPLYHWP4XAVCNFSM6AAAAABABBJULSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZYGI2DEOJUGY . You are receiving this because you modified the open/close state.Message ID: @.***>