Conversation
Fixes calc_ifdelay() for baud > 19200 that was setting bit timing delays incorrectly leading to a variety of communication problems. Fixes ENOMSG error handling that was locking up modbus communication.
|
The same objection as in #3852. You are changing form and do not constrain yourself to substance (aka just fixing the bug). If you object to the form, then please submit that as a separate PR where we can discuss that with a broader audience and decide whether the style of coding needs to be adjusted. But, if so, then you should not change one very tiny corner, but adjust the entire source. |
…n with pktuart hardware implementation. (175u * baudrate + 99999u) / 100000u; could be implemented as (7u * baudrate + 3999(/4000; which would improve the calculation range but doesn't matter because already screening out large baud rates.
|
Thank you. However, now there are two issues with this backport:
|
|
(19200 < baudrate) and (baudrate > 19200) are both false when baudrate equals 19200. Correct, missed the "trying to interpret" message in one PR. I will fix that tomorrow. |
|
You are right about the comparison; when something like that my mind goes into condition reversal mode. However, my statement stands, it is pure poison and contrary convention. It doesn't help understanding, doesn't improve readability and doesn't qualify in defensive programming (preventing the single '=' (assignment) problem). |
|
"pure poison and contrary convention", I'm guessing you never had an HP
postfix calculator?
Placing the constant first in relational ops is completely valid and once
you use that order for avoiding assignment vs relational mistakes the mind
easily flips for all relational operators.
I have reverted both PRs to conform to your preference and corrected the
MSG_WARN.
Let's get this issue closed and make the improved code available!
Beyond the ENOMSG lockup, I suspect the baud rate problem created more
widespread problems for anyone using hm2_modbu with baud > 19200. I was
seeing occasion messages about incorrect length (that recovered). All
communications problems went away once I applied the calc_ifdelay() fix.
The ENOMSG bug could have remained in the code for a long time if
calc_ifdelay() was correct.
A lot of these discussions would go better over a coffee or beer. Where are
you located? I'm near Madison, WI, USA.
…On Wed, Mar 11, 2026 at 2:06 AM BsAtHome ***@***.***> wrote:
*BsAtHome* left a comment (LinuxCNC/linuxcnc#3857)
<#3857 (comment)>
You are right about the comparison; when something like that my mind goes
into condition reversal mode.
However, my statement stands, it is pure poison and contrary convention.
It doesn't help understanding, doesn't improve readability and doesn't
qualify in defensive programming (preventing the single '=' (assignment)
problem).
—
Reply to this email directly, view it on GitHub
<#3857 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHDVLM22MOWOQTYVK6EUV34QEGBFAVCNFSM6AAAAACWNIG3EKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DAMZWHE3TMMJXGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Actually, I had an HP RPN calculator and loved it (beautiful red small 7-segment display). However, this work is not postfix (nor prefix as in Lisp). C is infix.
Agreed!
Right. The problem with - especially older - equipment is that it may not be able to saturate the serial data line and actually leave inter-character delays in the stream. And those will be picked up as inter-packet delay if that is set too low. Good this particular bug was caught.
Love to chat over coffee, but I'm not even close. About 6700 km east (and a bit north) on the other side of the pond in that small country that has about one third of its land mass under sea level and has lots of windmills and levees. And, I wont be traveling to the USA again, ever (unfortunate but necessary). |
Fixes calc_ifdelay() for baud > 19200 that was setting bit timing delays incorrectly leading to a variety of communication problems. Fixes ENOMSG error handling that was locking up modbus communication.