Conversation
eddbaa4 to
fe78b58
Compare
|
Question on the premise: One thing I noticed in the toggle2nist hunk: with |
|
Exposed as the casual programmer, again. :/ Background: 
I was fairly certain that I had not seen this behavior before porting the components to the new api so I had a closer look and after changing those 'on', 'off' reads the issue was gone.
You are right, it should. I was thinking along the lines that I shouldn't read the pins I had just written to but that doesn't actually happen in the same cycle. So admittedly this is a case of 'I seem to have fixed it but I don't know why'. Maybe @BsAtHome knows?
Good catch, I didn't notice that. As usual, you have much more analytical eye the me. |
|
Ok, then this is not about the new API, but about the behavior change I flagged, that did seem indeed to be a better behavior. |
| rtapi_bool on_val = on; | ||
| rtapi_bool off_val = off; |
There was a problem hiding this comment.
I'm not sure what you are trying to gain by this. The values of on and off are only read once in the second else if. They are set in other clauses, but their written values do not depend on what was read.
Therefore, you are not gaining anything by doing this. Actually, you are making things slightly worse. The if(in_val...) clause, which unconditionally writes the pins now has both read (because you said to at the top) and then written with no reference to their original values.
|
My apologies, I totally did not analyze the actual issue. I'll come back when I have actually done the homework ... |
| debounce_cntr = 0; | ||
| } | ||
| } else if ((!ison_val && off) || (ison_val && on) || (pulse_length > max_pulse_length)) { | ||
| } else if ((!ison_val && state == 0) || (ison_val && state == 1) || (pulse_length > max_pulse_length)) { |
There was a problem hiding this comment.
Where does pulse_length get reset when a new pulse starts? At idle it counts 0..max and only the timeout branch zeroes it, so a fresh pulse inherits a random residual count and can expire a cycle or two in. Doesn't that explain the 1-in-30 short pulses on your scope?
This hunk pins it at 0 in the matched idle states, which is why your test passed. But what about startup with is_on already high, or a device switched externally while the button is held (ison_val and state disagree)? Wouldn't pulse_length = 0; next to state = 1; debounce_cntr = 0; in both edge branches cover every path?
| debounce_cntr = 0; | ||
| } | ||
| } else if ((!ison_val && off) || (ison_val && on) || (pulse_length > max_pulse_length)) { | ||
| } else if ((!ison_val && off_val) || (ison_val && on_val) || (pulse_length > max_pulse_length)) { |
There was a problem hiding this comment.
This comp keeps the original condition, so pulse_length still free-runs at idle here. Same starved-pulse failure you scoped on toggle2nist, isn't it? Same reset in both edge branches should fix it.
Both 'momentary2nist' and 'toggle2nist' suffer from a loose pulse_length counter that leads to 'on'-,'off'-signals getting cut short because the counter is not reset properly and counts when idling. The counter is now reset when the pins are changed and no longer counts when the component is idle. Also changes two misleading comments as the input pin change is debounced after the conditions are met.
fe78b58 to
bb2f7c6
Compare
3 participants
Two places did not get converted to the new hal api.
Also changes two misleading comments as the input pin change is debounced after the conditions are met.
Tested on real hardware