backlog: Contain flag mutation side-effects#24879
Open
joluxer wants to merge 3 commits into
Open
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.
Collaborator
|
Thanks, good for me. @arendst ok to merge? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Context
The previous PR fixed the backlog timer:
BacklogLoopnow unconditionallyowns
backlog_timerafter each drain step. That fix depends on one invariant:backlog_mutexis set and cleared exclusively byBacklogLoop.That invariant currently has no enforcement mechanism -- it holds by
convention.
backlog_mutex,backlog_timer, andbacklog_nodelaysit inTasmotaGlobal_t, writable from any.inofile without restriction. Adriver or rule handler that touches these fields will silently break the
previous fix without any compiler or linker feedback.
This PR removes that exposure by making the fields inaccessible outside their
translation unit. All mutation now goes through explicit entry points; accidental
modification from unrelated code paths is structurally impossible.
Changes
Protection boundary
backlog_mutex,backlog_timer,backlog_nodelay,backlog_delay_guard,backlog_no_mqtt_response, and the backlog queue are no longer reachable from.inoscope. All mutation goes through explicit entry points in a newBacklog::API.Implementation
tasmota/tasmota_support/support_backlog.cpp-- state in anonymousnamespace
tasmota/include/support_backlog.h-- public API declarationstasmota/tasmota_support/support_backlog.ino-- one-line adapter:BacklogLoop()forwards toBacklog::Loop()SuppressMqttResponse()added tosupport.inoas designated mutation pathfor
no_mqtt_responsefrom backlog context (replaces direct field access).SettingsParam(index)added as bounds-checked reference getter forSettings->param[]- a small bonus for the whole system.All callers in
support_command.inoandxdrv_10_rules.inomechanicallyconverted to the
Backlog::API. No logic changed.Memory impact (measured, this PR only)
The flash delta is the cost of the enforcement boundary. Without it, the
invariant from the previous PR remains advisory. The RAM delta reflects
alignment differences between anonymous-namespace variables in a
.cpptranslation unit and packed fields in
TasmotaGlobal_t.No logic changed.
Requires pr/backlog-timer-fix: this PR enforces at module level the timer-ownership invariant established by the previous fix.
Tested:
Build-tested on ESP8266 (tasmota-4M) and ESP32 (tasmota32). Target-tested on ESP32: completed.
Checklist: