sim: correctness fixes — wired parameters, PushButton/Monoflop Reset, InputRef mirroring - #16
sim: correctness fixes — wired parameters, PushButton/Monoflop Reset, InputRef mirroring#16discostu105 wants to merge 3 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).
Loxone lets any parameter (Formula Input1-Input4, comparator Input2, timer durations) be driven by a wire instead of a fixed value. The graph refused such wires (add_wire required dir == Input, degrading them to warnings), and both evaluators read parameters straight from the local connector signal, so a wired parameter silently kept its Def= value. - graph: add_wire accepts any non-Output sink - engine: BlockEvalInfo resolves param_sources through wires like input_sources (feedback reads prev-tick; autodiff duals follow too) - compiler (JIT): the input_source map covers parameter connectors and every EvalStep param index resolves through it Found via lxir's pool example: a VirtualIn wired into a Formula's Input1 evaluated as 0 instead of the injected value.
eisber
left a comment
eisber
left a comment
There was a problem hiding this comment.
Docker-based review found two compiled/interpreter consistency gaps in the new behavior.
| 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.
Compiled execution does not mirror InputRef. CompiledGraph::from_graph has no InputRef arm, so this block falls through to EvalStep::Copy, which only copies the first input to the first output. In a Docker repro with AI=42, SimEngine returns Q=1, AQ=42, but CompiledGraph returns Q=0, AQ=0. Please add an InputRef-specific compiled step covering both inputs and outputs, plus an enabled equivalence case using the real I, AI, Q, AQ layout.
| let params = &info.params; | ||
| // Wired parameters read their driving output's signal; unwired | ||
| // ones read their own connector (holding the Def= value). | ||
| let params: Vec<usize> = info.params.iter().map(|&cid| input_source[cid].0).collect(); |
There was a problem hiding this comment.
Wired Formula parameters are still ignored by compiled execution. Resolving parameter source indices here works for compiled block types such as Gain, but Formula has no compiled match arm and falls through to EvalStep::Copy. With Input1 wired to 5 and expression I1*2, the Docker repro returns 10 from SimEngine and 5 from CompiledGraph. Since parsed Formula operands are parameters rather than regular inputs, please implement compiled Formula semantics using these resolved parameter slots and cover a parsed/wired Formula in equivalence tests.
2 participants
Three independent correctness fixes for
lox-sim, found while building lxir'slxir testcommand, which compiles a test DSL into SimSpec JSON and runs it throughlox sim run --jsonagainst real configs.1. Wires into parameter connectors are honored (
8d44ffc)Loxone lets any parameter be driven by a wire instead of a fixed value — Formula
Input1–Input4, comparatorInput2, timer durations. Previously:Graph::add_wirerequired the sink to beConnectorDir::Input, so wires intoParameterconnectors were refused and degraded to warnings at parse time;Def=value.Now the graph accepts any non-Output sink, the interpreter resolves
param_sourcesthrough wires exactly likeinput_sources(feedback wires read prev-tick; autodiff duals follow the same resolution), and the JIT compiler'sinput_sourcemap covers parameter connectors with everyEvalStepparam index resolved through it.Observed failure: a
VirtualInwired into a Formula'sInput1evaluated as 0 instead of the injected value.2. Monoflop Reset; InputRef mirrors its fed side to Q and AQ (
1fb350d)Monoflopnow honors itsResetinput (cancels the running pulse).InputRefmirrors whichever side feeds it to bothQandAQ, matching how downstream blocks consume refs in real configs.3. PushButton honors Reset and InputDisable (
735bc8d)PushButton/PushButton2previously ignored theirResetandInputDisableconnectors.All three are test-covered (
cargo test -p lox-sim: 498 passing). Reset semantics that could not be validated against a live Miniserver are marked with the existing "WARNING: Assumed behavior" comment convention.Happy to split this into three PRs if you prefer.