backlog: isolate per-sequence state to prevent flag corruption across sequences#24880
backlog: isolate per-sequence state to prevent flag corruption across sequences#24880joluxer wants to merge 4 commits into
Conversation
BacklogLoop previously left backlog_timer at the value written by CommandHandler at the start of ExecuteCommand -- millis() at command start time plus P_BACKLOG_DELAY. After a long execution (rule-trigger chain) that point was already past: TimeReached() returned true immediately on the next iteration, draining the queue without pause regardless of SetOption34. A nested CmndBacklog reset the timer to millis() during execution, compounding the problem: the queue drained as fast as commands were enqueued. BacklogLoop now unconditionally sets backlog_timer after each timed drain step. The inter-command interval becomes deterministic: exactly P_BACKLOG_DELAY between drain steps, regardless of what CommandHandler or nested Backlog commands wrote during ExecuteCommand. Exception: CmndDelay called from within a drain sets backlog_delay_guard (detected via backlog_mutex=true). BacklogLoop preserves the timer for that one step and clears the guard. External commands can still shorten an active delay -- that matches original Tasmota behaviour.
Introduce SettingsParam(index) as a named function returning a uint8_t reference to Settings->param[index]. Adds bounds protection: an index >= PARAM8_SIZE returns a reference to a stable zero byte instead of an out-of-bounds access. Replace three direct Settings->param[] accesses in the SetOption handler (CmndSetoption, GetOption, CmndSetoptionBase) to use the new accessor.
Move the backlog queue, timer, and control flags out of TasmotaGlobal_t and the global .ino scope into a dedicated translation unit (support_backlog.cpp). The state was accessible from any .ino file; the new module limits mutation to explicit paths through the Backlog:: API. All internal state is held in an anonymous namespace. The public interface is declared in support_backlog.h. A one-line support_backlog.ino adapter keeps BacklogLoop() visible to the unity build. Removes from TasmotaGlobal_t: backlog_timer, backlog_nodelay, backlog_mutex, backlog_delay_guard, backlog_no_mqtt_response. Removes global LList<char*> backlog. Adds SuppressMqttResponse() to support.ino as the designated mutation path for TasmotaGlobal.no_mqtt_response from backlog context. No intended behavior change.
SetOption166 (default 0) controls whether external commands (MQTT, Serial, Button, ...) extend the Backlog drain window after execution. SO166=0: CommandHandler sets the drain timer to millis() + BACKLOG_EXT_DELAY (100 ms, separate from SO34, overridable at build time via user_config_override.h). SO166=1: BacklogLoop is the sole timer owner; external commands do not stall the drain. The inter-command delay within a sequence (SO34) is unchanged. Each queue entry now carries a flavor byte (bit0=nodelay, bit1=no_mqtt_resp) baked in from staged flags at enqueue time. D_CMND_NODELAY is resolved in CmndBacklog() and ExecuteCommandBlock() at enqueue rather than stored as a queue token. Loop() reads the flavor byte per drain step; _nodelay and _no_mqtt_resp each split into _staged (set by callers) and _current (set from the flavor byte, read by IsNodelay()). ExecuteCommandBlock() resets staged flags to plain-Backlog defaults before its tokenisation loop: IF blocks no longer inherit ambient state from a preceding CmndBacklog() call. ScheduleDelay() is a no-op when _nodelay_current is set. Default-configuration behavior: the inter-command delay (SO34) is unchanged. The post-external-command drain window changes from SO34 to BACKLOG_EXT_DELAY (100 ms vs. 200 ms default) -- this is intentional and separately configurable.
|
Please review this: PR 24880 Review FindingsBlocking issue:
|
|
Thanks for the suggestion, I like the idea. I'll look at it during weekend, when I'm back at my laptop. |
Description
Part of the backlog improvement series documented in discussion #24776.
Implementation choices are open to discussion. If you'd prefer a different approach, please comment -- I'll revise.
Problem
Bug -- Concurrent-rule flag clobber:
Rule1: Backlog0 cmd1;cmd2andRule2: Backlog cmd3;cmd4firing together:all four commands ran timed. The second rule's enqueue reset the global
nodelayflag to false before the first rule's commands were drained.Per-entry intent was silently discarded.
Root cause:
Behavioral intent (
nodelay,no_mqtt_response) was stored asglobal state, set at enqueue and consumed at drain. Any intervening
enqueue or command run could overwrite it.
Changes
Fix: intent baked in at enqueue time (bit0=nodelay, bit1=no_mqtt_resp):
Each queue entry now carries its behavioral flags as a prefix byte, set at
enqueue from the staged state. Flags travel with the command;
globals no longer affect them at drain-time.
NoDelayremains one-shot: raises the nodelay flag for exactly the nextenqueued command, then reverts to the sequence baseline (
Backlog0/2: allnodelay;
Backlog/1/3: timed). This matches the documented one-command scopeand is now structurally guaranteed rather than dependent on drain-loop state.
ExecuteCommandBlock(IF/ENDIF engine) resets staged flags to plain-Backlogdefaults before inserting commands. IF blocks now behave like a fresh
Backloginvocation: timed, MQTT on. Authors who want
NoDelayinside an IF blockwrite it explicitly -- readable and correct.
SetOption166 (default 0) -- separate drain window for external commands:
Previously, external commands (MQTT, Serial, Button) stalled the drain window
via SO34 -- the same timer that governs inter-command spacing within a Backlog
sequence. This conflated two independent concerns.
SO166=0 (default): external commands schedule
BACKLOG_EXT_DELAY(100 ms,configurable via
user_config_override.h), separate from SO34. Existingbehavior is preserved for users who do not set SO166. Backlog queue drain stalls
for short time.
SO166=1:
BacklogLoopis sole timer owner. External commands do not stall thebacklog queue drain. The inter-command delay within a Backlog sequence (SO34) is
unchanged in either mode.
Backlog::WarnIfNoDelay(cmd_name_P):Diagnostic hook for command handlers with settling-time requirements or
interlock dependencies making them unsafe at zero inter-command delay. Logs at
ERROR level when called from a NoDelay drain context. Available for any driver
or command handler to opt in.
Delaycommand uses it.Behavior summary
Rule1: Backlog0 cmd1;cmd2/Rule2: Backlog cmd3;cmd4(both fire)Backlog2 cmd1; IF cond; cmd2; ENDIFBacklog POWER1; NoDelay; POWER2; POWER3POWER2no delayDelayinsideBacklog0Backlog0 cmd1; cmd2Default
Backlog/Backlog1behavior: no change.Memory impact (measured, this PR only)
Ref: pr/backlog-encapsulation tip
RAM −8 B on tasmota32: per-entry flags replace three drain-loop globals.
Flash cost is the expanded queue entry and IF-block reset logic.
Requires pr/backlog-encapsulation: staged flags and queue state must be
inaccessible from
.inoscope for the per-entry guarantee to hold.Build-tested on ESP8266 (tasmota-4M) and ESP32 (tasmota32). Target-tested on ESP32: completed.
Checklist: