Repository navigation
Fix seven storage, capture and UI defects found by review and bench testing - #6
Merged
Merged
Conversation
All three convert a recoverable fault into damage. None of them is the crash that was reported earlier; these were found by review. 1. Out-of-bounds write in consumeDecode() [ir_service.cpp] The bound and the write loop walked different index ranges. The library's getCorrectedRawLength() budgets over rawbuf[0..rawlen-2]; the write loop covers rawbuf[1..rawlen-1]. So the budget reserved split-space for the leading gap, which is dropped and never written, and reserved none for the final element, which is. An interval above 65535 us in that last slot costs two unbudgeted entries and runs past the end of capBuf_. The leading gap is normally the largest value in the buffer, so its surplus has been masking the shortfall. That is luck, not correctness. Now counted by walking exactly the elements the loop will read. A hard guard inside the loop stays as defence in depth: this writes a fixed buffer from interrupt-sourced data, so a future arithmetic slip should surface as a reported error rather than silent corruption. 2. LittleFS left unmounted after a failed filesystem OTA [web_ui_admin.cpp] A filesystem update unmounts before Update.begin(). If that call failed, nothing remounted: the early return at UPLOAD_FILE_END skipped the remount entirely, and the device ran with no filesystem until the next reboot -- no web UI, no stored commands, no schedule saves. Tracked with a flag and remounted from every terminal path, including otaFinish() as a backstop. The remount is now begin(false), not begin(true). An update that died partway can leave the partition half-written, and formatting it silently would destroy every learned command with no confirmation. Staying unmounted is recoverable; the PROGMEM recovery page still serves and accepts a filesystem upload. 3. Schedules deleted by a storage glitch [main.cpp, schedules.cpp] pruneMissingCommands() ran unconditionally at boot and persists its result. If the store failed to mount, or came up empty after an emergency format, every schedule looked orphaned and was permanently deleted -- while the commands themselves were very likely still intact on a partition that simply could not be read that once. Now gated on irStore.begin() succeeding, plus a second guard that refuses to prune when the store is empty but schedules exist. Orphaned schedules cost nothing: they fail to fire and show as "missing command". Related, same family: IrStore::begin() used LittleFS.begin(true), so any damaged partition was silently reformatted and the boot log looked normal afterwards. It now attempts a plain mount first and says out loud what it is about to erase before falling back to a format. Verified on hardware over OTA: all four targets build warning-free, the loopback self-test still passes, and a schedule survives a reboot with its command store intact. The OTA-failure and corrupt-partition paths are reasoned about rather than exercised -- reproducing them means deliberately corrupting a filesystem holding real captures. Co-Authored-By: Claude Opus 5 <[email protected]>
Two more review findings. A third, a claimed receiver lockout after transmit while the monitor is active, is not reproduced -- see below. 4. "Discard" was a no-op on a finished capture [ir_service.cpp] cancelLearn() only reset the state while Waiting, so pressing Discard on a Captured signal changed nothing: the state stayed Captured and the UI redrew the same card. The buffer kept the data too, so a following "capture another part" appended to the signal the user had just rejected, silently mixing it into a new command. It now clears the state, the error, and the whole pending capture from whichever state it is called in. The other callers -- OTA start and ArduinoOTA onStart -- want a full reset anyway. 9. No way to clear a stored password [web_ui_admin.cpp, data/] Both layers refused an empty secret: the firmware required strlen(...) > 0, and the browser only sent a field when non-empty. So no request could ever clear one. You could not join an open WiFi network, drop to an anonymous MQTT broker, or reopen the setup AP. The only escape was a factory reset, which also destroys every learned command. The API now follows ordinary REST semantics: absent means unchanged, present means write this -- including an empty string. Typing nothing still means "no change", which is what people expect of a password box, so each field gained an explicit "Clear it" tick that sends the empty string, with a note saying what clearing that particular secret implies. The ticks reset after a save so a second save cannot repeat a clear that was meant once. Verified in a browser: an untouched form sends no secret fields at all, ticking Clear sends "", and typing sends the typed value. Verified on hardware over OTA: setting mqttPass sets mqttPassSet, omitting it leaves it set, and an explicit "" clears it; learn cancel returns to idle with the capture buffer emptied. Not fixed -- finding 6, receiver lockout after transmit with the monitor active. Every path is balanced: sendStored, testCapture, sendRawArray and selfTest each restore the receiver they disabled, enableIRIn() resumes internally, txBusy_ is cleared on all exits including the send-failure path, and the monitor branch in loop() always resumes. Rather than edit the receiver state machine on an unreproduced report -- which is how the null timer crash got in -- this needs a bench test with a real remote. Co-Authored-By: Claude Opus 5 <[email protected]>
WiFi scan found nine networks and offered one. The SSID box was an <input list="ssid-options">, and a datalist filters itself against whatever is already typed in its input. That box is pre-filled with the current network, so the list narrowed to the one SSID that matched -- while the status line cheerfully said nine were found. The wrong control for the job, and its failure mode gives the user no clue. Replaced with an explicit tap-to-pick list: signal bars, RSSI, open networks flagged, and the tapped row highlighted before focus moves to the password box. Duplicates are still collapsed and the strongest sorted first. The SSID field stays typeable for hidden networks. Also adds tools/ir-backup.sh. A filesystem update erases every learned command; the docs have always said to back up first, but the only way to do it was clicking through the browser, which is no use in the middle of a flashing script. save/restore/list against the REST API, restoring one command per request because that is what the device can accept. Written after hand-rolling the same extraction regex twice and getting it wrong the second time, which came within one command of losing a real capture. Verified in a browser: four mock networks all render where the datalist would have shown one, and tapping a row fills the SSID. Verified on hardware: six commands backed up, filesystem OTA'd, all six restored with timings and frame counts intact. Co-Authored-By: Claude Opus 5 <[email protected]>
The PR template asks for docs and a changelog entry, so these are the ones this branch actually owes. - API.md: the /api/settings password contract changed. It used to say an empty string could never blank a secret; now absent means unchanged and an explicit empty string clears, which is what makes joining an open network possible. Notes that clearing authPass also forces authEnabled off. - USAGE.md: documents tools/ir-backup.sh, and states plainly that a firmware update keeps commands while a filesystem update erases them -- the distinction that cost captures twice during testing. - CHANGELOG.md: an Unreleased section covering all seven fixes and the new tool. Co-Authored-By: Claude Opus 5 <[email protected]>
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.
What this changes
Seven defects found by code review and by bench testing on real hardware, plus a
backup CLI. Five were reported findings; two more surfaced while verifying them.
Storage and capture safety
consumeDecode(). The bound came from thelibrary's
getCorrectedRawLength(), which budgets overrawbuf[0..rawlen-2],while the write loop walks
rawbuf[1..rawlen-1]. Different sets: it reservessplit-space for the leading gap we drop and never write, and none for the final
element we do write. An interval above 65535 us in that last slot costs two
unbudgeted entries and runs past the end of
capBuf_. The leading gap isnormally the largest value in the buffer, so its surplus had been masking the
shortfall — luck, not correctness. Now counted over exactly the elements the
loop reads, with a hard guard inside the loop as defence in depth.
unmounts before
Update.begin(); if that call failed, nothing remounted andthe device ran with no storage until reboot — no web UI, no commands, no
schedule saves. Tracked with a flag and remounted from every terminal path.
The remount is now
begin(false): quietly reformatting a half-writtenpartition would destroy every learned command with no confirmation.
pruneMissingCommands()ran atboot regardless of whether the store mounted, and persists its result. A store
that failed to mount made every schedule look orphaned, and they were deleted
permanently while the commands were very likely still intact. Now gated on a
successful mount, plus a guard that refuses to prune when the store is empty
but schedules exist.
IrStore::begin()formatted silently.LittleFS.begin(true)reformats onany mount failure — right for a blank partition, catastrophic for a damaged
one, and the boot log looked normal afterwards. It now attempts a plain mount
first and says what it is about to erase.
Usability
cancelLearn()only actedwhile still listening, so the state stayed
Capturedand the UI redrew thesame card. The buffer kept the data too, so a following "capture another part"
appended to the signal the user had just rejected.
so joining an open WiFi network, using an anonymous MQTT broker, or reopening
the setup AP were impossible without a factory reset — which also destroys
every learned command. The API now follows ordinary REST semantics (absent =
unchanged, present = write it, including
""), and each field gained anexplicit Clear it tick, since typing nothing should still mean "no change".
<input list="...">, and a datalist filters against whatever is already typedin its input — which is pre-filled with the current network. Replaced with an
explicit tap-to-pick list showing signal bars, RSSI and open networks.
Added
tools/ir-backup.sh— save, list and restore learned commands over the RESTAPI from a terminal. A filesystem update erases every command and the docs
have always said to back up first, but the only way to do it was clicking
through the browser, which is no use inside a flashing script.
Why
Every one of these turns a recoverable situation into damage, or hides a
working system behind a broken control. The three storage bugs share a theme
worth calling out: the codebase treated formatting the flash as a benign
recovery step in three separate places, and it never is.
The backup script exists because I hand-rolled the same extraction regex twice
while testing and got it wrong the second time, coming within one command of
losing a real air-conditioner capture.
How it was tested
pio run -e esp32dev -e esp32dev_oled -e esp32s3 -e esp32c3— clean, zero warnings"", typing sends the value; all four scanned networks render where the datalist showed one; tapping a row fills the SSIDmqttPassset →mqttPassSet: true; field omitted → staystrue; explicit""→falseidlewith the capture buffer emptieddocs/updated — API.md (settings contract), USAGE.md (backup CLI, firmware-vs-filesystem)CHANGELOG.mdupdated under UnreleasedNotes for the reviewer
Two paths are reasoned about rather than exercised. Reproducing the failed
filesystem OTA and the corrupt-partition format means deliberately feeding a
device a damaged image while it holds real captures. I did not do that to a
working unit. The logic is straightforward to read; the flag is set at the one
unmount and cleared at every exit.
One reported finding is deliberately not fixed — a claimed receiver lockout
after transmitting while the diagnostics monitor is active. Every path is
balanced:
sendStored,testCapture,sendRawArrayandselfTesteachrestore the receiver they disabled,
enableIRIn()resumes internally,txBusy_is cleared on all exits including the send-failure path, and the monitor branch
in
loop()always resumes. Editing the receiver state machine on anunreproduced report is precisely how the null-timer crash on
maingot in, sothis needs a bench repro first.
The
goto overflowinconsumeDecode()is unreachable by construction nowthat the bound is exact. It stays because that loop writes a fixed buffer from
interrupt-sourced data, and a future arithmetic slip should surface as a
reported error rather than as silent corruption.
🤖 Generated with Claude Code