Cut the updater session's comments back to what they have to say #71

Merged
Claude merged 9 commits from docs/updater-session-comments into feat/firmware-updates 2026-09-20 06:52:09 +00:00
Collaborator

A comment pass over updater/session.py, updater/link.py, updater/bus.py and their two test files. No behaviour change.

Narration, restatement and the essays around the spec citations are gone; every spec section number is kept, as a bare citation. Where a comment only existed because the code was unclear, the code moved instead: the build-of-zero rule that was written out three times in two files is now session.announced_build, which link.py imports.

Tests and lint are unchanged in what they cover. Lint keeps D selected under the pep257 convention, so every module, class and public function still needs a docstring — the shortened ones are cut, not deleted.

A comment pass over `updater/session.py`, `updater/link.py`, `updater/bus.py` and their two test files. No behaviour change. Narration, restatement and the essays around the spec citations are gone; every spec section number is kept, as a bare citation. Where a comment only existed because the code was unclear, the code moved instead: the build-of-zero rule that was written out three times in two files is now `session.announced_build`, which `link.py` imports. Tests and lint are unchanged in what they cover. Lint keeps `D` selected under the `pep257` convention, so every module, class and public function still needs a docstring — the shortened ones are cut, not deleted.
Cut the link and bus comments back, and name the build-of-zero rule
Some checks failed
Lint, type check and test / hassfest (pull_request) Successful in 24s
Lint, type check and test / release (pull_request) Has been cancelled
Lint, type check and test / quality (pull_request) Has been cancelled
918cb58e37
Cut the updater test docstrings back to the reason each test exists
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 8s
Lint, type check and test / quality (pull_request) Successful in 1m40s
Lint, type check and test / release (pull_request) Has been skipped
0fa0a969f4
Claude left a comment

announced_build is exactly equivalent at all three sites (x or None then or build collapses to x or build), no other behaviour change in the five files, nothing touched outside them, tabs and British English clean, ruff/mypy/pytest green.

Two small things, both inline.

`announced_build` is exactly equivalent at all three sites (`x or None` then `or build` collapses to `x or build`), no other behaviour change in the five files, nothing touched outside them, tabs and British English clean, ruff/mypy/pytest green. Two small things, both inline.
@ -5,3 +3,1 @@
thread throughout, because a request arrives on one and the session it starts
runs on the loop. A test that deadlocked the two would hang rather than fail,
so every one of them is waited on with a deadline.
No van and no CAN interface; the bus is the session tests' own fake. The API is
Author
Collaborator

The spec section 11 citation that was on this line is gone — the only one dropped in the branch, and the PR body says every one is kept. It still survives on FakeNode in tests/test_updater_session.py, so either put it back here or fix the claim in the description.

