From bef289ef42205d80b9cd12cac03e032ebe63584a Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Mon, 14 Sep 2026 09:33:53 +0000 Subject: [PATCH] fix(extnotif): make isNagging the only armed flag for the nag cycle (#11828) * fix(extnotif): make isNagging the only armed flag for the nag cycle ExternalNotificationModule kept the nag cycle's armed state in two places that could disagree: the isNagging bool, and nagCycleCutoff reserving UINT32_MAX for "not armed". handleInputEvent() read only the second one: if (nagCycleCutoff != UINT32_MAX) { stopNow(); return 1; } The field is declared `= 1`, while isNagging starts false, so at boot that test said "armed" when nothing was nagging. The first input event of every boot was therefore answered with stopNow() and a non-zero return - and a non-zero return ends the observer chain (Observable::notifyObservers in src/Observer.h returns on the first one), so that event was swallowed from every later observer. The handler is registered whenever external_notification.enabled, and InputBroker only short-circuits while nagging() is true, so the event does reach it. The same read had a second failure mode once per ~49.7-day wrap: armNagCycle() computes `millis() + durationMs`, which can land exactly on UINT32_MAX. When it does, a real nag is running with isNagging true, but this read says "not armed" and the module's own handler never stops it. Time::skipZero() cannot help here - it lifts 0 to 1 and leaves UINT32_MAX alone, which src/UptimeClock.h static_asserts. So the fix is not a zero guard, it is removing the second opinion. isNagging is the armed flag - which is what the comment above the expiry check already claimed, and what the other four reads already use - and nagCycleCutoff is now only ever a deadline, read after isNagging has been checked. Nothing reserves a value, which matters because an arm site spelled `millis() + interval` can produce any value there is, so no value is safe to reserve. That is the shape the TODO(deadline-type) note in src/mesh/Throttle.h is aiming at, and that note is updated to match rather than keep describing the sentinel this removes. Worth knowing for review, though not changed here: InputBroker::handleInputEvent already calls stopNow() itself when nagging() is true, and returns without notifying observers. Every path that starts a notification calls armNagCycle() first, so isNagging is true for the whole life of any real nag. That makes this handler reachable only when there is nothing to stop - its stopNow() was never doing useful work. Gated rather than deleted, because removing a public handler and its observer registration is a bigger call than fixing the defect. Co-Authored-By: Claude Opus 5 * style(extnotif): trim comments to the house limit --------- Co-authored-by: Claude Opus 5 Co-authored-by: nomdetom --- src/mesh/Throttle.h | 6 +++--- src/modules/ExternalNotificationModule.cpp | 11 ++++++----- src/modules/ExternalNotificationModule.h | 4 +++- test/test_throttle/test_main.cpp | 8 ++++++-- 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/mesh/Throttle.h b/src/mesh/Throttle.h index bd16ab1ccf..86740f5777 100644 --- a/src/mesh/Throttle.h +++ b/src/mesh/Throttle.h @@ -32,7 +32,7 @@ class Throttle /// Deadline::in(ms) / .armed() / .passed() / .disarm(). A hand-built `now + interval` could then /// no longer land on the sentinel by accident, and "armed" would stay a question separate from /// "passed" - the split that has to survive, because which way "inactive" falls is the caller's - /// to decide. Same size and cost as the bare uint32_t. The conversion sites, grouped by the four + /// to decide. Same size and cost as the bare uint32_t. The conversion sites, grouped by the three /// meanings they give the sentinel today: /// 0 = unarmed - Power.cpp rebootAtMsec/shutdownAtMsec, GPS.cpp fixHoldEnds, AdminModule.cpp /// enterDfuAtMsec and the other timerEndsAtMillis()/skipZero() arm sites dodge @@ -41,8 +41,8 @@ class Throttle /// guard, so this third state wants naming rather than repeating. /// 0 = due now - ethClient.cpp ntp_renew, forced at link-up. A computed renewal now dodges 0, /// so only a deliberate write still means "due now". - /// UINT32_MAX - ExternalNotificationModule.cpp nagCycleCutoff, whose armed() also lives in a - /// second variable (isNagging) and whose arm site can land on the sentinel. + /// ExternalNotificationModule.cpp nagCycleCutoff reserves nothing: isNagging is the only armed + /// flag and the deadline is read only while it is set. static bool deadlinePassed(uint32_t deadlineMs); /// deadlinePassed() against a caller-supplied "now", for a loop that snapshots the time once and diff --git a/src/modules/ExternalNotificationModule.cpp b/src/modules/ExternalNotificationModule.cpp index ef0627f06b..28eaf6ccd8 100644 --- a/src/modules/ExternalNotificationModule.cpp +++ b/src/modules/ExternalNotificationModule.cpp @@ -88,12 +88,11 @@ int32_t ExternalNotificationModule::runOnce() #if defined(HAS_I2S_SPEAKER_NRF52) isRtttlPlaying = isRtttlPlaying || nrf52RtttlPlayer.isPlaying(); #endif - // isNagging is the armed flag; nagCycleCutoff holds a real deadline only while it is set - // (UINT32_MAX once stopped, 1 at boot), so short-circuit before the comparison. + // isNagging is the armed flag; nagCycleCutoff is only a deadline while it is set, so + // short-circuit before the comparison. `millis() + durationMs` can land on any value. const bool nagWindowExpired = !isNagging || Throttle::deadlinePassed(nagCycleCutoff); if (nagWindowExpired && !isRtttlPlaying) { // Turn off external notification immediately when timeout is reached, regardless of song state - nagCycleCutoff = UINT32_MAX; ExternalNotificationModule::stopNow(); isNagging = false; return INT32_MAX; // save cycles till we're needed again @@ -309,9 +308,9 @@ void ExternalNotificationModule::stopNow() #endif // Prevent the state machine from immediately re-triggering outputs after a manual stop. + // Clearing isNagging disarms the cycle; nagCycleCutoff is never read without it. isNagging = false; buzzerShouldAlert = false; - nagCycleCutoff = UINT32_MAX; #ifdef HAS_I2S // GPIO0 is used as mclk for I2S audio and set to OUTPUT by the sound library @@ -624,7 +623,9 @@ void ExternalNotificationModule::handleSetRingtone(const char *from_msg) #if !MESHTASTIC_EXCLUDE_INPUTBROKER int ExternalNotificationModule::handleInputEvent(const InputEvent *event) { - if (nagCycleCutoff != UINT32_MAX) { + // Testing the deadline instead of isNagging was true at boot, and the non-zero return + // swallowed the first input event from every later observer. + if (isNagging) { stopNow(); return 1; } diff --git a/src/modules/ExternalNotificationModule.h b/src/modules/ExternalNotificationModule.h index 969638583c..95ff5b4d66 100644 --- a/src/modules/ExternalNotificationModule.h +++ b/src/modules/ExternalNotificationModule.h @@ -64,7 +64,9 @@ class ExternalNotificationModule : public SinglePortModule, private concurrency: int handleInputEvent(const InputEvent *arg); #endif - uint32_t nagCycleCutoff = 1; + /// When the current nag cycle ends. Meaningful only while isNagging is set; never test it for a + /// magic value. + uint32_t nagCycleCutoff = 0; void setExternalState(uint8_t index = 0, bool on = false); bool getExternal(uint8_t index = 0); diff --git a/test/test_throttle/test_main.cpp b/test/test_throttle/test_main.cpp index e2630ba3dd..b0fe23a04c 100644 --- a/test/test_throttle/test_main.cpp +++ b/test/test_throttle/test_main.cpp @@ -148,8 +148,12 @@ void test_deadlinePassed_reads_disarmed_sentinels_as_passed() { Time::setTestMillis(6247); - TEST_ASSERT_TRUE(Throttle::deadlinePassed(0)); // "inactive" for rebootAtMsec et al - TEST_ASSERT_TRUE(Throttle::deadlinePassed(UINT32_MAX)); // "inactive" for nagCycleCutoff + TEST_ASSERT_TRUE(Throttle::deadlinePassed(0)); // "inactive" for rebootAtMsec et al + + // UINT32_MAX is not a usable "far future" either - at a low uptime it is a hair BEHIND now, so + // it reads as passed like any other past value. ExternalNotificationModule used to reserve it + // for "unarmed" and now keeps that state in its isNagging flag instead. + TEST_ASSERT_TRUE(Throttle::deadlinePassed(UINT32_MAX)); // The guarded form every caller must use. const uint32_t disarmed = 0;