Bound the single-event slot count the pump reports - #40
Merged
Conversation
slot_count() returned ClockProgramOverview byte 4 straight off the wire, and read_all() and find_free_slot() turn that into one Class 10 read per slot. A pump reporting 255 would spend minutes walking sub-ids that cannot hold a single event, and on a link where those reads go unanswered it is the read timeout each. The ceiling needs no new constant and no judgement about what is reasonable. The sub-id is 900 + slot and the weekly schedule's layer records begin at 1000, so slot 100 addresses layer 0: anything past 99 is a different object however the pump counts. SLOT_LIMIT was already enforced on the write path - the read path had simply been missed. An unreadable overview still returns None rather than 0, because "we do not know" is not "the pump has no slots", and the callers treat those differently. Nobody has seen a pump report a wrong count, here or on the ESPHome side. This is the shape being fixed - a wire-supplied number used as a loop bound with no ceiling - which esphome-alpha-hwr#284 filed after finding the same thing in the event-log chain, where the field is two bytes wide and can ask for 65,535 reads. Verified against the pump: it reports 5, read_all() reads 5, and find_free_slot() still answers, so the clamp is inert on real hardware and the tests carry the overlarge case.
Contributor
There was a problem hiding this comment.
Pull request overview
Bounds wire-reported single-event slot counts to the addressable protocol range, preventing excessive reads.
Changes:
- Clamp oversized slot counts to
SLOT_LIMIT. - Preserve
Nonefor unreadable overviews. - Add boundary tests and document the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Review summary |
|---|---|
tests/unit/services/test_single_event_rules.py |
Moderate finding (3 votes): the clamp test aborts on the first slot; return a valid 13-byte empty event and/or assert exactly SLOT_LIMIT reads. |
src/alpha_hwr/services/single_event.py |
Applies the slot-count ceiling. |
CHANGELOG.md |
Documents the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| reads.append(sub) | ||
| if (obj, sub) == (84, 1): | ||
| return bytes([0, 0, 10, 2, 255, 0, 0, 0, 0, 0]) | ||
| return None # every slot unreadable, so read_all bails |
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.
slot_count()returnedClockProgramOverviewbyte 4 straight off the wire, andread_all()/find_free_slot()turn that into one Class 10 read per slot. A pump reporting 255 would spend minutes walking sub-ids that cannot hold a single event — longer if those reads go unanswered, since it is the read timeout each.The ceiling is structural
No new constant and no judgement about what is reasonable. The sub-id is
900 + slotand the weekly schedule's layer records begin at 1000, so slot 100 addresses layer 0 — anything past 99 is a different object however the pump counts.SLOT_LIMITwas already enforced on the write path. The read path had simply been missed.An unreadable overview still returns
Nonerather than0, because "we do not know" is not "the pump has no slots", and the callers treat those differently.Provenance
Nobody has seen a pump report a wrong count, here or upstream. This fixes the shape — a wire-supplied number used as a loop bound with no ceiling — which
esphome-alpha-hwr#284filed after finding it in the event-log chain, where the field is two bytes wide and can ask for 65,535 reads. Ours is bounded at 255 only because the field is one byte.Verification
Against the pump: it reports 5,
read_all()reads 5,find_free_slot()still answers — so the clamp is inert on real hardware. Four tests carry the cases that matter, including one asserting no slot read ever reaches the schedule layers when the pump claims 255.751 tests, ruff, mypy and basedpyright clean.