Skip to content

Fix issues in 'momentary2nist' and 'toggle2nist' components - #4576

Open
Sigma1912 wants to merge 1 commit into
LinuxCNC:masterfrom
Sigma1912:2nist_comps-fix-issues
Open

Sigma1912 wants to merge 1 commit into
LinuxCNC:masterfrom
Sigma1912:2nist_comps-fix-issues

Conversation

@Sigma1912

Copy link
Copy Markdown
Contributor
  • 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. Fix: The counter is now reset when the pins are changed and no longer counts when the component is idle.

  • 'toggle2nist' can endup being desynchronized when the toggle switch is flipped back and forth very fast. Fix: The input pin state is now compared to the state of the device so a switch position disagreeing with the device state will always trigger a change.

  • Both components now verify a succesful change in the device by comparing to the expected state rather than output pin values that change in idle.

  • For both components the idle state is now the default branch.

Comment thread src/hal/components/toggle2nist.comp Outdated
Comment thread src/hal/components/toggle2nist.comp Outdated
@Sigma1912
Sigma1912 force-pushed the 2nist_comps-fix-issues branch from 08c4bb9 to c3ee68f Compare September 20, 2026 14:56
}

if (in_val && state == 0 ) { /* input has changed from debounced 0 -> 1 */
if (in_val && !ison_val && state == 2 ) { /* change from 0 -> 1 has been requested */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

state zero-inits to 0, which the code below reads as "OFF pulse pending". If the device is already on at startup with the switch off (ison=1, in=0): both edge branches require state == 2, the wait branch counts to max, then the timeout branch runs forever without re-arming because in_val never changes. Isn't the startup desync then stuck until a double flip of the switch, which is exactly the case the description says is now always corrected? Would a one-shot init fix it (variables zero-init, so invert the flag):

if (!init_done) {
    state = 2;
    old_in_val = in_val;
    init_done = 1;
}

Comment thread src/hal/components/toggle2nist.comp Outdated
}
} else if ((!ison_val && off) || (ison_val && on) || (pulse_length > max_pulse_length)) {
// reset outputs once device has switched or maximum pulse length is reached
} else if ( (state < 2) && (ison_val != state) && (pulse_length < max_pulse_length) ) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this (and the timeout comparison below) use max_pulse_length_val? As written the clamp above never applies (setp max_pulse_length 5 takes effect raw), the pin goes through the getter twice per cycle, and the stashed local is set but never used.

Comment thread src/hal/components/toggle2nist.comp Outdated
Comment thread src/hal/components/momentary2nist.comp Outdated
}
} else if ((!ison_val && off) || (ison_val && on) || (pulse_length > max_pulse_length)) {
// reset outputs once device has switched or maximum pulse length is reached
} else if ( (ison_val != state) && (pulse_length < max_pulse_length) ) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With state == 2 at idle, ison_val != state is always true, so pulse_length counts to max once at idle. Harmless since the pulse start resets it, but wouldn't a state < 2 && gate make the intent clearer and match toggle2nist?

- 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.
Fix: The counter is now reset when the pins are changed and no longer counts when the component is idle.

- 'toggle2nist' can endup being desynchronized when the toggle switch is flipped back and forth very fast.
Fix: The input pin state is now compared to the state of the device so a switch position disagreeing with
the device state will always trigger a change.

- Both components now verify a succesful change in the device by comparing to the expected state rather than
  output pin values that change in idle.

- For both components the idle state is now the default branch.

- remove unnecessary debounce reset when waiting for device confirmation

- set max_pulse_length to a more suitable default value and make it the minimum for toggle2nist

- clarification in the description
@Sigma1912
Sigma1912 force-pushed the 2nist_comps-fix-issues branch from c3ee68f to c83e088 Compare September 21, 2026 14:24
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.

2 participants