fix(sim): PushButton/Monoflop Reset handling; InputRef output mirroring - #15
fix(sim): PushButton/Monoflop Reset handling; InputRef output mirroring#15discostu105 wants to merge 2 commits into
Conversation
The parser signature for PushButton/PushButton2/PushButton2Sel only mapped InputTrigger and On, so a wired Reset landed at an input index the eval never read — a PushButton driven off via Reset stayed on forever. - parser: add Reset + InputDisable to the PushButton and PushButtonSel input signatures (synthesized when absent, so layouts stay stable) - interpreter: Reset dominates On and forces off; InputDisable gates the trigger edge (PushButton and PushButton2; assumed behavior, flagged) - JIT: same semantics in EvalStep::PushButton (usize::MAX sentinel for absent connectors) so interpreter/JIT equivalence holds Found while simulating the r50 UV-lamp weekday scenario (PushButton with On: weekday-Or, Reset: Not(weekday-Or)).
…d AQ - Monoflop: the parser signature lacked Reset, so a wired Reset was never read and a running pulse could not be aborted. Reset now clears the countdown and blocks retriggering while held (interpreter + JIT; assumed behavior, flagged). - InputRef: the impl forwarded I->Q and AI->AQ separately, but the Miniserver mirrors the one fed side onto BOTH outputs — the r50 corpus wires 'ref.AI <- mem.AQ' with consumers reading ref.Q. Q is now the digital view (non-zero -> 1) and AQ the analog value of whichever side is fed. Found while simulating the r50 alarm chain (Monoflop 'alarm_halten' with Reset: heimkommen_ref.Q, triggered through an AI-fed InputRef).
eisber
left a comment
eisber
left a comment
There was a problem hiding this comment.
Docker validation is green, but the new branches are not covered by the current suite. I found the following correctness and coverage gaps.
| let ai = inputs.get(1).copied().unwrap_or(0.0); | ||
| vec![i, ai] | ||
| let q = if i != 0.0 || ai != 0.0 { 1.0 } else { 0.0 }; | ||
| let aq = if ai != 0.0 { ai } else { i }; |
There was a problem hiding this comment.
ai != 0.0 uses the runtime value to infer which connector is fed. A connected analog input can legitimately produce 0.0; if I is also present, this then switches AQ to the digital value instead of preserving the analog zero. Please select the mirrored source from connector connectivity/presence rather than its current value, and add a regression case with AI = 0 and nonzero I.
| vec![i, ai] | ||
| let q = if i != 0.0 || ai != 0.0 { 1.0 } else { 0.0 }; | ||
| let aq = if ai != 0.0 { ai } else { i }; | ||
| vec![q, aq] |
There was a problem hiding this comment.
This behavior is not implemented in CompiledGraph: InputRef still falls through to the generic EvalStep::Copy, which copies only input 0 to output 0. Consequently an AI-only reference still produces compiled Q = 0, and compiled AQ is never written. Please add a dedicated compiled InputRef step that mirrors the selected input to both outputs, plus a non-ignored interpreter/JIT parity test for the motivating AI-only case.
| } else if is_high(force_on) { | ||
| self.is_on = true; | ||
| } else if !is_high(prev_trigger) && is_high(trigger) { | ||
| } else if !is_high(disable) && !is_high(prev_trigger) && is_high(trigger) { |
There was a problem hiding this comment.
InputDisable currently gates only the trigger, so Reset and On still change the block while it is active. Loxone's Switch documentation defines DisPc as “Disables all inputs when On,” which includes these inputs: https://www.loxone.com/enen/kb/switch/. Please apply the disable gate to Reset/On as well, consistently in PushButton, PushButton2, and the compiled step.
| ), | ||
| "PushButton" | "PushButton2" | "PushButton2Sel" => ( | ||
| &["InputTrigger", "On"], | ||
| &["InputTrigger", "On", "Reset", "InputDisable"], |
There was a problem hiding this comment.
Please add regression coverage for the newly exposed connectors. The current jit_equivalence connector table still supplies only the old PushButton/Monoflop signatures, and that broad test is ignored, so the passing suite never exercises Reset/InputDisable through parser/interpreter/JIT. Targeted tests should cover reset dominance/held reset, disable behavior, and absent/unwired connectors.
2 participants
Three simulator correctness gaps, found while simulating scenarios from a real Miniserver config (weekday-gated pool UV lamp, alarm hold chain) and fixed in both the interpreter and the JIT compiler so their equivalence holds.
PushButton / PushButton2: Reset and InputDisable were ignored
The parser signature only mapped
InputTriggerandOn, so a wiredResetconnector landed at an input index the eval never read — a PushButton driven off via Reset stayed on forever.Reset+InputDisableadded to the PushButton/PushButton2/PushButton2Sel and PushButtonSel input signatures (synthesized when absent, so existing layouts keep their indices)EvalStep::PushButton, with ausize::MAXsentinel for absent connectorsMonoflop: Reset input missing from the signature
A wired Reset was never read, so a running pulse could not be aborted. Reset now clears the countdown and blocks retriggering while held (interpreter + JIT).
InputRef: mirror the fed side onto BOTH outputs
The impl forwarded
I -> QandAI -> AQseparately, but the Miniserver mirrors the one fed side onto both outputs — real configs wireref.AI <- mem.AQwith consumers readingref.Q.Qis now the digital view (non-zero → 1) andAQthe analog value of whichever side is fed.Notes
WARNING: Assumed behavior — not validated against Miniservercomments in the code, following the dominance order that matches observed behavior in the configs I simulate; happy to adjust if you have reference traces.lox-simsuite passes (498 tests).