From c47d4122fe6ac1898dd1f374d41ca67dc62f3f13 Mon Sep 17 00:00:00 2001 From: Emmanuel Levijarvi Date: Wed, 22 Jul 2026 23:06:25 -0700 Subject: [PATCH 1/4] Add confirmed reservation/TOU write helpers and canonical schedule comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/nwp500/__init__.py | 7 ++ src/nwp500/models/schedule.py | 31 ++++++ src/nwp500/models/tou.py | 35 +++++++ src/nwp500/reservations.py | 60 +++++++++++- src/nwp500/tou_schedule.py | 88 +++++++++++++++++ tests/test_canonical_schedule.py | 161 +++++++++++++++++++++++++++++++ tests/test_reservations.py | 97 +++++++++++++++++++ tests/test_tou_schedule.py | 157 ++++++++++++++++++++++++++++++ 8 files changed, 635 insertions(+), 1 deletion(-) create mode 100644 src/nwp500/tou_schedule.py create mode 100644 tests/test_canonical_schedule.py create mode 100644 tests/test_tou_schedule.py diff --git a/src/nwp500/__init__.py b/src/nwp500/__init__.py index 9901c4ca..7e98d5ec 100644 --- a/src/nwp500/__init__.py +++ b/src/nwp500/__init__.py @@ -123,6 +123,10 @@ delete_reservation, fetch_reservations, update_reservation, + update_reservations_confirmed, +) +from nwp500.tou_schedule import ( + configure_tou_schedule_confirmed, ) from nwp500.unit_system import ( get_unit_system, @@ -214,6 +218,9 @@ "add_reservation", "delete_reservation", "update_reservation", + "update_reservations_confirmed", + # TOU schedule helpers + "configure_tou_schedule_confirmed", # MQTT Client "NavienMqttClient", "MqttConnectionConfig", diff --git a/src/nwp500/models/schedule.py b/src/nwp500/models/schedule.py index 91a1a528..999ed771 100644 --- a/src/nwp500/models/schedule.py +++ b/src/nwp500/models/schedule.py @@ -87,6 +87,21 @@ def mode_name(self) -> str: except ValueError: return f"Unknown ({self.mode})" + def canonical_key(self) -> tuple[int, int, int, int, int, int]: + """Raw protocol fields as a stable, hashable tuple. + + Used to compare a desired reservation entry against a device + read-back without depending on field order or computed properties. + """ + return ( + self.enable, + self.week, + self.hour, + self.min, + self.mode, + self.param, + ) + class ReservationSchedule(NavienBaseModel): """Complete reservation schedule from the device. @@ -132,6 +147,22 @@ def enabled(self) -> bool: """ return self.reservation_use == 2 + def canonical(self) -> tuple[bool, tuple[tuple[int, ...], ...]]: + """Normalized, order-independent representation of this schedule. + + Entry order in a device read-back is not guaranteed to match the + order a program was written in, so entries are sorted by their raw + field tuple. Two schedules holding the same reservations return + equal (and equally hashable) results from this method regardless of + entry order — the intended way to compare a desired program against + a device read-back (see + :func:`nwp500.reservations.update_reservations_confirmed`). + """ + return ( + self.enabled, + tuple(sorted(entry.canonical_key() for entry in self.reservation)), + ) + class WeeklyReservationEntry(NavienBaseModel): """A single entry in a weekly temperature reservation schedule. diff --git a/src/nwp500/models/tou.py b/src/nwp500/models/tou.py index 3efe7249..124665a8 100644 --- a/src/nwp500/models/tou.py +++ b/src/nwp500/models/tou.py @@ -121,6 +121,26 @@ def decoded_price_max(self) -> float: divisor: float = 10.0**self.decimal_point return float(self.price_max) / divisor + def canonical_key( + self, + ) -> tuple[int, int, int, int, int, int, int, int, int]: + """Raw protocol fields as a stable, hashable tuple. + + Used to compare a desired TOU period against a device read-back + without depending on field order or computed properties. + """ + return ( + self.season, + self.week, + self.start_hour, + self.start_min, + self.end_hour, + self.end_min, + self.price_min, + self.price_max, + self.decimal_point, + ) + class TOUReservationSchedule(NavienBaseModel): """TOU schedule as returned by the MQTT ``tou/rd`` response topic. @@ -166,3 +186,18 @@ def enabled(self) -> bool: Protocol convention: 0=disabled, 2=enabled. """ return self.reservation_use == 2 + + def canonical(self) -> tuple[bool, tuple[tuple[int, ...], ...]]: + """Normalized, order-independent representation of this schedule. + + Mirrors :meth:`nwp500.models.schedule.ReservationSchedule.canonical`: + periods are sorted by their raw field tuple so two schedules with the + same periods compare equal regardless of the order the device + returned them in. + """ + return ( + self.enabled, + tuple( + sorted(period.canonical_key() for period in self.reservation) + ), + ) diff --git a/src/nwp500/reservations.py b/src/nwp500/reservations.py index 28bbe455..7011c007 100644 --- a/src/nwp500/reservations.py +++ b/src/nwp500/reservations.py @@ -12,7 +12,7 @@ import asyncio import logging from collections.abc import Sequence -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from .encoding import build_reservation_entry, encode_week_bitfield from .models import ReservationSchedule @@ -78,6 +78,63 @@ def on_schedule(schedule: ReservationSchedule) -> None: ) +async def update_reservations_confirmed( + mqtt: NavienMqttClient, + device: Device, + reservations: Sequence[dict[str, Any]], + *, + enabled: bool = True, + timeout: float = 10.0, +) -> ReservationSchedule | None: + """Write the full reservation list and confirm the device applied it. + + Sends ``update_reservations`` and waits for the device's ``rsv/rd`` + echo, returning the parsed :class:`ReservationSchedule` the device now + holds. Compare it against the desired program with + :meth:`ReservationSchedule.canonical`, e.g.:: + + confirmed = await update_reservations_confirmed(mqtt, device, entries) + assert confirmed is not None + assert confirmed.canonical() == desired_schedule.canonical() + + Args: + mqtt: Connected MQTT client. + device: Target device. + reservations: List of raw reservation entry dicts to write. + enabled: Whether reservations are enabled (default: True). + timeout: Seconds to wait for the confirming response. + + Returns: + The :class:`ReservationSchedule` the device echoed back after the + write, or ``None`` if no response arrived within ``timeout``. + """ + 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) + except TimeoutError: + return None + finally: + try: + await mqtt.unsubscribe_reservation_response(device, on_schedule) + except Exception: + _logger.warning( + "Failed to unsubscribe reservations response handler for " + "device %s", + device.device_info.mac_address, + exc_info=True, + ) + + async def add_reservation( mqtt: NavienMqttClient, device: Device, @@ -283,6 +340,7 @@ async def update_reservation( __all__ = [ "fetch_reservations", + "update_reservations_confirmed", "add_reservation", "delete_reservation", "update_reservation", diff --git a/src/nwp500/tou_schedule.py b/src/nwp500/tou_schedule.py new file mode 100644 index 00000000..75f93801 --- /dev/null +++ b/src/nwp500/tou_schedule.py @@ -0,0 +1,88 @@ +""" +TOU (Time-of-Use) schedule management helpers. + +Companion to :mod:`nwp500.reservations`: the device protocol requires +sending the full TOU period list for every change, and confirming that a +write landed requires waiting for the device's ``tou/rd`` echo rather than +just the MQTT publish packet id. + +All functions are ``async`` and require a connected :class:`NavienMqttClient`. +""" + +import asyncio +import logging +from collections.abc import Sequence +from typing import TYPE_CHECKING, Any + +from .models import TOUReservationSchedule + +if TYPE_CHECKING: + from .models import Device + from .mqtt import NavienMqttClient + +_logger = logging.getLogger(__name__) + + +async def configure_tou_schedule_confirmed( + mqtt: NavienMqttClient, + device: Device, + controller_serial_number: str, + periods: Sequence[dict[str, Any]], + *, + enabled: bool = True, + timeout: float = 10.0, +) -> TOUReservationSchedule | None: + """Write the TOU schedule and confirm the device applied it. + + Sends ``configure_tou_schedule`` and waits for the device's ``tou/rd`` + echo, returning the parsed :class:`TOUReservationSchedule` the device + now holds. Compare it against the desired program with + :meth:`TOUReservationSchedule.canonical`, e.g.:: + + confirmed = await configure_tou_schedule_confirmed( + mqtt, device, serial, periods + ) + assert confirmed is not None + assert confirmed.canonical() == desired_schedule.canonical() + + Args: + mqtt: Connected MQTT client. + device: Target device. + controller_serial_number: Controller serial number. + periods: List of raw TOU period dicts to write. + enabled: Whether TOU is enabled (default: True). + timeout: Seconds to wait for the confirming response. + + Returns: + The :class:`TOUReservationSchedule` the device echoed back after + the write, or ``None`` if no response arrived within ``timeout``. + """ + 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: + return await asyncio.wait_for(future, timeout=timeout) + except TimeoutError: + return None + finally: + try: + await mqtt.unsubscribe_tou_response(device, on_schedule) + except Exception: + _logger.warning( + "Failed to unsubscribe TOU response handler for device %s", + device.device_info.mac_address, + exc_info=True, + ) + + +__all__ = ["configure_tou_schedule_confirmed"] diff --git a/tests/test_canonical_schedule.py b/tests/test_canonical_schedule.py new file mode 100644 index 00000000..1c842d5f --- /dev/null +++ b/tests/test_canonical_schedule.py @@ -0,0 +1,161 @@ +"""Golden-vector tests for canonical reservation/TOU schedule representations. + +These cover the equality/hash helper requested in issue #111: a consumer +needs to compare a desired program against a device read-back without +hand-diffing raw fields, and without caring about the order entries were +sent/returned in. +""" + +from nwp500.encoding import build_tou_period, decode_reservation_hex +from nwp500.models import ( + ReservationEntry, + ReservationSchedule, + TOUPeriod, + TOUReservationSchedule, +) + + +class TestReservationEntryCanonicalKey: + def test_matches_known_decoded_vector(self): + """Golden vector from decode_reservation_hex's own docstring + example: "013e061e0478" -> enable=1, week=62, hour=6, min=30, + mode=4, param=120.""" + decoded = decode_reservation_hex("013e061e0478")[0] + entry = ReservationEntry(**decoded) + assert entry.canonical_key() == (1, 62, 6, 30, 4, 120) + + def test_key_is_hashable(self): + entry = ReservationEntry( + enable=2, week=84, hour=6, min=30, mode=3, param=120 + ) + assert hash(entry.canonical_key()) == hash((2, 84, 6, 30, 3, 120)) + + +class TestReservationScheduleCanonical: + def _entries(self) -> list[ReservationEntry]: + return [ + ReservationEntry( + enable=2, week=64, hour=6, min=0, mode=3, param=120 + ), + ReservationEntry( + enable=2, week=32, hour=18, min=30, mode=4, param=110 + ), + ] + + def test_order_independent_equality(self): + entries = self._entries() + a = ReservationSchedule(reservationUse=2, reservation=entries) + b = ReservationSchedule( + reservationUse=2, reservation=list(reversed(entries)) + ) + assert a.canonical() == b.canonical() + + def test_hash_stable_and_order_independent(self): + entries = self._entries() + a = ReservationSchedule(reservationUse=2, reservation=entries) + b = ReservationSchedule( + reservationUse=2, reservation=list(reversed(entries)) + ) + assert hash(a.canonical()) == hash(b.canonical()) + + def test_differing_programs_are_unequal(self): + a = ReservationSchedule( + reservationUse=2, + reservation=[ + ReservationEntry( + enable=2, week=64, hour=6, min=0, mode=3, param=120 + ) + ], + ) + b = ReservationSchedule( + reservationUse=2, + reservation=[ + ReservationEntry( + enable=2, week=64, hour=7, min=0, mode=3, param=120 + ) + ], + ) + assert a.canonical() != b.canonical() + + def test_enabled_flag_is_part_of_canonical_form(self): + entries = self._entries() + enabled = ReservationSchedule(reservationUse=2, reservation=entries) + disabled = ReservationSchedule(reservationUse=1, reservation=entries) + assert enabled.canonical() != disabled.canonical() + + +class TestTOUPeriodCanonicalKey: + def test_matches_built_period(self): + period_dict = build_tou_period( + season_months=[6, 7, 8], + week_days=["MO", "TU", "WE", "TH", "FR"], + start_hour=14, + start_minute=0, + end_hour=19, + end_minute=0, + price_min=10, + price_max=25, + decimal_point=2, + ) + period = TOUPeriod(**period_dict) + assert period.canonical_key() == ( + period_dict["season"], + period_dict["week"], + 14, + 0, + 19, + 0, + 10, + 25, + 2, + ) + + +class TestTOUReservationScheduleCanonical: + def _periods(self) -> list[TOUPeriod]: + return [ + TOUPeriod( + season=4095, + week=254, + startHour=0, + startMinute=0, + endHour=11, + endMinute=59, + priceMin=10, + priceMax=15, + decimalPoint=2, + ), + TOUPeriod( + season=4095, + week=254, + startHour=12, + startMinute=0, + endHour=23, + endMinute=59, + priceMin=20, + priceMax=25, + decimalPoint=2, + ), + ] + + def test_order_independent_equality(self): + periods = self._periods() + a = TOUReservationSchedule(reservationUse=2, reservation=periods) + b = TOUReservationSchedule( + reservationUse=2, reservation=list(reversed(periods)) + ) + assert a.canonical() == b.canonical() + + def test_hash_stable_and_order_independent(self): + periods = self._periods() + a = TOUReservationSchedule(reservationUse=2, reservation=periods) + b = TOUReservationSchedule( + reservationUse=2, reservation=list(reversed(periods)) + ) + assert hash(a.canonical()) == hash(b.canonical()) + + def test_differing_programs_are_unequal(self): + periods = self._periods() + a = TOUReservationSchedule(reservationUse=2, reservation=periods) + b = TOUReservationSchedule(reservationUse=2, reservation=[periods[0]]) + assert a.canonical() != b.canonical() diff --git a/tests/test_reservations.py b/tests/test_reservations.py index 45bc5b76..e49fe1a4 100644 --- a/tests/test_reservations.py +++ b/tests/test_reservations.py @@ -11,6 +11,7 @@ delete_reservation, fetch_reservations, update_reservation, + update_reservations_confirmed, ) # --------------------------------------------------------------------------- @@ -147,6 +148,102 @@ async def fake_request(device: Any) -> None: assert result is schedule +# --------------------------------------------------------------------------- +# update_reservations_confirmed +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_update_reservations_confirmed_success( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """update_reservations_confirmed returns the device's echoed schedule.""" + entries = [_entry()] + echoed = _make_schedule(entries) + captured_callback: list[Any] = [] + + async def fake_subscribe_reservation(device: Any, cb: Any) -> int: + captured_callback.append(cb) + return 1 + + mock_mqtt.subscribe_reservation_response.side_effect = ( + fake_subscribe_reservation + ) + + async def fake_update(device: Any, reservations: Any, **kwargs: Any) -> int: + for cb in captured_callback: + cb(echoed) + return 1 + + mock_mqtt.update_reservations.side_effect = fake_update + + result = await update_reservations_confirmed( + mock_mqtt, mock_device, entries + ) + + assert result is echoed + mock_mqtt.update_reservations.assert_awaited_once_with( + mock_device, entries, enabled=True + ) + mock_mqtt.unsubscribe_reservation_response.assert_called_once_with( + mock_device, ANY + ) + + +@pytest.mark.asyncio +async def test_update_reservations_confirmed_timeout( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """Returns None on timeout, still writes and unsubscribes.""" + mock_mqtt.subscribe_reservation_response = AsyncMock() + mock_mqtt.update_reservations = AsyncMock() # never fires callback + + result = await update_reservations_confirmed( + mock_mqtt, mock_device, [_entry()], timeout=0.01 + ) + + assert result is None + mock_mqtt.update_reservations.assert_awaited_once() + mock_mqtt.unsubscribe_reservation_response.assert_called_once_with( + mock_device, ANY + ) + + +@pytest.mark.asyncio +async def test_update_reservations_confirmed_matches_desired( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """The echoed schedule's canonical() form matches what was written, + even if the device returns entries in a different order.""" + entries = [_entry(hour=6), _entry(hour=9)] + desired = _make_schedule(entries) + # Device echoes the same entries back in reverse order + echoed = _make_schedule(list(reversed(entries))) + captured_callback: list[Any] = [] + + async def fake_subscribe_reservation(device: Any, cb: Any) -> int: + captured_callback.append(cb) + return 1 + + mock_mqtt.subscribe_reservation_response.side_effect = ( + fake_subscribe_reservation + ) + + async def fake_update(device: Any, reservations: Any, **kwargs: Any) -> int: + for cb in captured_callback: + cb(echoed) + return 1 + + mock_mqtt.update_reservations.side_effect = fake_update + + result = await update_reservations_confirmed( + mock_mqtt, mock_device, entries + ) + + assert result is not None + assert result.canonical() == desired.canonical() + + # --------------------------------------------------------------------------- # add_reservation # --------------------------------------------------------------------------- diff --git a/tests/test_tou_schedule.py b/tests/test_tou_schedule.py new file mode 100644 index 00000000..534cd153 --- /dev/null +++ b/tests/test_tou_schedule.py @@ -0,0 +1,157 @@ +"""Tests for the nwp500.tou_schedule public helpers.""" + +from typing import Any +from unittest.mock import ANY, AsyncMock, MagicMock + +import pytest + +from nwp500.models import TOUPeriod, TOUReservationSchedule +from nwp500.tou_schedule import configure_tou_schedule_confirmed + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture +def mock_device() -> MagicMock: + device = MagicMock() + device.device_info.device_type = "NWP500" + return device + + +@pytest.fixture +def mock_mqtt(mock_device: MagicMock) -> MagicMock: + mqtt = MagicMock() + mqtt.client_id = "test-client" + mqtt.subscribe_tou_response = AsyncMock() + mqtt.unsubscribe_tou_response = AsyncMock() + mqtt.configure_tou_schedule = AsyncMock() + return mqtt + + +def _make_schedule( + periods: list[dict[str, Any]], enabled: bool = True +) -> TOUReservationSchedule: + return TOUReservationSchedule( + reservationUse=2 if enabled else 0, + reservation=[TOUPeriod(**p) for p in periods], + ) + + +def _period( + season: int = 4095, + week: int = 254, + start_hour: int = 0, + start_minute: int = 0, + end_hour: int = 23, + end_minute: int = 59, + price_min: int = 10, + price_max: int = 25, + decimal_point: int = 2, +) -> dict[str, Any]: + return { + "season": season, + "week": week, + "startHour": start_hour, + "startMinute": start_minute, + "endHour": end_hour, + "endMinute": end_minute, + "priceMin": price_min, + "priceMax": price_max, + "decimalPoint": decimal_point, + } + + +# --------------------------------------------------------------------------- +# configure_tou_schedule_confirmed +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_configure_tou_schedule_confirmed_success( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """Returns the device's echoed schedule after a successful write.""" + periods = [_period()] + echoed = _make_schedule(periods) + captured_callback: list[Any] = [] + + async def fake_subscribe(device: Any, cb: Any) -> int: + captured_callback.append(cb) + return 1 + + mock_mqtt.subscribe_tou_response.side_effect = fake_subscribe + + async def fake_configure( + device: Any, controller_serial_number: Any, periods: Any, **kwargs: Any + ) -> int: + for cb in captured_callback: + cb(echoed) + return 1 + + mock_mqtt.configure_tou_schedule.side_effect = fake_configure + + result = await configure_tou_schedule_confirmed( + mock_mqtt, mock_device, "SERIAL123", periods + ) + + assert result is echoed + mock_mqtt.configure_tou_schedule.assert_awaited_once_with( + mock_device, "SERIAL123", periods, enabled=True + ) + mock_mqtt.unsubscribe_tou_response.assert_called_once_with(mock_device, ANY) + + +@pytest.mark.asyncio +async def test_configure_tou_schedule_confirmed_timeout( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """Returns None on timeout, still writes and unsubscribes.""" + mock_mqtt.subscribe_tou_response = AsyncMock() + mock_mqtt.configure_tou_schedule = AsyncMock() # never fires callback + + result = await configure_tou_schedule_confirmed( + mock_mqtt, mock_device, "SERIAL123", [_period()], timeout=0.01 + ) + + assert result is None + mock_mqtt.configure_tou_schedule.assert_awaited_once() + mock_mqtt.unsubscribe_tou_response.assert_called_once_with(mock_device, ANY) + + +@pytest.mark.asyncio +async def test_configure_tou_schedule_confirmed_matches_desired( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """The echoed schedule's canonical() form matches what was written, + even if the device returns periods in a different order.""" + periods = [ + _period(start_hour=0, end_hour=11), + _period(start_hour=12, end_hour=23), + ] + desired = _make_schedule(periods) + echoed = _make_schedule(list(reversed(periods))) + captured_callback: list[Any] = [] + + async def fake_subscribe(device: Any, cb: Any) -> int: + captured_callback.append(cb) + return 1 + + mock_mqtt.subscribe_tou_response.side_effect = fake_subscribe + + async def fake_configure( + device: Any, controller_serial_number: Any, periods: Any, **kwargs: Any + ) -> int: + for cb in captured_callback: + cb(echoed) + return 1 + + mock_mqtt.configure_tou_schedule.side_effect = fake_configure + + result = await configure_tou_schedule_confirmed( + mock_mqtt, mock_device, "SERIAL123", periods + ) + + assert result is not None + assert result.canonical() == desired.canonical() From 7c13dca1a1682a286c04df72d6725e09e0113d50 Mon Sep 17 00:00:00 2001 From: Emmanuel Levijarvi Date: Thu, 23 Jul 2026 08:26:40 -0700 Subject: [PATCH 2/4] Match confirmed writes against sent content, not first 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. --- src/nwp500/reservations.py | 19 +++++++++++++++++-- src/nwp500/tou_schedule.py | 19 +++++++++++++++++-- tests/test_reservations.py | 35 +++++++++++++++++++++++++++++++++++ tests/test_tou_schedule.py | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 4 deletions(-) diff --git a/src/nwp500/reservations.py b/src/nwp500/reservations.py index 7011c007..87fb1a50 100644 --- a/src/nwp500/reservations.py +++ b/src/nwp500/reservations.py @@ -14,6 +14,7 @@ from collections.abc import Sequence from typing import TYPE_CHECKING, Any +from .converters import device_bool_from_python from .encoding import build_reservation_entry, encode_week_bitfield from .models import ReservationSchedule @@ -106,14 +107,28 @@ async def update_reservations_confirmed( Returns: The :class:`ReservationSchedule` the device echoed back after the - write, or ``None`` if no response arrived within ``timeout``. + write, or ``None`` if no matching response arrived within + ``timeout``. + + Note: + The device protocol has no request/response correlation id on + ``rsv/rd``, so a response is only accepted once its + :meth:`~nwp500.models.ReservationSchedule.canonical` form matches + what was just written. This avoids resolving on a stale/unrelated + ``rsv/rd`` message (e.g. from a concurrent read or a previous + write) that happens to arrive in the same window. """ + expected = ReservationSchedule( + reservationUse=device_bool_from_python(enabled), + reservation=list(reservations), + ).canonical() + future: asyncio.Future[ReservationSchedule] = ( asyncio.get_running_loop().create_future() ) def on_schedule(schedule: ReservationSchedule) -> None: - if not future.done(): + if not future.done() and schedule.canonical() == expected: future.set_result(schedule) await mqtt.subscribe_reservation_response(device, on_schedule) diff --git a/src/nwp500/tou_schedule.py b/src/nwp500/tou_schedule.py index 75f93801..633c4578 100644 --- a/src/nwp500/tou_schedule.py +++ b/src/nwp500/tou_schedule.py @@ -14,6 +14,7 @@ from collections.abc import Sequence from typing import TYPE_CHECKING, Any +from .converters import device_bool_from_python from .models import TOUReservationSchedule if TYPE_CHECKING: @@ -55,14 +56,28 @@ async def configure_tou_schedule_confirmed( Returns: The :class:`TOUReservationSchedule` the device echoed back after - the write, or ``None`` if no response arrived within ``timeout``. + the write, or ``None`` if no matching response arrived within + ``timeout``. + + Note: + The device protocol has no request/response correlation id on + ``tou/rd``, so a response is only accepted once its + :meth:`~nwp500.models.TOUReservationSchedule.canonical` form + matches what was just written. This avoids resolving on a + stale/unrelated ``tou/rd`` message (e.g. from a concurrent read or + a previous configure) that happens to arrive in the same window. """ + expected = TOUReservationSchedule( + reservationUse=device_bool_from_python(enabled), + reservation=list(periods), + ).canonical() + future: asyncio.Future[TOUReservationSchedule] = ( asyncio.get_running_loop().create_future() ) def on_schedule(schedule: TOUReservationSchedule) -> None: - if not future.done(): + if not future.done() and schedule.canonical() == expected: future.set_result(schedule) await mqtt.subscribe_tou_response(device, on_schedule) diff --git a/tests/test_reservations.py b/tests/test_reservations.py index e49fe1a4..6cc267b4 100644 --- a/tests/test_reservations.py +++ b/tests/test_reservations.py @@ -244,6 +244,41 @@ async def fake_update(device: Any, reservations: Any, **kwargs: Any) -> int: assert result.canonical() == desired.canonical() +@pytest.mark.asyncio +async def test_update_reservations_confirmed_ignores_stale_response( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """A rsv/rd response that doesn't match this write (e.g. the echo of a + concurrent, unrelated read or a previous write) must not resolve the + future early — only a response matching the sent entries should.""" + entries = [_entry(hour=6)] + stale = _make_schedule([_entry(hour=23)]) + matching = _make_schedule(entries) + captured_callback: list[Any] = [] + + async def fake_subscribe_reservation(device: Any, cb: Any) -> int: + captured_callback.append(cb) + return 1 + + mock_mqtt.subscribe_reservation_response.side_effect = ( + fake_subscribe_reservation + ) + + async def fake_update(device: Any, reservations: Any, **kwargs: Any) -> int: + for cb in captured_callback: + cb(stale) + cb(matching) + return 1 + + mock_mqtt.update_reservations.side_effect = fake_update + + result = await update_reservations_confirmed( + mock_mqtt, mock_device, entries + ) + + assert result is matching + + # --------------------------------------------------------------------------- # add_reservation # --------------------------------------------------------------------------- diff --git a/tests/test_tou_schedule.py b/tests/test_tou_schedule.py index 534cd153..88619e8d 100644 --- a/tests/test_tou_schedule.py +++ b/tests/test_tou_schedule.py @@ -155,3 +155,38 @@ async def fake_configure( assert result is not None assert result.canonical() == desired.canonical() + + +@pytest.mark.asyncio +async def test_configure_tou_schedule_confirmed_ignores_stale_response( + mock_mqtt: MagicMock, mock_device: MagicMock +) -> None: + """A tou/rd response that doesn't match this write (e.g. the echo of a + concurrent, unrelated read or a previous configure) must not resolve + the future early — only a response matching the sent periods should.""" + periods = [_period(start_hour=0, end_hour=11)] + stale = _make_schedule([_period(start_hour=12, end_hour=23)]) + matching = _make_schedule(periods) + captured_callback: list[Any] = [] + + async def fake_subscribe(device: Any, cb: Any) -> int: + captured_callback.append(cb) + return 1 + + mock_mqtt.subscribe_tou_response.side_effect = fake_subscribe + + async def fake_configure( + device: Any, controller_serial_number: Any, periods: Any, **kwargs: Any + ) -> int: + for cb in captured_callback: + cb(stale) + cb(matching) + return 1 + + mock_mqtt.configure_tou_schedule.side_effect = fake_configure + + result = await configure_tou_schedule_confirmed( + mock_mqtt, mock_device, "SERIAL123", periods + ) + + assert result is matching From b2cd0e47ef06c8276f82f0c6a43f7f3231d165fb Mon Sep 17 00:00:00 2001 From: Emmanuel Levijarvi Date: Thu, 23 Jul 2026 08:29:46 -0700 Subject: [PATCH 3/4] Redact MAC address in confirmed-write unsubscribe failure logs 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. --- src/nwp500/reservations.py | 4 +++- src/nwp500/tou_schedule.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/nwp500/reservations.py b/src/nwp500/reservations.py index 87fb1a50..ad87c7a6 100644 --- a/src/nwp500/reservations.py +++ b/src/nwp500/reservations.py @@ -142,10 +142,12 @@ def on_schedule(schedule: ReservationSchedule) -> None: try: await mqtt.unsubscribe_reservation_response(device, on_schedule) except Exception: + from .mqtt.utils import redact_mac + _logger.warning( "Failed to unsubscribe reservations response handler for " "device %s", - device.device_info.mac_address, + redact_mac(device.device_info.mac_address), exc_info=True, ) diff --git a/src/nwp500/tou_schedule.py b/src/nwp500/tou_schedule.py index 633c4578..a5822169 100644 --- a/src/nwp500/tou_schedule.py +++ b/src/nwp500/tou_schedule.py @@ -93,9 +93,11 @@ def on_schedule(schedule: TOUReservationSchedule) -> None: try: await mqtt.unsubscribe_tou_response(device, on_schedule) except Exception: + from .mqtt.utils import redact_mac + _logger.warning( "Failed to unsubscribe TOU response handler for device %s", - device.device_info.mac_address, + redact_mac(device.device_info.mac_address), exc_info=True, ) From 6a727f54871c52e4c6b1d31808acb942527605f6 Mon Sep 17 00:00:00 2001 From: Emmanuel Levijarvi Date: Thu, 23 Jul 2026 08:41:58 -0700 Subject: [PATCH 4/4] Fix pyright type errors in confirmed-write helpers (CI failure) 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. --- src/nwp500/reservations.py | 4 ++-- src/nwp500/tou_schedule.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nwp500/reservations.py b/src/nwp500/reservations.py index ad87c7a6..fc4602ed 100644 --- a/src/nwp500/reservations.py +++ b/src/nwp500/reservations.py @@ -16,7 +16,7 @@ from .converters import device_bool_from_python from .encoding import build_reservation_entry, encode_week_bitfield -from .models import ReservationSchedule +from .models import ReservationEntry, ReservationSchedule if TYPE_CHECKING: from .models import Device @@ -120,7 +120,7 @@ async def update_reservations_confirmed( """ expected = ReservationSchedule( reservationUse=device_bool_from_python(enabled), - reservation=list(reservations), + reservation=[ReservationEntry(**entry) for entry in reservations], ).canonical() future: asyncio.Future[ReservationSchedule] = ( diff --git a/src/nwp500/tou_schedule.py b/src/nwp500/tou_schedule.py index a5822169..3941b968 100644 --- a/src/nwp500/tou_schedule.py +++ b/src/nwp500/tou_schedule.py @@ -15,7 +15,7 @@ from typing import TYPE_CHECKING, Any from .converters import device_bool_from_python -from .models import TOUReservationSchedule +from .models import TOUPeriod, TOUReservationSchedule if TYPE_CHECKING: from .models import Device @@ -69,7 +69,7 @@ async def configure_tou_schedule_confirmed( """ expected = TOUReservationSchedule( reservationUse=device_bool_from_python(enabled), - reservation=list(periods), + reservation=[TOUPeriod(**period) for period in periods], ).canonical() future: asyncio.Future[TOUReservationSchedule] = (