fix: device protocol correctness#94
Merged
Merged
Conversation
- Send flat raw protocol payloads for weekly reservation and recirculation schedules; the model dump double-nested the request and leaked computed display fields (incl. unit-converted temperature) into device commands. Add NavienBaseModel.to_protocol_dict() - Preserve command queue order on failed flush (deque with re-insert at front instead of tail) - Discard queued commands older than max_queued_command_age (default 300s) instead of replaying stale control commands after long outages - Use truncated remainder (firmware semantics) instead of Python's floored modulo in the ASYMMETRIC Fahrenheit formula; fixes off-by-one conversions for sub-zero temperatures - Correct freeze protection default limits to raw half-Celsius 12/20 (43F/50F documented fixed range) instead of 43/65 - Emit error_detected when the error code changes between two non-zero codes - Round TOU prices half-up via Decimal (round() banker's rounding under-encoded exact halves); encode bool prices instead of passing them through as pre-encoded ints - Fix inverted enable-flag doc, wrong week-bitfield example (158->84), and int/float doctest values
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens device protocol correctness across MQTT command payloads, queued command handling, and multiple encoding/decoding edge cases, with a new regression test suite to lock in the intended on-wire behavior.
Changes:
- Fix schedule command payloads to send flat lists of raw protocol entries (via
NavienBaseModel.to_protocol_dict()), avoiding double-nesting and computed/display-field leakage. - Rework MQTT command queue flushing to preserve ordering on failure and to discard stale queued commands based on
MqttConnectionConfig.max_queued_command_age. - Correct protocol encoding/decoding edge cases (negative ASYMMETRIC Fahrenheit rounding, freeze protection raw defaults, error-change event emission, TOU price encoding rounding/bool handling) and document them.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_protocol_correctness.py | Adds targeted regression tests for protocol payload shapes, queue ordering/expiry, temperature rounding, defaults, events, and TOU price encoding. |
| src/nwp500/_base.py | Introduces to_protocol_dict() to dump only declared fields (by alias) for device-bound payloads. |
| src/nwp500/mqtt/control.py | Updates schedule-related commands to send flat raw protocol entries rather than dumping full models. |
| src/nwp500/mqtt/command_queue.py | Switches queue to deque, preserves order on failed flushes, and adds send-time expiry logic. |
| src/nwp500/mqtt/utils.py | Adds max_queued_command_age config field (used by the queue expiry behavior). |
| src/nwp500/mqtt/state_tracker.py | Emits error_detected when the error code changes between non-zero values. |
| src/nwp500/temperature.py | Fixes ASYMMETRIC rounding for negative raw values and updates doctest expectations. |
| src/nwp500/models/status.py | Corrects freeze protection default raw values to match documented limits. |
| src/nwp500/encoding.py | Implements HALF_UP TOU price rounding and prevents bool from silently passing as pre-encoded int in TOU builders; fixes protocol docs/examples. |
| CHANGELOG.rst | Documents the protocol correctness fixes under Unreleased. |
This was referenced Jul 5, 2026
added 2 commits
July 5, 2026 09:38
# Conflicts: # CHANGELOG.rst
- RawCelsius.to_fahrenheit() now returns float as annotated (round() without ndigits returns int); doctest updated to 140.0 - encode_price() builds the Decimal from the original value instead of round-tripping through float, which lost precision for Decimal inputs and introduced binary floating-point artifacts; signature widened to accept Decimal explicitly - MqttConnectionConfig.__post_init__ validates max_queued_commands >= 1, max_queued_command_age >= 0 (or None), and operation_timeout > 0 to fail fast instead of producing hard-to-debug runtime errors - Tests for config validation, Decimal-input precision, and the float return type
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.
Fifth batch from the comprehensive repo evaluation: fixes for what the library actually sends to (and decodes from) the physical appliance.
Bugs fixed
Malformed schedule commands (
mqtt/control.py,_base.py):update_weekly_reservation()andconfigure_recirculation_schedule()dumped their schedule models as-is, which:request.reservation.reservation/request.schedule.schedule) — inconsistent with the documented flat shapeupdate_reservations()sends, andcomputed_fields into the command —enabled,days,time,mode_name, and a unit-convertedtemperature(e.g.140.0°F) alongside the raw half-Celsiusparam: 120.Both now send flat lists of raw protocol entries via a new
NavienBaseModel.to_protocol_dict()helper (declared fields only, by alias).Command queue reordering (
mqtt/command_queue.py): a command that failed mid-flush was re-queued at the tail, behind newer commands —[power_on, set_temp]became[set_temp, power_on]on the next flush. The queue is now adequeand failed commands are re-inserted at the front.Stale command replay (
mqtt/command_queue.py,mqtt/utils.py):QueuedCommand.timestampwas stored but never checked, so a multi-hour outage replayed hours-oldset_power/set_dhw_temperaturecommands to a physical water heater on reconnect. Commands older thanMqttConnectionConfig.max_queued_command_age(default 300 s;Nonedisables) are now discarded at send time with a warning.Sub-zero Fahrenheit off-by-one (
temperature.py): the ASYMMETRIC rounding formula used Python's floored%(always non-negative:-11 % 10 == 9) where the firmware/NaviLink app uses a truncated remainder (-1). Raw-11(-5.5 °C) decoded to 22 °F vs the app's 23 °F — routine for the outdoor temperature sensor in winter. Now usesmath.fmodsemantics; positive values unchanged.Freeze protection defaults in the wrong unit domain (
models/status.py): defaults43/65were Fahrenheit display values stored in raw half-Celsius fields, decoding to 70.7 °F / 90.5 °F when the device omits the fields — any UI clamping a slider to them would allow out-of-range setpoints. Corrected to raw12/20= 43 °F / 50 °F, matching the fixed limits documented indata_conversions.rst.Missing error-change events (
mqtt/state_tracker.py):error_detectedfired only on 0 → non-zero; an E799 → E407 transition emitted nothing, so consumers kept displaying the stale error.TOU price encoding (
encoding.py):encode_price()usedround()(banker's rounding), under-encoding exact halves at even boundaries (0.125 @ dp=2 → 12 instead of 13); nowDecimalROUND_HALF_UP.build_tou_period()treatedboolprices as pre-encoded integers (True→ price 1) because bool subclasses int.Protocol doc errors (
encoding.py,temperature.py): enable-flag convention documented inverted (1=enabled instead of 2=enabled);build_reservation_entry()example showedweek: 158for Mon/Wed/Fri instead of 84 (64+16+4); temperature doctests showed int raw values where floats are returned (all temperature.py doctests now actually pass).Tests
New
tests/test_protocol_correctness.py(17 tests): exact payload shape for both schedule commands,to_protocol_dict()exclusions, queue order preservation across failed flushes, expiry (incl.Nonedisable and overflow), negative/positive ASYMMETRIC conversions, freeze defaults, error-change/unchanged/cleared events, half-up price encoding, bool/int TOU price handling.Validation
pytest: 525 passedruff check/ruff format --check: cleanpyright src/nwp500: 0 errorsmypy src/nwp500: no issues