Toggle2nist fix race conditions #3193
Conversation
Sigma1912
force-pushed
the
toggle2nist_fix_race_conditions
branch
from
b08f99e to
83afcad
Compare
src/hal/components/toggle2nist.comp
Outdated
| function _ nofp; | ||
| license "GPL"; | ||
| author "Anders Wallin"; | ||
| author "David Mueller, Based on work by Anders Wallin and John Kasunich"; |
There was a problem hiding this comment.
Isn't Anders Wallin still the author? 🤔
There was a problem hiding this comment.
I'm not really keen on having my name there. The reason I changed that is that I believe the 'author' is the person who could be contacted when questions arise. Since I completely changed the function it seems pointless to contact Anders Wallin.
To be honest, it has always been a mystery to me as to who the 'author' is supposed to be for something that is substantially changed. Is the original author still the 'author' if somebody changes half of the content?
There was a problem hiding this comment.
I don't care, but I think maybe just a list of authors, like author "Anders Wallin, David Mueller"; ?
src/hal/components/toggle2nist.comp
Outdated
|
|
||
| """; | ||
|
|
||
| pin in bit in; | ||
| pin in bit is_on; | ||
| pin out bit on; | ||
| pin out bit off; | ||
| variable int old_in; | ||
| variable int to_state=0; | ||
| param rw u32 debounce = 2 "debounce delay in cycles"; |
There was a problem hiding this comment.
How can we disable debouncing? With debounce = 0 ?
However, isn't it better to have a default value without debouncing to not eventually change the behaviour of existing configurations and add a note like "if you connect a physical button, a debounce value of minimum 2 is strongly recommend!" ?
There was a problem hiding this comment.
I copied that over from the 'toggle' component which I presumed to be a good example.
As for existing configs, since the current component is buggy people had to find workarounds (see links above). So if somebody has put an additional 'toggle' component between the button and the 'toggle2nist' component to make it work then the fixed version of it will now require two button presses which is actually what one would expect when we read the description of the component. Not sure how to circumvent this unless we want to keep the broken things and just add the fixed things to it.
There was a problem hiding this comment.
Adding the debounce is a good think, I agree.
Only thinking that it adds some very little delay but I guess that would hurt nobody.
To be 100% safe we could use the update-ini script to disable the debouncing, but that would be cracking a walnut with a sledgehammer...
There was a problem hiding this comment.
I get your point and I have no problem with setting the default to 0. The only reason I chose 2 was to make the two 'toggle' components consistent.
Sigma1912
force-pushed
the
toggle2nist_fix_race_conditions
branch
from
83afcad to
adf5743
Compare
|
"param"s used to be a way to save HAL shared memory, but we have increased it so many times now that it probably isn't worth the bother. It's just about possible that someone might want to alter the debounce in HAL logic, so I suggest making it a pin. |
Sigma1912
force-pushed
the
toggle2nist_fix_race_conditions
branch
from
adf5743 to
c4d1540
Compare
src/hal/components/toggle2nist.comp
Outdated
|
|
||
| \[bu] On a rising edge on pin \\fIin\\fR when \\fIis-on\\fR is low: It sets \\fIon\\fR until \\fIis-on\\fR becomes high. | ||
|
|
||
| \[bu] On a rising edge on pin \\fIin\\fR when \\fIis-on\\fR is high: It sets \\fIoff\\fR until \\fIis-on\\fR becomes low. | ||
|
|
||
|
|
||
| ┐ ┌─────xxxxxxxxxxxx┐ ┌─────xxxxxxxxxxxx┐ | ||
| ┐ ┌─────xxxxxxxxxxxx┐ ┌─────xxxxxxxxxxxx┐ |
There was a problem hiding this comment.
Just for curiosity - what has changed here?
There was a problem hiding this comment.
I have asked myself the same as I have not changed anything in those lines. . I noticed there is something about this in the commit history:
https://github.com/LinuxCNC/linuxcnc/pull/3007/files
So not sure what this means, @petterreinholdtsen would probably know but if saving the file in a text editor breaks the diagram then it may be better to just remove it.
There was a problem hiding this comment.
I managed to fix this by copying the spacing from the 'toggle' component.
|
Just a comment, rather than something that needs to be changed, but it is preferred to use "signed" and "unsigned" as the type designators for integer pins. (as it says in the docs). The reason I mention this is that I hope to convert all integer hal pins to 64 bits before 2.10 is released. There is no point having 32 and 64 bit, in my opinion. Behind the scenes this will also involve widening some of the step and encoder internal values, hopefully making wrap-around impossible, rather than just unlikely. |
|
Comment here when you think that this is finished and ready. |
Sigma1912
force-pushed
the
toggle2nist_fix_race_conditions
branch
3 times, most recently
from
36d943d to
1f13544
Compare
Sigma1912
force-pushed
the
toggle2nist_fix_race_conditions
branch
from
1f13544 to
0bc97e6
Compare
|
Ready, as far as I'm concerned. |
|
Thanks |
This adds button input debounce and fixes issues when connecting to halui inputs:
https://forum.linuxcnc.org/24-hal-components/40557-problem-with-toggle2nist-component?start=0
https://forum.linuxcnc.org/42-deutsch/54560-mms-einschalten-mit-einem-taster