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;