Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add java constant for non-standard baud rate 921600 #177

Conversation

hiddenalpha
Copy link

@hiddenalpha hiddenalpha commented Nov 27, 2024

Fixes: issue 176

@hiddenalpha hiddenalpha marked this pull request as ready for review November 27, 2024 13:56
@tresf
Copy link

tresf commented Nov 27, 2024

@hiddenalpha some questions:

  1. Should we add more? I did a cursory search for this after @alonarbel asked on Discord and found projects using some interesting values.

  2. I noticed that _nix_based returns UINT_MAX in scenarios where the baud can't be mapped. Do you have a guess as to how these OSs will behave?

  3. Since we use an int Java-side for our values (instead of something we can filter, such as an Enum) we don't do any sanity checking on baudRate, so it's my belief that adding this value to our list of constants only improves the readability of our API, NOT the functionality of it, right? I believe by adding this constant it does nothing functionality to add support for this higher baud rates to the API. Am I wrong?

@alonarbel , in #176 you mention that you believe a buffer overflow occurs as a result of this value. What may be helpful in that bug report is:

  • OS used for testing
  • Hardware used for testing
  • Any C or C++ examples of people communicating with this hardware at higher baud rates is greatly appreciated so that this project can attempt to implement something similar.

Copy link

@tresf tresf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More discussion is needed before we can definitely say that this PR closes #176. Please see main thread bullet points.

@hiddenalpha
Copy link
Author

hiddenalpha commented Nov 27, 2024

Should we add more? ...

Feel free :D I guess originally only the most common/portable values were added as constants. Because setParams() takes an integer, a caller can pass whatever value he likes. So theoretically the constants are not needed at all and seem to be there more for convenience.
Code in jssc.cpp already handles way more values than provided as java constants.
The Windows variant seems to accept any integer value (see win/jssc.cpp). _nix variant also handles more values than available as java constants (see some lines in getBaudRateByNum()).

... noticed that _nix_based returns UINT_MAX ... Do you have a guess as to how these OSs will behave?

The UINT_MAX is handled in setParams() (the else branch). Additionally, there seem to be some extra code for __APPLE__ where it uses ioctl() to set non-standard rates (see setParams() a bit later). On windows jssc just hands over the exact value to the API call. But I do not know what behavior this code finally yields.

Since we use an int Java-side for our values (instead of something we can filter, such as an Enum) we don't do any sanity checking on baudRate, so it's my belief that adding this value to our list of constants only improves the readability of our API, NOT the functionality of it, right? I believe by adding this constant it does nothing functionality to add support for this higher baud rates to the API. Am I wrong?

Agree. As already mentioned before, to me those java constants look like pure convenience. Adding more constants does not change any functionality. Therefore one could argue to just close this PR and explain that raw integer values could be passed to setParam whenever someone needs some special baud rates (likely #176 would be such a place where explanation is requested).

@alonarbel
Copy link

Hi @tresf and @hiddenalpha ,

As you mentioned, adding a constant for 921600 wouldn't help. In our application, we allow users to select the baud rate they want to use based on their hardware. When a user wants to connect, we use your setParam function and provide any baud rate parameter they choose, not just the predefined ones in your library.

However, we've encountered an issue when users send very long commands (via the writeBytes function) at 921600 baud. The commands are being truncated even if they use rts/cts flow control.(when the command are with regular size the command are sent fine, for example a command that truncated has 980 characters).
Our OS is windows.

@hiddenalpha
Copy link
Author

hiddenalpha commented Dec 1, 2024

As you mentioned, adding a constant for 921600 wouldn't help.

Ok. Then I vote to decline this PR :) I opened because I was not sure if the constants maybe led to confusion.

we use your setParam function and provide any baud rate parameter they choose, not just the predefined ones in your library.
... when users send very long commands (via the writeBytes function) at 921600 baud. The commands are being truncated even if they use rts/cts flow control.(when the command are with regular size the command are sent fine, for example a command that truncated has 980 characters).
Our OS is windows.

Thanks for clarification. So we have to look for the cause somewhere else :)

... believe, a buffer overflow occurs as a result of this value. What may be helpful in that bug report is:

  • OS used for testing
  • Hardware used for testing
  • Any C or C++ examples of people communicating with this hardware at higher baud rates is greatly appreciated so that this project can attempt to implement something similar.

As its not an issue with the java constants, I think we should continue collecting more facts (in scope of #176) as tresf already suggested.

@hiddenalpha hiddenalpha closed this Dec 1, 2024
@tresf tresf mentioned this pull request Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants