Cut the firmware-update comments back to what the code cannot say #58
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/cull-firmware-update-comments"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A comments-only pass over the production code this branch added:
lib/can-protocol/src,lib/node-runtime/src,srcandplatformio.ini. Comment lines the branch adds go from 198 to 69.What survives is upstream quirks (
WiFi.persistent(false), theoctets()byte order,esp_ota_end()beforeactivate(), what each CAN backend's transmit signal means), ordering a future editor would break, and cross-file contracts likeinstalled()survivingcancel()— each one line. Everything narrating the next line, restating a signature, or repeating whatdocs/can-protocol.mdalready holds is gone, leaving the bareSpec section Npointers.One code change rather than a comment:
BootGuard::committing_is nowcommitAttempted_, which says what the flag is for, so its two-line explanation could go.Two comments inherited from earlier commits on this branch gave a reason that does not hold.
Node::loop's boot-guard call is not last so that this loop's own frames count — both backends latchtransmitCompleted_inpoll(), which runs at the top — so the claim is dropped. AndWifiManager::loop's earlyrebootIfInstalled()is not about draining frames, sinceonFramealready ignores control frames oncerebootDue(); it is there to beat the radio-lost early return below, which is what the comment now says. Neither call moved.No behaviour change.
pio test -e nativepasses all 412 cases, andnode_lighting,node_bathroomandhost_simall build.test/andtools/are untouched — a separate pass covers those.Cut these too — each says what
docs/can-protocol.mdsection 11 already says, and three of them dropped theSpec section 11pointer while keeping the prose, which is backwards:lib/node-runtime/src/wifi_manager.cpp:109— "No status frame on the way out; the next announce's build id says it took." Spec 11: "arriving after it is ignored, with no status frame" and "a new build identifier inSYS_ANNOUNCEafter it is what says an update took". Replace with// Spec section 11.lib/node-runtime/src/wifi_manager.cpp:132— "Only an update request clears it, so action 1 cannot take state 6 off the bus." Spec 11: "State 6 stays reported until the session ends or another update is asked for." Replace with// Spec section 11.lib/node-runtime/src/wifi_manager.cpp:103— "A transfer already running reaches completion or failure first." Word for word from spec 11. Leave the pointer alone.lib/node-runtime/src/session_credentials.h:55— "Held in RAM and never written to NVS." Spec 11: "held in RAM and never persisted". Cut to// Spec section 11.lib/node-runtime/src/firmware_updater.h:110-112— the five minute limit and the stall timeout are both in spec 11. One// Spec section 11.over the two constants.Cut these too — recoverable from the code beside them:
lib/node-runtime/src/wifi_manager.h:55— "Public because a join fallback calls it with a timeout of its own." The fallback call is inloop()in the same unit.lib/node-runtime/src/firmware_updater.cpp:176— "No range requests, so a retry restarts the whole fetch and the slot with it." The four lines under it arereceived_ = 0andsink_.abort().lib/node-runtime/src/boot_control.h:11— "False on a committed image, and on a bootloader with no rollback support." First half follows from the name; second half is the same factboot_control_arduino.cpp:11states, where it belongs.lib/node-runtime/src/firmware_manifest.cpp:39— "Leaves the cursor on the value, which for memberUnknown is the caller's to skip." Both call sites callskipValue()onmemberUnknownin plain sight.lib/node-runtime/src/firmware_manifest.cpp:28— "Just enough JSON to walk objects and pick named members out of them." Describes what the class visibly is.lib/node-runtime/src/can_bus_mcp2515.cpp:148— "Latched, so this costs no SPI read once the bus has been proved."can_bus.halready says latched.Fix these:
lib/node-runtime/src/firmware_updater.h:142— "Both say nothing untilinstalled()is true" is wrong forslot():openSlot()setsslot_while the stage is stillImage. Either narrow it tobytesWritten()or drop it. (Pre-existing on the branch, but in scope for this pass.)lib/node-runtime/src/update_transport_arduino.cpp:155— "Both block the whole runtime" now sits over two constants, so "both" reads as the constants. It means connecting and reading the headers.platformio.ini:23— "which WiFi and TLS fill" claims a TLS client that is not in the build: the transport isHTTPClientover a plainWiFiClient, and spec 11.1 specifies HTTPGET. Also the dropped "Both boards are 4 MB" was what explained[lolin32]repeating the same setting with no comment of its own.Otherwise:
committing_→commitAttempted_is a pure rename — same initialisation inbegin(), same single guarded commit attempt inloop(), no other reference in the tree. No deletion in the diff loses a fact that is not either in the code or in the spec.#if defined(ARDUINO)headers all matchlevel_endpoint.cpp. Tabs, British spelling andlowerCamelCaseconstants are clean, andclang-format --dry-run -Werroris quiet on every changed file.Second round. Nothing to restore and nothing inaccurate; every shortened pointer checks out against the spec (8.9 carries the CRC parameters and the 98-byte ceiling, 11 the 30 s window, five-minute limit, credentials-never-persisted and never-cancel-after-install, 11.1 the Content-Length rule, 9.3 the three missed heartbeats).
updateStallTimeoutMs = 10000is the one surviving constant whose value is nowhere in the spec, which is fine but worth knowing.A further round of cuts below, ordered by how confident I am. Two notes at the end that are not this PR's to fix.
Conventions all check out:
#if defined(ARDUINO)headers matchlevel_endpoint.cpp, tabs throughout (the only space-indented added lines are clang-format'spublic:), no US spellings, no new constants,platformio.iniin its own style, andCONTRIBUTING.md's Tests section does carry what the native comment now defers to.Cut these too
lib/node-runtime/src/wifi_manager.cpp— five bare// Spec section 11.in one file, three of them insidebeginJoin(theupdateRunning()guard, theforUpdateclear, the trailing one on the radio check). Repeated that often a pointer stops pointing. Put one onbeginJoinand drop the inner three.lib/node-runtime/src/bulk_receiver.h:13— spec 8.9 says this verbatim ("a 32-byte SSID and a 64-byte key, each with its terminator"), and there is already a// Spec section 8.9.three lines up. Delete.lib/node-runtime/src/session_credentials.cpp:8— the three-line body says exactly this. Delete.lib/node-runtime/src/firmware_sink.h:16— restates the signature plusnoFirmwareSlot, which is named nine lines up. Delete.lib/node-runtime/src/update_transport.h:7— first sentence is the class name; the second is already implied bybegin/read/endtaking no handle. Delete.lib/node-runtime/src/sha256.h:18— drop "Writes sha256Length bytes"; keep "Hashing more needs a reset() first", which is the trap.lib/node-runtime/src/firmware_manifest.h:14— weaker call: nothing rides on 2048 being 2048, so the estimate that justifies it earns less than the others.lib/node-runtime/src/firmware_sink.h:9— weaker call: "The inactive application slot" is spec 11 step 5, so the bare pointer alone would do.Other
committing_→commitAttempted_rename is behaviour-identical and is the right way to delete that comment, but it is a code change in a comment-cull PR. Flagging only so it is deliberate.wifi_manager.cpp:77suppresses the 5 s refresh for state 5, and spec section 11 says states refresh "every 5 s like any other state". Pre-existing onfeat/firmware-updates, not introduced here, but the surviving comment now asserts it flatly as design.Close to the floor. Three things left.
Cut these too:
lib/node-runtime/src/wifi_manager.cpp:120—// Spec section 11.onbeginJoin. The class already says "Spec sections 8.1 and 11" (wifi_manager.h:42), and section 11 has no subsections to narrow to, so it only repeats. Worse, the one non-obvious rule in the function —if (updateRunning()) return;, i.e. a second action 3 mid-update only pushes the deadline out — is not in section 11 at all, so the pointer sends a reader somewhere that does not answer the question.lib/node-runtime/src/wifi_manager.cpp:108—// Spec section 11.onrebootIfInstalled, same duplication. Weaker call than the one above; if you keep two pointers in this file, keep line 102 (the!updateRunning()clause is the spec's "never cuts a transfer short") and line 207 (nameApimplements the SSID rules verbatim).lib/node-runtime/src/wifi_manager.cpp:98— "Again here, so an install is acted on in the loop it happened in."test_an_image_installed_inside_the_updaters_loop_reboots_in_that_loopcarries the same fact and fails if the call goes.Restore:
lib/node-runtime/src/sha256.h:18—finish(uint8_t* out)leaves a caller no way to know it writes 32 bytes;sha256Lengththree lines up is the only hint. Make it// Writes sha256Length bytes; hashing more needs a reset() first.Other:
SYS_WIFI_CONTROLaction 3 arriving during a running update does. The code extends the timeout and ignores the rest. Separate from this PR, but it is the second spec gap this branch has turned up.Everything else in the post-image holds: each survivor is a bare spec pointer, an upstream quirk (
CONFIG_BOOTLOADER_APP_ROLLBACK_ENABLE,IPAddress,WiFi.persistent, ArduinoUpdate::end(), the MCP2515 CANINTF bits), an ordering requirement, or a cross-file contract on an abstract interface. Deleted facts are all recoverable — the CRC variant and the 98-byte ceiling are both spelled out in spec 8.9, the Content-Length rule in 11.1, the 30 s window in 11. Citations check out: 9.3 does carry the three-missed-heartbeats rule. No behaviour change;committing_→commitAttempted_is private and 412/412 pass.@ -18,3 +16,3 @@void update(const uint8_t* data, uint32_t length);// Writes sha256Length bytes. Hashing more needs a reset() first.// Hashing more needs a reset() first.Restore the output size:
// Writes sha256Length bytes; hashing more needs a reset() first.A rawuint8_t* outgives a new caller nothing to size the buffer from.@ -101,3 +95,4 @@// Here, not next loop: a control frame between the two clears updateStarted_.if (updater_->failed()) updateFailed_ = true;// Again here, so an install is acted on in the loop it happened in.Cut.
test_an_image_installed_inside_the_updaters_loop_reboots_in_that_loopsays the same thing and fails if the call goes.@ -115,3 +108,1 @@// Step 7 of the session, and not the session's to cancel. No status frame on the way// out; the build identifier in the next announce is what says the update took. Spec// section 11.// Spec section 11.Cut — duplicates the class-level pointer in
wifi_manager.h:42. Keep line 102 and line 207 instead, where the spec text dictates the code.@ -126,10 +117,10 @@ bool WifiManager::rebootIfInstalled() {return true;}// Spec section 11.Cut. The class comment already points at section 11, and the function's one non-obvious rule (
if (updateRunning()) return;— a second action 3 mid-update only extends the deadline) is not in section 11, so the pointer does not answer the question it invites.The comment set is at the floor — nothing left to cut. Three inaccuracies in what survives, two of them inherited from the base branch rather than reworded here.
Fix these
lib/node-runtime/src/node.cpp:88—// Last, so this loop's own frames count towards committing the image.is false on hardware. Both backends latchtransmitCompleted_inpoll()alone (can_bus_mcp2515.cpp:150,can_bus_twai.cpp:127), andNode::loopcallsbus_.poll()once at line 68, before the heartbeat and the endpoint reports — so the guard always reads a latch from a previous loop and placing the call last buys nothing. Only the test fake latches insend()(test/test_boot_commit/test_boot_commit.cpp:34), which is why moving the guard call up to just afterbus_.poll()failstest_a_node_commits_once_its_own_traffic_reaches_the_busand nothing else. Behaviour is fine either way (one loop's delay inside a 30 s window); the stated reason is wrong. The same claim sits in that test's header comment, lines 7-8 — out of scope here, worth fixing alongside.lib/node-runtime/src/wifi_manager.cpp:62—// Ahead of anything an inbound frame changed: Node::loop drains frames first.misattributes the call.onFramealready drops every control frame whilerebootDue()(line 31), so an inbound frame cannot change anything this check needs to get ahead of. What it actually protects is the early returns below it: delete it and the only failure istest_a_radio_lost_after_an_install_still_reboots(test/test_wifi_manager/test_wifi_manager.cpp:1126), where a dropped radio disables and returns at line 69 before the second check at line 98. Say that instead.platformio.ini:24— inline.Other
The description says comment lines go from 198 to 88; it is 70 now.
@ -23,2 +23,2 @@; Both boards are 4 MB. 1.875 MB an app slot rather than default.csv's 1.25 MB,; which gets tight once WiFi and an HTTPS client go in.; Both boards are 4 MB. 1.875 MB an app slot against default.csv's 1.25 MB,; which WiFi and the update client fill."which WiFi and the update client fill" is not true: the built images are 841 KB (node_lighting) and 812 KB (node_bathroom) against default.csv's 1.25 MB app slot. The base branch's "gets tight" was accurate — keep that sense, or drop the clause.
Clean. Nothing to act on.
Both reworded notes check out: moving
rebootIfInstalled()below theobserved == offpath would leavestate_off, and every later loop then returns at line 65 without rebooting.bus_.poll(now)is at the top ofNode::loop, so the old "last, so this loop's own frames count" was indeed false and the deletion is right.Base merge is consistent — commit
07e3448only touchedtest/test_boot_commit, and its "the MCP2515 and TWAI backends set" pairs withcan_bus.h's "SocketCAN never sets it" rather than repeating or contradicting it.One optional nit:
[env:native]now points at CONTRIBUTING.md Tests for "the rest", but that section covers the sanitisers and-v, not-g. The reason-gis there (file and line in a report instead of an address) is now recorded nowhere. Either fold one clause back into the comment or add it to CONTRIBUTING.md.@ -62,2 +60,2 @@; UBSan report; by default it prints and the test still passes. -g is what puts a; file and line in the report rather than an address.; -fno-sanitize-recover is what fails a test on a UBSan report; by default it; prints and the test still passes. CONTRIBUTING.md, Tests, has the rest."CONTRIBUTING.md, Tests, has the rest" — that section covers
-fsanitize,-vand the leak suppression, but not-g. The reason for-g(file and line in a sanitiser report rather than an address) is now recorded nowhere. Minor: either keep one clause here or add it there.