The `spec section 11` citation that was on this line is gone — the only one dropped in the branch, and the PR body says every one is kept. It still survives on `FakeNode` in `tests/test_updater_session.py`, so either put it back here or fix the claim in the description.
@ -921,7 +863,6 @@ async def test_credentials_a_node_has_no_room_for_reach_no_node(
"""Section 8.9 gives 32 bytes to the name and 64 to the key, and they are separate.
A node answers result 4, which says only that some node refused something.
Author
Collaborator

This paragraph is now a fact with no conclusion: it says result 4 is unspecific and stops, having lost the clause that tied it to assert named in result.detail. Either drop the paragraph or end it with why the check is local ("so this says which half was wrong").

This paragraph is now a fact with no conclusion: it says result 4 is unspecific and stops, having lost the clause that tied it to `assert named in result.detail`. Either drop the paragraph or end it with why the check is local ("so this says which half was wrong").
Take the review: put back a citation and finish a sentence
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 10s
Lint, type check and test / quality (pull_request) Successful in 1m41s
Lint, type check and test / release (pull_request) Has been skipped
e3e2df3cd2
Claude left a comment

Both earlier findings are fixed. One thing left, and it is prose accuracy rather than code.

Both earlier findings are fixed. One thing left, and it is prose accuracy rather than code.
@ -1009,3 +948,1 @@
Both ends of the range reach it by accident: a deadline under a minute
rounds to nothing, and the codec keeps the low byte of one past 255. Either
would leave a node's radio up until something else took it down.
Both ends reach it by accident: under a minute rounds to nothing, and the
Author
Collaborator

"Both ends reach it" is not true of the past-a-byte case. The parameter is 9 hours = 540 minutes, and the codec keeps the low byte: 0x1C, 28 minutes, not 0. Only exact multiples of 256 reach 0. The sentence this replaced didn't claim that — it ended "Either would leave a node's radio up until something else took it down", which generalised the harm. Say the truncation gives an arbitrary timeout rather than the one meant, or drop the second half.

"Both ends reach it" is not true of the `past-a-byte` case. The parameter is 9 hours = 540 minutes, and the codec keeps the low byte: 0x1C, 28 minutes, not 0. Only exact multiples of 256 reach 0. The sentence this replaced didn't claim that — it ended "Either would leave a node's radio up until something else took it down", which generalised the harm. Say the truncation gives an arbitrary timeout rather than the one meant, or drop the second half.
@ -65,2 +60,2 @@
(spec section 8.1), and never past the byte, whose low half is all the
codec keeps. Both are how a deadline meant to be short becomes no deadline.
Never 0, which spec section 8.1 reads as the idle timeout never firing, and
never past the byte the codec keeps. Either leaves a node's radio up.
Author
Collaborator

Same overclaim, carried over from before this branch rather than introduced by it: "Either leaves a node's radio up" only holds for the wrap-to-zero case; past 255 the truncation usually gives a shorter timeout. Worth fixing while the sentence is being rewritten anyway.

Same overclaim, carried over from before this branch rather than introduced by it: "Either leaves a node's radio up" only holds for the wrap-to-zero case; past 255 the truncation usually gives a shorter timeout. Worth fixing while the sentence is being rewritten anyway.
Take the review: a clamp is not a claim that both ends reach zero
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 8s
Lint, type check and test / quality (pull_request) Successful in 1m46s
Lint, type check and test / release (pull_request) Has been skipped
8042ac2f2b
Claude left a comment

Both radio_minutes docstrings now read true against the clamp and against codec.encode, which masks a one-byte field with raw & 0xFF. One thing left.

Both `radio_minutes` docstrings now read true against the clamp and against `codec.encode`, which masks a one-byte field with `raw & 0xFF`. One thing left.
@ -404,3 +375,2 @@
"sequence": sequence,
# The length is what stops the last frame where the
# content stops rather than padding it out.
# Declared short, so the padding below is not content.
Author
Collaborator

Declared short, so the padding below is not content. reads as though the padding goes on the wire and is excluded by the declared length. It doesn't: length is the counted field, so Message.length_for shortens the frame and the zero bytes are never sent. That distinction is the point of the surrounding tests, since padding that did go out would land under the CRC. Say that the length stops the frame where the chunk stops.

`Declared short, so the padding below is not content.` reads as though the padding goes on the wire and is excluded by the declared length. It doesn't: `length` is the counted field, so `Message.length_for` shortens the frame and the zero bytes are never sent. That distinction is the point of the surrounding tests, since padding that did go out would land under the CRC. Say that the length stops the frame where the chunk stops.
Take the review: the padding pads an integer, not a frame
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 26s
Lint, type check and test / quality (pull_request) Successful in 1m51s
Lint, type check and test / release (pull_request) Has been skipped
570a8e9503
Claude left a comment

Round three's point is settled and nothing else is left. The new wording matches Message.length_for and codec.encode: passing length shortens the frame to 2 + len(chunk), and the ljust zeros only exist to left-align the six-byte integer, so they are truncated before the frame goes out.

Round three's point is settled and nothing else is left. The new wording matches `Message.length_for` and `codec.encode`: passing `length` shortens the frame to `2 + len(chunk)`, and the `ljust` zeros only exist to left-align the six-byte integer, so they are truncated before the frame goes out.
Merge feat/firmware-updates into docs/updater-session-comments
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 40s
Lint, type check and test / quality (pull_request) Successful in 1m52s
Lint, type check and test / release (pull_request) Has been skipped
d67c342e14
Merge feat/firmware-updates into docs/updater-session-comments
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 8s
Lint, type check and test / quality (pull_request) Successful in 1m39s
Lint, type check and test / release (pull_request) Has been skipped
843a04860e
Merge feat/firmware-updates into docs/updater-session-comments
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 8s
Lint, type check and test / quality (pull_request) Successful in 1m40s
Lint, type check and test / release (pull_request) Has been skipped
2b4b3cec92
Claude merged commit 1ff253b552 into feat/firmware-updates 2026-09-20 06:52:09 +00:00
Claude deleted branch docs/updater-session-comments 2026-09-20 06:52:09 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rob/CampervanHomeAssistant!71
No description provided.