Add confirmed reservation/TOU write + canonical schedule comparison#118
Merged
Conversation
…mparison update_reservations and configure_tou_schedule were fire-and-forget: they returned only the MQTT publish packet id, with no way to confirm the device actually applied the write. Consumers programming a native reservation schedule from an external source (e.g. an ML demand forecast) need a program->verify loop. - Add update_reservations_confirmed() (nwp500.reservations) and configure_tou_schedule_confirmed() (new nwp500.tou_schedule module): both send the write and await the matching rsv/rd or tou/rd echo, returning the parsed schedule the device now holds (mirrors the existing fetch_reservations subscribe/future/timeout pattern). - Add ReservationEntry.canonical_key() / ReservationSchedule.canonical() and the TOUPeriod/TOUReservationSchedule equivalents: a stable, order-independent, hashable representation of raw protocol fields so a desired program can be compared against a device read-back with `a.canonical() == b.canonical()` instead of hand-diffing. The stale "param (temperature offset by 20°F)" docstring in decode_reservation_hex (encoding.py) is fixed separately in #113. Fixes #111
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds “write then verify” helper APIs for reservation and TOU schedule updates over MQTT, plus canonical (order-independent) schedule representations to make desired-vs-device comparisons stable and hashable.
Changes:
- Add
update_reservations_confirmed()andconfigure_tou_schedule_confirmed()helpers that await the device’srsv/rd/tou/rdecho and return the parsed schedule (orNoneon timeout). - Add canonical tuple representations for reservation entries/schedules and TOU periods/schedules to enable stable equality/hash comparisons independent of device return ordering.
- Add/extend tests to cover confirmed-write success/timeout paths and canonical equality/hash behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/nwp500/reservations.py |
Adds update_reservations_confirmed() using the subscribe/future/timeout pattern. |
src/nwp500/tou_schedule.py |
New module with configure_tou_schedule_confirmed() implementing confirmed TOU writes. |
src/nwp500/models/schedule.py |
Adds ReservationEntry.canonical_key() and ReservationSchedule.canonical() for stable comparisons. |
src/nwp500/models/tou.py |
Adds TOUPeriod.canonical_key() and TOUReservationSchedule.canonical() for stable comparisons. |
src/nwp500/__init__.py |
Exports the two new public helpers from the top-level package. |
tests/test_reservations.py |
Adds tests for update_reservations_confirmed() (success/timeout/canonical match). |
tests/test_tou_schedule.py |
New tests for configure_tou_schedule_confirmed() (success/timeout/canonical match). |
tests/test_canonical_schedule.py |
New golden-vector tests for canonical key/canonical schedule behavior (reservation + TOU). |
Comment on lines
+111
to
+123
| future: asyncio.Future[ReservationSchedule] = ( | ||
| asyncio.get_running_loop().create_future() | ||
| ) | ||
|
|
||
| def on_schedule(schedule: ReservationSchedule) -> None: | ||
| if not future.done(): | ||
| future.set_result(schedule) | ||
|
|
||
| await mqtt.subscribe_reservation_response(device, on_schedule) | ||
| try: | ||
| await mqtt.update_reservations(device, reservations, enabled=enabled) | ||
| try: | ||
| return await asyncio.wait_for(future, timeout=timeout) |
Owner
Author
There was a problem hiding this comment.
Good catch — fixed in 7c13dca. The device protocol has no request/response correlation id on rsv/rd, so instead of resolving on the first message received, the callback now only accepts a response whose canonical() form matches what was just written, ignoring stale/unrelated responses until a match arrives or the timeout expires. Added test_update_reservations_confirmed_ignores_stale_response covering this.
Comment on lines
+60
to
+73
| future: asyncio.Future[TOUReservationSchedule] = ( | ||
| asyncio.get_running_loop().create_future() | ||
| ) | ||
|
|
||
| def on_schedule(schedule: TOUReservationSchedule) -> None: | ||
| if not future.done(): | ||
| future.set_result(schedule) | ||
|
|
||
| await mqtt.subscribe_tou_response(device, on_schedule) | ||
| try: | ||
| await mqtt.configure_tou_schedule( | ||
| device, controller_serial_number, periods, enabled=enabled | ||
| ) | ||
| try: |
Owner
Author
There was a problem hiding this comment.
Same fix applied here in 7c13dca — the TOU echo is now matched against the canonical() form of what was configured, so a stale/unrelated tou/rd response is ignored instead of resolving the future early. Added test_configure_tou_schedule_confirmed_ignores_stale_response.
Review feedback on #118 (Copilot): update_reservations_confirmed and configure_tou_schedule_confirmed resolved on the FIRST rsv/rd or tou/rd message received after subscribing, including a stale/unrelated response from a concurrent read or a previous write. The device protocol has no request/response correlation id on these topics, so instead only accept a response whose canonical() form matches what was just written - ignore anything else and keep waiting until a match arrives or timeout expires.
CodeQL flagged clear-text logging of sensitive data (py/clear-text- logging-sensitive-data, high severity) in configure_tou_schedule_confirmed's unsubscribe failure handler. update_reservations_confirmed had the same pattern. Both now use the existing nwp500.mqtt.utils.redact_mac() helper (already used for this exact purpose elsewhere, e.g. device_info_cache.py, mqtt/client.py) instead of logging the raw MAC address.
pyright flagged passing raw dicts directly into ReservationSchedule / TOUReservationSchedule's `reservation` field: list[dict] isn't assignable to list[ReservationEntry] / list[TOUPeriod] under pyright's invariant list typing, even though pydantic validates and coerces the dicts fine at runtime. Construct the entry/period models explicitly instead, satisfying the type checker that CI's tox run enforces.
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.
Summary
update_reservationsandconfigure_tou_scheduleare fire-and-forget — they return only the MQTT publish packet id, with no way to confirm the device actually applied a write. This adds a program → verify loop:update_reservations_confirmed()(nwp500.reservations) andconfigure_tou_schedule_confirmed()(newnwp500.tou_schedulemodule): both send the write and await the matchingrsv/rd/tou/rdecho, returning the parsed schedule the device now holds instead of just a packet id. Mirrors the existingfetch_reservationssubscribe/future/timeout pattern, so behavior (timeout, unsubscribe-on-exit, ignoring late/duplicate responses) is consistent with the read path.ReservationEntry.canonical_key()/ReservationSchedule.canonical(), and theTOUPeriod/TOUReservationScheduleequivalents. Each returns a stable, order-independent, hashable tuple of raw protocol fields, so a consumer can compare a desired program against a device read-back withdesired.canonical() == confirmed.canonical()(or hash it) without hand-diffing or caring what order the device returned entries in.Both new public helpers are exported from the top-level
nwp500package (update_reservations_confirmed,configure_tou_schedule_confirmed).Not included here
The stale
decode_reservation_hexdocstring ("param (temperature offset by 20°F)", should be half-degrees Celsius) mentioned in this issue is fixed separately in #113 to keep that doc fix isolated and easy to review.Fixes #111
Test plan
tests/test_reservations.py: new tests forupdate_reservations_confirmed— success, timeout, and order-independentcanonical()match against the device echo (mirrors existingfetch_reservationstests).tests/test_tou_schedule.py(new): equivalent coverage forconfigure_tou_schedule_confirmed.tests/test_canonical_schedule.py(new): golden-vector tests forcanonical_key()/canonical()— including a vector decoded fromdecode_reservation_hex's own docstring example, order-independence, hash stability, and that differing programs compare unequal.620 passed, 4 skipped.ruff check/ruff format --checkclean on all touched/new files.