Fix #112 watchdog race in Out::update() causing spurious disarm#118
Merged
Conversation
The check 'micros() - _watchdog_ts >= OUT_MOT_TIMEOUT' has no guaranteed evaluation order between the micros() call and the _watchdog_ts load. If set_output() runs between those two reads (e.g. from another task/core), micros() can be sampled before _watchdog_ts is reloaded with a later timestamp. The unsigned subtraction then underflows and the comparison spuriously trips, disarming motors mid-flight (the OUT: ARMED stutter). Snapshot _watchdog_ts into a local before reading micros(), and mark _watchdog_ts volatile so the compiler cannot reload it between the two references. Hardware-verified workaround by the reporter on Arduino Nano ESP32.
Owner
|
@evanofficial Many thanks for fixing this! |
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
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.
Fixes #112
Problem
Out::update()checks the motor watchdog with:C++ does not specify evaluation order between the
micros()call and the load of_watchdog_ts. Ifset_output()(ortestmotor_set_output()) updates_watchdog_tsfrom another task/core between those two reads,micros()can be sampled with an older value while_watchdog_tsis reloaded with a newer one. The unsigned subtraction then underflows to a value far greater thanOUT_MOT_TIMEOUT, the comparison trips, and the motors are spuriously disarmed mid-flight — exactly theOUT: ARMEDstutter the reporter observed.The reporter (@slavicd) hardware-verified on an Arduino Nano ESP32 that snapshotting both values into locals before the comparison makes the stutter unreproducible.
Fix
_watchdog_tsinto a local before readingmicros(), locking in evaluation order._watchdog_tsasvolatileso the compiler cannot reload it between the two references or hoist the load across themicros()call.Architectures
_watchdog_tsisuint32_t, so individual loads/stores are already atomic on every supported target (ESP32-S3/ESP32, RP2350/RP2040, STM32 — all 32-bit cores). No platform-specific atomics or critical sections are needed; the fix is portable C++ and compiles cleanly under both Arduino IDE and PlatformIO.Audit
All readers and writers of
_watchdog_tswere audited:Out::update()(line 181) — fixedOut::set_output(),Out::testmotor_enable(),Out::testmotor_set_output()— unchanged, none in ISR contextTest plan