From f0e8b1d896594277b34aa86c0bc31b35eced4aaf Mon Sep 17 00:00:00 2001 From: cjkas Date: Tue, 13 Oct 2026 15:05:06 +0200 Subject: [PATCH] fix: refuse a radio job when its outcomes might not fit in the event ring A full event ring threw an outcome away, breaking the rule that every job we submit ends with exactly one of finished, aborted or dropped. A lost outcome left a favorite uncommitted and a held shade waiting out its 30s timeout. submit() and hasRoom() now keep room for two outcomes of every job not yet ended, so an event can't be lost; if loop() stops collecting them, new jobs are refused instead. Co-Authored-By: Claude Opus 5.5 --- src/RadioScheduler.cpp | 15 +++++- src/RadioScheduler.h | 6 ++- .../test_radioscheduler.cpp | 50 +++++++++++++++++++ 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/RadioScheduler.cpp b/src/RadioScheduler.cpp index 7e1bfcf..dea13e2 100644 --- a/src/RadioScheduler.cpp +++ b/src/RadioScheduler.cpp @@ -21,6 +21,10 @@ uint32_t RadioScheduler::submit(somfy_tx_job_t &job, bool priority, uint16_t del ESP_LOGE(TAG, "Radio TX queue full, dropping transmission"); return 0; } + if(!this->outcomesFit()) { + ESP_LOGE(TAG, "Radio TX outcomes not collected, dropping transmission"); + return 0; + } // 0 means no job so it is skipped on wrap. if(++this->lastSeq == 0) ++this->lastSeq; job.seq = this->lastSeq; @@ -40,7 +44,13 @@ uint32_t RadioScheduler::submit(somfy_tx_job_t &job, bool priority, uint16_t del return job.seq; } bool RadioScheduler::hasRoom(bool priority) const { - return !this->suspendMask && admits(TX_JOB_QUEUE_SIZE - this->count, priority); + return !this->suspendMask && admits(TX_JOB_QUEUE_SIZE - this->count, priority) && this->outcomesFit(); +} +bool RadioScheduler::outcomesFit() const { + // Each job not yet ended reports at most two outcomes: started, then how it + // ended. Counting repeater frames too keeps this simple and errs safe. + const uint32_t unended = this->count + (this->taken ? 1u : 0u) + 1u; + return this->eventCount + 2u * unended <= TX_EVENT_QUEUE_SIZE; } bool RadioScheduler::waitingFor(uint32_t remoteAddress) const { for(uint8_t i = 0; i < this->count; i++) { @@ -112,7 +122,8 @@ void RadioScheduler::post(tx_event_t type, const somfy_tx_job_t &job, uint32_t a // Repeater frames don't belong to any of our remotes. if(job.raw) return; if(this->eventCount >= TX_EVENT_QUEUE_SIZE) { - ESP_LOGW(TAG, "Radio TX event queue full"); + // submit() leaves room for every outcome so this can't happen short of a bug. + ESP_LOGE(TAG, "Radio TX event queue full, outcome lost"); return; } somfy_tx_event_t &evt = this->events[(this->eventHead + this->eventCount) % TX_EVENT_QUEUE_SIZE]; diff --git a/src/RadioScheduler.h b/src/RadioScheduler.h index 88808c0..9a3ded9 100644 --- a/src/RadioScheduler.h +++ b/src/RadioScheduler.h @@ -18,7 +18,8 @@ class RadioScheduler { public: // Queues a job. Its ticket comes back and is also set in job.seq, 0 when - // the queue is full or suspended. A priority job (a stop) may use the + // the queue is full or suspended, or when outcomes aren't collected with + // popEvent() fast enough to leave room for its own. A priority job (a stop) may use the // reserved slots and goes ahead of every job queued at the back, unless a // job of its remote is already waiting: overtaking that would put the // rolling codes out of order. A delayed job doesn't go on air until @@ -66,6 +67,9 @@ class RadioScheduler { somfy_tx_event_t events[TX_JOB_QUEUE_SIZE * 3]; uint8_t eventHead = 0; uint8_t eventCount = 0; + // Whether the outcomes of every job not yet ended, and of one more, still + // fit in events. A job is refused otherwise so no outcome is ever lost. + bool outcomesFit() const; // Whether a job of the remote is still waiting. bool waitingFor(uint32_t remoteAddress) const; void remove(uint8_t i); diff --git a/test/test_radioscheduler/test_radioscheduler.cpp b/test/test_radioscheduler/test_radioscheduler.cpp index 3ff0887..4d35465 100644 --- a/test/test_radioscheduler/test_radioscheduler.cpp +++ b/test/test_radioscheduler/test_radioscheduler.cpp @@ -356,6 +356,54 @@ static void test_every_job_gets_one_outcome_across_a_flush() { } for(uint8_t i = 0; i < 6; i++) TEST_ASSERT_EQUAL_UINT8(1, outcomes[i]); } +static void test_no_outcome_is_lost_when_events_arent_collected() { + // loop() collects the events. If it stalls, jobs are refused rather than + // their outcomes thrown away. + RadioScheduler s; + uint32_t accepted = 0; + for(uint32_t i = 0; i < 200; i++) { + if(!submit(s, REMOTE_A)) break; + accepted++; + runNext(s); + } + TEST_ASSERT_TRUE(accepted > 0); + TEST_ASSERT_TRUE(accepted < 200); + TEST_ASSERT_FALSE(s.hasRoom(false)); + uint32_t started = 0, finished = 0; + somfy_tx_event_t evt; + while(s.popEvent(evt)) { + if(evt.type == tx_event_t::started) started++; + else if(evt.type == tx_event_t::finished) finished++; + } + TEST_ASSERT_EQUAL_UINT32(accepted, started); + TEST_ASSERT_EQUAL_UINT32(accepted, finished); + TEST_ASSERT_TRUE(s.hasRoom(false)); + TEST_ASSERT_NOT_EQUAL(0, submit(s, REMOTE_A)); +} +static void test_flush_reports_every_drop_when_events_arent_collected() { + RadioScheduler s; + uint32_t tickets[64]; + uint8_t n = 0; + // Outcomes of jobs already sent take up part of the event ring. + for(uint8_t i = 0; i < 20; i++) { + tickets[n] = submit(s, REMOTE_A); + TEST_ASSERT_NOT_EQUAL(0, tickets[n]); + n++; + runNext(s); + } + // Queue as many as are taken. + for(uint32_t t; n < 64 && (t = submit(s, REMOTE_B, 0, true)) != 0;) tickets[n++] = t; + TEST_ASSERT_TRUE(n > 20); + s.flush(0); + uint8_t outcomes[64] = {}; + somfy_tx_event_t evt; + while(s.popEvent(evt)) { + if(evt.type == tx_event_t::started) continue; + for(uint8_t i = 0; i < n; i++) + if(tickets[i] == evt.seq) outcomes[i]++; + } + for(uint8_t i = 0; i < n; i++) TEST_ASSERT_EQUAL_UINT8(1, outcomes[i]); +} static void test_unknown_ticket_is_ignored() { RadioScheduler s; TEST_ASSERT_FALSE(s.keepSending(0)); @@ -454,6 +502,8 @@ int main(int argc, char **argv) { RUN_TEST(test_flush_cuts_short_the_job_on_air); RUN_TEST(test_job_flushed_before_it_starts_is_dropped); RUN_TEST(test_every_job_gets_one_outcome_across_a_flush); + RUN_TEST(test_no_outcome_is_lost_when_events_arent_collected); + RUN_TEST(test_flush_reports_every_drop_when_events_arent_collected); RUN_TEST(test_unknown_ticket_is_ignored); RUN_TEST(test_suspend_flushes_and_refuses_jobs); RUN_TEST(test_resume_needs_every_reason_cleared);