From a3c8d1e6f7192f44353428e54411a4f9b211b767 Mon Sep 17 00:00:00 2001 From: Emmanuel Levijarvi Date: Mon, 3 Aug 2026 10:43:38 -0700 Subject: [PATCH 1/3] Preserve the device's unknown state on nine status flags The protocol encodes these flags as 0 = unknown, 1 = OFF, 2 = ON, and the library was collapsing 0 to False - reporting a definite OFF for a field the device explicitly declined to answer. Navien's own client confirms 0 is reserved rather than a value. NaviLink 2.03.00 (versionCode 141) decodes exactly this set of status fields through KDEnum.MgppOnOFFFlag, declared UNKNOWN(0), OFF(1), ON(2). Eight of the app's enums reserve zero this way, and two of them render it as something a user reads as absent: HPWHHeatSource shows "-" and HPWHDREvent shows "Not Applied". operation_busy heat_upper_use comp_use heat_lower_use anti_legionella_use air_filter_alarm_use anti_legionella_operation_busy recirc_reservation_use These become bool | None via the new DeviceTriState annotation. drOverrideStatus is the ninth field the app decodes this way, but this library exposes it as a raw int rather than a flag, so it is untouched. OnOffFlag gains the vendor's UNKNOWN = 0; it previously started at OFF = 1 and left the reserved value unrepresented. The rule is deliberately not global, because zero is not a global sentinel. Three other cases are now documented rather than guessed at: Zero is a real state in five of the app's enums - MgppOperationMode and OperationMode are STANDBY, HydroOperationMode is STOP, HydroFsmState is INIT, FilterChange is NORMAL. A device idling reports 0 constantly, so a blanket rule would blank the operating mode most of the time. Capability flags mean something else again. The app hides a feature's entire UI when its DID Use flag reads 0, so there 0 means "not fitted" - a definite answer. device_bool_to_python keeps collapsing those to False, which is correct, and is now documented as such instead of incidental. Temperatures carry no sentinel at all. The app has no out-of-band constant (no 0xFFFF, -999 or -1 anywhere in status handling), no zero-guard in any display path - the only == 0 checks on the status screen are errorCode, minorCode, waterSprayStatus and airFilterAlarmPeriod - and getTempText formats whatever arrives, so a reported 0 renders as 32 degF. This is recorded with a warning, because applying zero-as-none to temperature converters is a mistake that has already been made and reverted once: it reported working sensors as missing during cold-weather operation. Also worth knowing: all 126 numeric fields in the app's status model are primitive int with no boxed types, so a field absent from the JSON deserializes to 0. The vendor cannot distinguish absent from zero either, which is why no protocol-level "unreported" concept exists to recover. None is falsy, so `if status.comp_use:` is unchanged. Code distinguishing `is False` from "not reported", or doing arithmetic on these fields, needs a None check. For Home Assistant this is the wanted shape - None renders as Unknown instead of writing a fabricated OFF into the recorder database. The CLI renders these as "Unknown" rather than "No". Replaces #69, which took 0 as a universal sentinel, applied it to temperatures, and could not be rebased - it edits src/nwp500/models.py, which is a package now, through converter-level validators that temperature.py has since replaced. --- CHANGELOG.rst | 50 ++++++ docs/explanation/index.rst | 1 + docs/explanation/unknown-values.rst | 257 ++++++++++++++++++++++++++++ src/nwp500/cli/presentation.py | 13 +- src/nwp500/converters.py | 68 +++++++- src/nwp500/enums.py | 6 + src/nwp500/models/status.py | 25 ++- tests/test_model_converters.py | 60 +++++++ tests/test_models.py | 74 ++++++++ 9 files changed, 539 insertions(+), 15 deletions(-) create mode 100644 docs/explanation/unknown-values.rst diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 244c7848..b6094775 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,56 @@ Changelog Unreleased ========== +**BREAKING CHANGE**: nine status flags change type from ``bool`` to +``bool | None`` so the device's "unknown" state is no longer reported as a +definite OFF. + +Changed +------- +- **Status flags now preserve the device's unknown state.** The protocol + encodes these flags as ``0 = unknown, 1 = OFF, 2 = ON``, and the library + was collapsing 0 to ``False`` - inventing an OFF the device never claimed. + Confirmed against Navien's own NaviLink app (2.03.00, versionCode 141), + which decodes exactly this set of fields through an enum declared + ``UNKNOWN(0), OFF(1), ON(2)``; two sibling enums render their zero as + ``"-"`` and ``"Not Applied"`` rather than as an off state. + + Affected: ``operation_busy``, ``comp_use``, ``anti_legionella_use``, + ``anti_legionella_operation_busy``, ``heat_upper_use``, ``heat_lower_use``, + ``air_filter_alarm_use``, ``recirc_reservation_use``. + + ``None`` is falsy, so ``if status.comp_use:`` is unaffected. Code that + distinguishes ``is False`` from "not reported", or does arithmetic or + formatting on these fields, needs a ``None`` check. For Home Assistant + this is the wanted shape: ``None`` renders as "Unknown" instead of writing + a fabricated OFF into the recorder database. + +- ``OnOffFlag`` gains the vendor's ``UNKNOWN = 0`` member. It previously + started at ``OFF = 1``, leaving the device's reserved value unrepresented. + +- The CLI renders these flags as ``Unknown`` rather than ``No``. + +Added +----- +- ``converters.device_tristate_to_python`` and + ``models.status.DeviceTriState`` for flags the device may decline to + report. ``converters.device_bool_to_python`` is unchanged and remains + correct for capability flags. +- New ``docs/explanation/unknown-values.rst`` recording which field families + use 0 as a sentinel and which do not, with the app evidence for each. + +Fixed +----- +- Documented that **temperature fields carry no sentinel at all**. The app + has no out-of-band constant (no ``0xFFFF``/``-999``/``-1``), no zero-guard + in any display path, and formats whatever arrives - so a temperature of + zero means zero. This closes a recurring source of bugs where zero-as-none + was applied to temperature converters and reported working sensors as + missing during cold-weather operation. +- Documented that capability flags are a distinct case: the app hides a + feature's entire UI when its DID ``Use`` flag reads 0, so 0 there means + "not fitted" and the existing ``bool`` mapping is correct. + Version 9.2.1 (2026-07-30) ========================== diff --git a/docs/explanation/index.rst b/docs/explanation/index.rst index f1fe4c9c..b7e116a4 100644 --- a/docs/explanation/index.rst +++ b/docs/explanation/index.rst @@ -9,3 +9,4 @@ Understanding-oriented deep dives into the library's design and advanced feature advanced-features architecture + unknown-values diff --git a/docs/explanation/unknown-values.rst b/docs/explanation/unknown-values.rst new file mode 100644 index 00000000..042acb7f --- /dev/null +++ b/docs/explanation/unknown-values.rst @@ -0,0 +1,257 @@ +============== +Unknown Values +============== + +The device sometimes declines to report a field. This page records what +the protocol actually does about that, which fields are affected, and - +just as importantly - which fields look affected but are not. + +The short answer: **there is no single rule about zero.** Zero is a +reserved sentinel in some field families, a real value in others, and +carries no special meaning at all for temperatures. Earlier attempts to +apply one rule library-wide produced bugs in both directions. + +.. contents:: + :local: + :depth: 2 + + +Where the evidence comes from +============================= + +Navien's own Android client, NaviLink, was decompiled - version 2.03.00, +versionCode 141, published March 2026. The app is a first-party decoder +for the same MQTT payloads this library parses, so its enum tables and +its null-handling are direct evidence of intent rather than inference +from observed traffic. + +Two cautions apply to everything below. The app is a *consumer* of the +protocol, not its specification, and it ignores a great deal of what the +device sends. Absence of handling in the app is therefore weak evidence +on its own; presence of an explicit sentinel is strong evidence. + + +Enum-coded flags: zero is a sentinel +==================================== + +The app decodes many status fields through enums in ``KDEnum.java``. The +generic on/off flag is declared: + +.. code:: java + + public enum MgppOnOFFFlag { + UNKNOWN(0, "Unknown"), + OFF(1, "OFF"), + ON(2, "ON"); + } + +Zero is reserved and real values start at 1. This is not an accident of +one enum - eight of the app's enums do it, and two render their zero as +something a user would read as "no data": + +.. list-table:: + :header-rows: 1 + :widths: 42 20 38 + + * - App enum + - Zero member + - Display text + * - ``MgppOnOFFFlag`` + - ``UNKNOWN`` + - "Unknown" + * - ``HPWHHeatSource`` + - ``UNKNOWN`` + - **"-"** + * - ``HPWHDREvent`` + - ``UNKNOWN`` + - **"Not Applied"** + * - ``MgppRecirculationOperationMode`` + - ``UNKNOWN`` + - "Unknown" + * - ``MgppDHWControlTypeFlag`` + - ``UNKNOWN`` + - "Unknown" + * - ``HydroElectricalEfficiencyMode`` + - ``UNKNOWN`` + - "Unknown" + * - ``MgppReservationMode`` + - ``NOT_RESERVATION`` + - "NOT RESERVATION" + * - ``firmwareType`` + - ``Unknown`` + - "Unknown" + + +But in five other enums zero is real +------------------------------------ + +The rule is not global, and this is where a blanket converter goes wrong: + +.. list-table:: + :header-rows: 1 + :widths: 42 58 + + * - App enum + - Zero means + * - ``MgppOperationMode`` + - ``STANDBY`` - a real, common state + * - ``OperationMode`` + - ``STANDBY`` + * - ``HydroOperationMode`` + - ``STOP`` + * - ``HydroFsmState`` + - ``INIT`` + * - ``FilterChange`` + - ``NORMAL`` - filter is fine + +A device sitting in standby reports 0 constantly. Treating that as +"unknown" would blank the operating mode most of the time. + + +What this library does +---------------------- + +:class:`~nwp500.enums.OnOffFlag` carries the vendor's ``UNKNOWN = 0`` +member, and nine status fields - exactly the set the app decodes through +``MgppOnOFFFlag`` - are typed +:data:`~nwp500.models.status.DeviceTriState`, which maps 0 to ``None``: + +- ``operation_busy`` +- ``comp_use`` +- ``anti_legionella_use`` +- ``anti_legionella_operation_busy`` +- ``heat_upper_use`` +- ``heat_lower_use`` +- ``air_filter_alarm_use`` +- ``recirc_reservation_use`` + +(``drOverrideStatus`` is the ninth in the app, but this library exposes it +as a raw ``int`` rather than a flag, so it is left alone.) + +Every other flag keeps :data:`~nwp500.models.status.DeviceBool`. + + +Capability flags: zero means "not fitted" +========================================= + +The DID/feature ``Use`` flags are a third case, and the app treats them +differently from status flags - it does not decode them through an enum +at all, and checks them directly: + +.. code:: java + + if (... feature.getRecirculationUse() == 0) { + this.viewDataBinding.layoutHotButton.setVisibility(8); + this.viewDataBinding.linearLayoutControlRecirculation.setVisibility(8); + return; + } + +Zero hides the entire recirculation UI. For a capability flag, zero means +"this device does not have the feature", which is a definite answer, not +an absent one. ``False`` is the faithful mapping and +:data:`~nwp500.models.feature.CapabilityFlag` is unchanged. + + +Temperatures carry no sentinel +============================== + +This is the important negative result, because it is the one that has +been guessed wrong before. + +- **No out-of-band values.** No ``0xFFFF``, ``-999`` or ``-1`` appears + anywhere in the app's status handling. The only ``65535`` constants in + the app are CRC16 masks and infrared remote-control codes. +- **No zero-guard in any display path.** The only ``== 0`` comparisons on + the status screen are ``errorCode``, ``minorCode``, ``waterSprayStatus`` + and ``airFilterAlarmPeriod``. Not one is a temperature. +- **The formatter is unconditional.** ``getTempText()`` converts and + prints whatever arrives, so a device reporting 0 would render as + 32 degF. The app has no notion of an unavailable temperature. + +So a temperature of zero means zero. The library does not map any +temperature field to ``None``. + +.. warning:: + Do not add zero-as-none to a temperature field on the strength of it + "always reading 0" in a capture. Ambient and tank temperatures can + legitimately reach 0 degC, and a heat pump in a cold garage will get + there. A previous attempt did exactly this and had to be reverted after + it reported working sensors as missing during cold-weather operation. + + +Absent and zero are indistinguishable +===================================== + +All 126 numeric fields in the app's status model are declared as +primitive ``int`` - there is not a single boxed ``Integer`` among them. A +field missing from the JSON therefore deserializes to ``0``, and the +vendor's own client cannot tell "not reported" from "reported as zero" +either. + +There is consequently no protocol-level concept of "unreported" to +recover. Where a field is genuinely absent from the payload, that is +visible to this library through Pydantic's own missing-field handling, +not through any sentinel. + + +Fields the app never displays +============================= + +Several fields that look like natural candidates for N/A handling are +simply not rendered by the app for this device type. They exist as +getters on the status model with no UI call site: + +``outsideTemperature``, ``mixingRate``, ``currentInletTemperature``, +``dhwTemperature2``, ``recircTemperature``, ``recircFaucetTemperature``, +``heLowerOnTempSetting``. + +Observing that one of these "is always 0" in a capture is not evidence +that 0 is a sentinel. It is equally consistent with the sensor being +absent, the feature being unfitted, or the field being unused on this +model. The app makes no determination, so neither does this library. + +The one place the app does treat a numeric zero as "not configured" is +``airFilterAlarmPeriod``, and it handles it by substituting a different +*message* ("filter setup needed") rather than blanking a value - a +per-field UI decision, not a converter-level rule. + + +Migration +========= + +Nine fields change type from ``bool`` to ``bool | None``. ``None`` is +falsy, so truthiness checks are unaffected: + +.. code:: python + + if status.comp_use: # unchanged + ... + +The risk is in negative and identity checks, which no longer mean the +same thing: + +.. code:: python + + # Before: True for both OFF and unknown + # After: True for both OFF and unknown - but now you can tell them apart + if not status.comp_use: + ... + + # Distinguish explicitly + if status.comp_use is None: + ... # device is not reporting + elif status.comp_use: + ... # compressor running + +Anything doing arithmetic or formatting on these fields needs a ``None`` +check. For Home Assistant this is the desired shape: ``None`` renders as +"Unknown" rather than recording a fabricated OFF into the recorder +database and skewing history. + +The CLI renders these as ``Unknown`` rather than ``No``. + + +See also +======== + +- :doc:`../reference/protocol/data_conversions` - protocol field conversions diff --git a/src/nwp500/cli/presentation.py b/src/nwp500/cli/presentation.py index 8fcbf9dc..150301d2 100644 --- a/src/nwp500/cli/presentation.py +++ b/src/nwp500/cli/presentation.py @@ -29,6 +29,17 @@ def _format_number(value: Any) -> str: return str(value) +def _yes_no_unknown(value: bool | None) -> str: + """Render a tri-state device flag. + + ``None`` means the device reported its reserved 0 and is not claiming a + state, so it must not collapse to "No". + """ + if value is None: + return "Unknown" + return "Yes" if value else "No" + + def _get_unit_suffix( field_name: str, model_class: Any = DeviceStatus, @@ -119,7 +130,7 @@ def build_device_status_rows(device_status: Any) -> list[StatusRow]: ( "OPERATION STATUS", "Busy", - "Yes" if device_status.operation_busy else "No", + _yes_no_unknown(device_status.operation_busy), ) ) if hasattr(device_status, "current_statenum"): diff --git a/src/nwp500/converters.py b/src/nwp500/converters.py index 01dec4ee..c8f89b4c 100644 --- a/src/nwp500/converters.py +++ b/src/nwp500/converters.py @@ -13,6 +13,7 @@ __all__ = [ "device_bool_to_python", "device_bool_from_python", + "device_tristate_to_python", "tou_override_to_python", "div_10", "mul_10", @@ -23,28 +24,83 @@ def device_bool_to_python(value: Any) -> bool: """Convert device boolean representation to Python bool. - Device protocol uses: 1 = OFF/False, 2 = ON/True + Device protocol uses: 1 = OFF/False, 2 = ON/True, 0 = unknown/absent. - This design (using 1 and 2 instead of 0 and 1) is likely due to: - - 0 being reserved for null/uninitialized state - - 1 representing "off" in legacy firmware - - 2 representing "on" state + The 1/2 encoding is not arbitrary. The vendor's own client decodes these + fields through an enum declared ``UNKNOWN(0), OFF(1), ON(2)``, so 0 is a + reserved sentinel rather than a value. + + This converter collapses 0 to ``False``, which is correct for **capability + flags** (:data:`~nwp500.models.feature.CapabilityFlag`): the NaviLink app + hides a feature's entire UI when its ``Use`` flag reads 0, so 0 there means + "this device does not have the feature" and ``False`` is faithful. + + For **status flags**, where 0 means "the device is not reporting this right + now", collapsing to ``False`` invents an OFF the device never claimed. Use + :func:`device_tristate_to_python` for those. Args: value: Device value (typically 1 or 2). Returns: - Python boolean (1→False, 2→True). + Python boolean (1→False, 2→True, 0→False). Example: >>> device_bool_to_python(2) True >>> device_bool_to_python(1) False + >>> device_bool_to_python(0) + False """ return bool(value == 2) +def device_tristate_to_python(value: Any) -> bool | None: + """Convert a device on/off flag, preserving the device's unknown state. + + Identical to :func:`device_bool_to_python` except that the protocol's + reserved 0 maps to ``None`` instead of being flattened into ``False``. + + The device distinguishes three states and the library should not throw one + away. The NaviLink app decodes the affected status fields through + ``KDEnum.MgppOnOFFFlag``, which is declared ``UNKNOWN(0), OFF(1), ON(2)``; + two of the app's sibling enums render their zero as ``"-"`` and + ``"Not Applied"`` rather than as an off state. + + ``None`` is the right shape for the downstream consumer too: Home Assistant + renders it as "Unknown" instead of recording a fabricated OFF into the + history database. + + Note this applies only to flags the vendor itself decodes as an on/off + enum. It is **not** a general rule about zero - see + :func:`device_bool_to_python` for capability flags, and note that + temperature fields carry no sentinel at all. + + Args: + value: Device value (0, 1 or 2). + + Returns: + ``None`` when the device reports 0, otherwise 1→False, 2→True. + + Example: + >>> device_tristate_to_python(2) + True + >>> device_tristate_to_python(1) + False + >>> device_tristate_to_python(0) is None + True + """ + if value is None: + return None + try: + if int(value) == 0: + return None + except TypeError, ValueError: + return bool(value == 2) + return bool(int(value) == 2) + + def device_bool_from_python(value: bool) -> int: """Convert Python bool to device boolean representation. diff --git a/src/nwp500/enums.py b/src/nwp500/enums.py index ae726672..4a50688b 100644 --- a/src/nwp500/enums.py +++ b/src/nwp500/enums.py @@ -19,8 +19,14 @@ class OnOffFlag(IntEnum): Used for: Power status, TOU status, recirculation status, vacation mode, anti-legionella, and many other boolean device settings. + + ``UNKNOWN`` is the device's own sentinel, not a library invention. The + NaviLink app decodes these fields through an enum declared + ``UNKNOWN(0), OFF(1), ON(2)``, reserving 0 and starting real values at 1. + See :doc:`/explanation/unknown-values`. """ + UNKNOWN = 0 OFF = 1 ON = 2 diff --git a/src/nwp500/models/status.py b/src/nwp500/models/status.py index 1c231502..d3ff9627 100644 --- a/src/nwp500/models/status.py +++ b/src/nwp500/models/status.py @@ -5,6 +5,7 @@ from .._base import NavienBaseModel from ..converters import ( device_bool_to_python, + device_tristate_to_python, div_10, mul_10, tou_override_to_python, @@ -29,6 +30,14 @@ from ..unit_system import get_unit_system DeviceBool = Annotated[bool, BeforeValidator(device_bool_to_python)] +#: An on/off flag the device may decline to report. 0 becomes ``None`` +#: rather than ``False``, because the device reserves 0 to mean "unknown" +#: and a fabricated OFF is worse than an absent value. Applied only to the +#: fields the vendor's own client decodes through its on/off enum - see +#: ``docs/explanation/unknown-values.rst``. +DeviceTriState = Annotated[ + bool | None, BeforeValidator(device_tristate_to_python) +] Div10 = Annotated[float, BeforeValidator(div_10)] TenWhToWh = Annotated[float, BeforeValidator(mul_10)] TouStatus = Annotated[bool, BeforeValidator(bool)] @@ -256,7 +265,7 @@ class DeviceStatus(NavienBaseModel): did_reload: DeviceBool = Field( description="Indicates if the device has recently reloaded or restarted" ) - operation_busy: DeviceBool = Field( + operation_busy: DeviceTriState = Field( description=( "Indicates if the device is currently performing heating operations" ) @@ -295,7 +304,7 @@ class DeviceStatus(NavienBaseModel): "Whether ECO (Energy Cut Off) high-temp safety limit is triggered" ) ) - comp_use: DeviceBool = Field( + comp_use: DeviceTriState = Field( description=( "Compressor usage status (True=On, False=Off). " "The compressor is the main component of the heat pump" @@ -328,13 +337,13 @@ class DeviceStatus(NavienBaseModel): "Triggers error E799 if leak detected" ) ) - anti_legionella_use: DeviceBool = Field( + anti_legionella_use: DeviceTriState = Field( description=( "Whether anti-legionella function is enabled. " "Device periodically heats tank to prevent Legionella bacteria" ) ) - anti_legionella_operation_busy: DeviceBool = Field( + anti_legionella_operation_busy: DeviceTriState = Field( description=( "Whether the anti-legionella disinfection cycle " "is currently running" @@ -350,13 +359,13 @@ class DeviceStatus(NavienBaseModel): "1=Heat Pump, 2=Electric Element, 3=Both simultaneously" ) ) - heat_upper_use: DeviceBool = Field( + heat_upper_use: DeviceTriState = Field( description=( "Upper electric heating element usage status. " "Power: 3,755W @ 208V or 5,000W @ 240V" ) ) - heat_lower_use: DeviceBool = Field( + heat_lower_use: DeviceTriState = Field( description=( "Lower electric heating element usage status. " "Power: 3,755W @ 208V or 5,000W @ 240V" @@ -368,7 +377,7 @@ class DeviceStatus(NavienBaseModel): "Warning when water reaches potentially hazardous levels" ) ) - air_filter_alarm_use: DeviceBool = Field( + air_filter_alarm_use: DeviceTriState = Field( description=( "Air filter maintenance reminder enabled flag. " "Triggers alerts based on operating hours. Default: On" @@ -377,7 +386,7 @@ class DeviceStatus(NavienBaseModel): recirc_operation_busy: DeviceBool = Field( description="Recirculation operation busy status" ) - recirc_reservation_use: DeviceBool = Field( + recirc_reservation_use: DeviceTriState = Field( description="Recirculation reservation usage status" ) diff --git a/tests/test_model_converters.py b/tests/test_model_converters.py index 706718d8..5674ec13 100644 --- a/tests/test_model_converters.py +++ b/tests/test_model_converters.py @@ -13,6 +13,7 @@ from nwp500.converters import ( device_bool_to_python, + device_tristate_to_python, div_10, enum_validator, mul_10, @@ -60,6 +61,65 @@ def test_float_off(self): """Float 1.0 is not equal to 2.""" assert device_bool_to_python(1.0) is False + def test_zero_collapses_to_false_for_capability_flags(self): + """0 stays False here; that is correct for capability flags. + + The NaviLink app hides a feature's entire UI when its DID ``Use`` + flag reads 0, so for capabilities 0 means "device does not have + this" and False is faithful. Status flags use + device_tristate_to_python instead. + """ + assert device_bool_to_python(0) is False + + +class TestDeviceTriStateConverter: + """Test device_tristate_to_python converter. + + The device encodes these flags as 0 = UNKNOWN, 1 = OFF, 2 = ON. The + vendor's own client decodes them through an enum declared + ``UNKNOWN(0), OFF(1), ON(2)``, so 0 is a reserved sentinel and must not + become False. + """ + + def test_unknown_value(self): + """Device value 0 converts to None, not False.""" + assert device_tristate_to_python(0) is None + + def test_off_value(self): + """Device value 1 converts to False.""" + assert device_tristate_to_python(1) is False + + def test_on_value(self): + """Device value 2 converts to True.""" + assert device_tristate_to_python(2) is True + + def test_none_passes_through(self): + """A missing value stays None rather than becoming False.""" + assert device_tristate_to_python(None) is None + + @pytest.mark.parametrize( + ("raw", "expected"), + [("0", None), ("1", False), ("2", True), (0.0, None), (2.0, True)], + ) + def test_coerces_numeric_strings_and_floats(self, raw, expected): + """Unlike device_bool_to_python, this coerces before comparing. + + device_bool_to_python compares with ``== 2`` and so reports False + for the string "2". That quirk is pinned by its own tests; this + converter does not reproduce it, because mapping a real ON to + False is exactly the failure mode it exists to prevent. + """ + assert device_tristate_to_python(raw) is expected + + def test_unparseable_value_does_not_raise(self): + """A non-numeric value falls back rather than blowing up.""" + assert device_tristate_to_python("garbage") is False + + def test_three_states_are_distinguishable(self): + """The whole point: 0, 1 and 2 map to three distinct results.""" + results = [device_tristate_to_python(v) for v in (0, 1, 2)] + assert results == [None, False, True] + def test_float_on(self): """Float 2.0 equals int 2 in Python.""" assert device_bool_to_python(2.0) is True diff --git a/tests/test_models.py b/tests/test_models.py index 3e097bf7..ef70a98a 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,3 +1,5 @@ +from typing import ClassVar + import pytest from nwp500.models import ( @@ -273,3 +275,75 @@ def test_skips_empty_entries(self): ) assert len(schedule.reservation) == 1 assert schedule.reservation[0].week == 62 + + +class TestTriStateFlags: + """Status flags the device may decline to report. + + The device encodes these as 0 = UNKNOWN, 1 = OFF, 2 = ON. The vendor's + own client decodes exactly this set through ``KDEnum.MgppOnOFFFlag``, + declared ``UNKNOWN(0), OFF(1), ON(2)``, so 0 must not become False. + """ + + #: Fields the NaviLink app decodes through its on/off enum. Kept here so + #: the evidenced set is asserted rather than assumed. + TRISTATE_FIELDS = ( + "operation_busy", + "comp_use", + "anti_legionella_use", + "anti_legionella_operation_busy", + "heat_upper_use", + "heat_lower_use", + "air_filter_alarm_use", + "recirc_reservation_use", + ) + + ALIASES: ClassVar[dict[str, str]] = { + "operation_busy": "operationBusy", + "comp_use": "compUse", + "anti_legionella_use": "antiLegionellaUse", + "anti_legionella_operation_busy": "antiLegionellaOperationBusy", + "heat_upper_use": "heatUpperUse", + "heat_lower_use": "heatLowerUse", + "air_filter_alarm_use": "airFilterAlarmUse", + "recirc_reservation_use": "recircReservationUse", + } + + @pytest.mark.parametrize("field", TRISTATE_FIELDS) + def test_zero_becomes_none(self, default_status_data, field): + """0 surfaces as None rather than a fabricated OFF.""" + data = dict(default_status_data) + data[self.ALIASES[field]] = 0 + assert getattr(DeviceStatus(**data), field) is None + + @pytest.mark.parametrize("field", TRISTATE_FIELDS) + def test_one_is_off(self, default_status_data, field): + """1 is a real OFF and stays False.""" + data = dict(default_status_data) + data[self.ALIASES[field]] = 1 + assert getattr(DeviceStatus(**data), field) is False + + @pytest.mark.parametrize("field", TRISTATE_FIELDS) + def test_two_is_on(self, default_status_data, field): + """2 is a real ON and stays True.""" + data = dict(default_status_data) + data[self.ALIASES[field]] = 2 + assert getattr(DeviceStatus(**data), field) is True + + def test_unknown_is_falsy_but_distinguishable(self, default_status_data): + """None is falsy, so naive `if flag:` keeps its old behaviour. + + The migration risk is the opposite check: `if not flag` and + `flag is False` no longer mean the same thing. + """ + data = dict(default_status_data) + data["compUse"] = 0 + status = DeviceStatus(**data) + assert not status.comp_use + assert status.comp_use is not False + + def test_capability_flags_are_unaffected(self, default_status_data): + """Plain DeviceBool status fields still collapse 0 to False.""" + data = dict(default_status_data) + data["dhwUse"] = 0 + assert DeviceStatus(**data).dhw_use is False From 825cfdf456b408c446ce516bb7a164583902ee60 Mon Sep 17 00:00:00 2001 From: Emmanuel Levijarvi Date: Mon, 3 Aug 2026 11:01:51 -0700 Subject: [PATCH 2/3] Address Copilot review on #121 Three of the four comments were correct. The CLI miss is a real bug. anti_legionella_operation_busy is tri-state now but build_device_status_rows still rendered it with a two-way ternary, so an unknown showed as "No" - exactly the failure this branch exists to remove, reintroduced one screen over. I had grepped only the narrower pattern and missed the second call site. Fixed, and examples/advanced/combined_callbacks.py had the same shape ('On' if comp_use else 'Off') and is fixed too. Adds tests/test_cli_tristate_presentation.py to stop this recurring. It checks all three states end to end through build_device_status_rows, and carries a structural guard that fails if any tri-state field is rendered with a two-way ternary anywhere in presentation.py. Both were confirmed to fail when the bug is reintroduced and pass when it is not. The module skips cleanly without the optional cli extra, since CI installs it only through tox. The two count comments were also right: the prose said nine fields while the list held eight. Nine is the app's count; the library types eight of them, because drOverrideStatus is exposed as a raw int rather than a flag. Reworded so the two numbers cannot be read as contradicting. The syntax comment is incorrect and no change is made. Copilot flagged `except TypeError, ValueError:` as invalid Python 3 that would raise SyntaxError on import. That is PEP 758, valid from Python 3.14, which this project requires (setup.cfg python_requires >= 3.14). It parses on 3.14 and raises only on 3.13, ruff 0.16.1 normalises to this form, and the suite imports and passes. Worth recording that this line has now misled two readers, but the fix is not to fight the formatter. --- CHANGELOG.rst | 6 +- docs/explanation/unknown-values.rst | 10 +- examples/advanced/combined_callbacks.py | 8 +- src/nwp500/cli/presentation.py | 2 +- tests/test_cli_tristate_presentation.py | 126 ++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 9 deletions(-) create mode 100644 tests/test_cli_tristate_presentation.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b6094775..0a2ebd7c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,7 +5,7 @@ Changelog Unreleased ========== -**BREAKING CHANGE**: nine status flags change type from ``bool`` to +**BREAKING CHANGE**: eight status flags change type from ``bool`` to ``bool | None`` so the device's "unknown" state is no longer reported as a definite OFF. @@ -32,7 +32,9 @@ Changed - ``OnOffFlag`` gains the vendor's ``UNKNOWN = 0`` member. It previously started at ``OFF = 1``, leaving the device's reserved value unrepresented. -- The CLI renders these flags as ``Unknown`` rather than ``No``. +- The CLI renders these flags as ``Unknown`` rather than ``No``. Both + affected rows are updated: "Busy" under OPERATION STATUS and + "Operation Busy" under ANTI-LEGIONELLA. Added ----- diff --git a/docs/explanation/unknown-values.rst b/docs/explanation/unknown-values.rst index 042acb7f..08f3df8d 100644 --- a/docs/explanation/unknown-values.rst +++ b/docs/explanation/unknown-values.rst @@ -112,8 +112,8 @@ What this library does ---------------------- :class:`~nwp500.enums.OnOffFlag` carries the vendor's ``UNKNOWN = 0`` -member, and nine status fields - exactly the set the app decodes through -``MgppOnOFFFlag`` - are typed +member. The app decodes nine status fields through ``MgppOnOFFFlag``; +eight of them exist here as flags and are typed :data:`~nwp500.models.status.DeviceTriState`, which maps 0 to ``None``: - ``operation_busy`` @@ -125,8 +125,8 @@ member, and nine status fields - exactly the set the app decodes through - ``air_filter_alarm_use`` - ``recirc_reservation_use`` -(``drOverrideStatus`` is the ninth in the app, but this library exposes it -as a raw ``int`` rather than a flag, so it is left alone.) +The ninth is ``drOverrideStatus``, which this library exposes as a raw +``int`` rather than a flag, so it is left alone. Every other flag keeps :data:`~nwp500.models.status.DeviceBool`. @@ -219,7 +219,7 @@ per-field UI decision, not a converter-level rule. Migration ========= -Nine fields change type from ``bool`` to ``bool | None``. ``None`` is +Eight fields change type from ``bool`` to ``bool | None``. ``None`` is falsy, so truthiness checks are unaffected: .. code:: python diff --git a/examples/advanced/combined_callbacks.py b/examples/advanced/combined_callbacks.py index d0593ca7..08cda0ac 100755 --- a/examples/advanced/combined_callbacks.py +++ b/examples/advanced/combined_callbacks.py @@ -92,7 +92,13 @@ def on_status(status: DeviceStatus): print(f" Mode: {status.operation_mode.name}") print(f" DHW Temp: {status.dhw_temperature:.1f}{unit}") print(f" DHW Charge: {status.dhw_charge_per:.1f}%") - print(f" Compressor: {'On' if status.comp_use else 'Off'}") + # comp_use is tri-state: None means the device is not + # reporting, which must not be shown as "Off". + if status.comp_use is None: + comp = "Unknown" + else: + comp = "On" if status.comp_use else "Off" + print(f" Compressor: {comp}") # Callback for feature/capability info def on_feature(feature: DeviceFeature): diff --git a/src/nwp500/cli/presentation.py b/src/nwp500/cli/presentation.py index 150301d2..e912fa94 100644 --- a/src/nwp500/cli/presentation.py +++ b/src/nwp500/cli/presentation.py @@ -579,7 +579,7 @@ def build_device_status_rows(device_status: Any) -> list[StatusRow]: ( "ANTI-LEGIONELLA", "Operation Busy", - "Yes" if device_status.anti_legionella_operation_busy else "No", + _yes_no_unknown(device_status.anti_legionella_operation_busy), ) ) diff --git a/tests/test_cli_tristate_presentation.py b/tests/test_cli_tristate_presentation.py new file mode 100644 index 00000000..01c08359 --- /dev/null +++ b/tests/test_cli_tristate_presentation.py @@ -0,0 +1,126 @@ +"""CLI rendering of tri-state status flags. + +The device encodes these flags as 0 = unknown, 1 = OFF, 2 = ON. A field +reading 0 must never be presented as "No" - that reports a definite OFF +the device never claimed. + +This module guards the presentation layer specifically, because the model +change is easy to make without updating every render site: a missed one +is silent, since ``None`` is falsy and a two-way ternary happily prints +"No". +""" + +import pytest + +# The CLI package pulls in the optional `cli` extra (click/rich). Skip +# rather than hard-fail when running against a bare install. +pytest.importorskip("rich", reason="requires the 'cli' extra") +pytest.importorskip("click", reason="requires the 'cli' extra") + +from nwp500.cli.presentation import ( + _yes_no_unknown, + build_device_status_rows, +) +from nwp500.models import DeviceStatus + +# Status flags typed DeviceTriState, with the row label the CLI gives each. +# Only those actually rendered by build_device_status_rows appear here. +RENDERED_TRISTATE_ROWS = { + "operation_busy": ("OPERATION STATUS", "Busy"), + "anti_legionella_operation_busy": ("ANTI-LEGIONELLA", "Operation Busy"), +} + +TRISTATE_ALIASES = { + "operation_busy": "operationBusy", + "anti_legionella_operation_busy": "antiLegionellaOperationBusy", +} + + +class TestYesNoUnknown: + """The formatter itself.""" + + def test_none_is_unknown(self): + assert _yes_no_unknown(None) == "Unknown" + + def test_true_is_yes(self): + assert _yes_no_unknown(True) == "Yes" + + def test_false_is_no(self): + assert _yes_no_unknown(False) == "No" + + def test_all_three_are_distinct(self): + rendered = [_yes_no_unknown(v) for v in (None, False, True)] + assert len(set(rendered)) == 3 + + +class TestTriStateRendering: + """End-to-end: a 0 on the wire must reach the CLI as "Unknown".""" + + def _rows(self, status_data, alias, raw): + """Build the CLI rows, keyed by (section, label).""" + data = dict(status_data) + data[alias] = raw + return { + (section, label): value + for section, label, value in build_device_status_rows( + DeviceStatus(**data) + ) + } + + @pytest.mark.parametrize( + ("field", "location"), list(RENDERED_TRISTATE_ROWS.items()) + ) + def test_zero_renders_unknown(self, device_status_dict, field, location): + """0 must not be presented as "No".""" + rows = self._rows(device_status_dict, TRISTATE_ALIASES[field], 0) + assert rows[location] == "Unknown" + + @pytest.mark.parametrize( + ("field", "location"), list(RENDERED_TRISTATE_ROWS.items()) + ) + def test_one_renders_no(self, device_status_dict, field, location): + """1 is a real OFF and still reads "No".""" + rows = self._rows(device_status_dict, TRISTATE_ALIASES[field], 1) + assert rows[location] == "No" + + @pytest.mark.parametrize( + ("field", "location"), list(RENDERED_TRISTATE_ROWS.items()) + ) + def test_two_renders_yes(self, device_status_dict, field, location): + """2 is a real ON and still reads "Yes".""" + rows = self._rows(device_status_dict, TRISTATE_ALIASES[field], 2) + assert rows[location] == "Yes" + + +def test_no_tristate_field_uses_a_two_way_ternary(): + """Structural guard against the miss this test module exists for. + + A two-way ``"Yes" if else "No"`` compiles and passes any + test that only exercises 1 and 2, then silently mislabels unknown as + "No". Catch it in the source instead. + """ + from pathlib import Path + + import nwp500.cli.presentation as presentation + + source = Path(presentation.__file__).read_text() + tristate_fields = ( + "operation_busy", + "comp_use", + "anti_legionella_use", + "anti_legionella_operation_busy", + "heat_upper_use", + "heat_lower_use", + "air_filter_alarm_use", + "recirc_reservation_use", + ) + offenders = [ + line.strip() + for line in source.splitlines() + if '"Yes" if' in line + and any(f".{field}" in line for field in tristate_fields) + ] + assert not offenders, ( + "tri-state fields rendered with a two-way ternary " + f"(use _yes_no_unknown): {offenders}" + ) From d6c95fdd9f026e750af95de50ac783599f0cef13 Mon Sep 17 00:00:00 2001 From: Emmanuel Levijarvi Date: Mon, 3 Aug 2026 11:17:32 -0700 Subject: [PATCH 3/3] Fix every Sphinx docs warning (203 -> 0) I waved these off as pre-existing when they surfaced during this branch's docs build. They were pre-existing - a clean build of main produces the same seven warning types, and this branch introduces none - but that was not a reason to leave them, and I should have checked before saying so rather than after. Three broken includes, which were also swallowing docutils InputErrors in the build log: docs/project/authors.rst ../AUTHORS.rst -> ../../AUTHORS.rst docs/project/changelog.rst ../CHANGELOG.rst -> ../../CHANGELOG.rst docs/project/license.rst ../LICENSE.txt -> ../../LICENSE.txt Each resolved to docs/ rather than the repository root, so the pages rendered empty. That is also what produced the "doesn't have a title" toctree warnings for project/authors and project/changelog: with the include failing there was no content, so there was no title to find. Fixing the paths fixes both symptoms. LICENSE.txt is included :literal: since plain text is not valid reStructuredText. Two dead cross-references: docs/reference/installation.rst pointed at :doc:`quickstart`, which does not exist; the page is tutorials/getting-started. mqtt_events.MqttClientEvents pointed at :doc:`../guides/event_system`; there is no guides/ directory, and the events documentation is at reference/python_api/events. One orphan: protocol/quick_reference was in no toctree. Added to the Protocol Reference list in docs/reference/index.rst. Two code blocks in how-to/manage-units.rst were marked python but hold formulas with degree symbols, which Pygments cannot lex as Python. Retyped as text, which is what they are. Also removed docs/api/, stale sphinx-apidoc output from January still sitting in the working tree. It is gitignored, so it never reached CI, but it generated four more warnings on every local build and made the real ones harder to see. Verified: sphinx now reports "build succeeded." with zero warnings, and the previously empty authors/changelog/license pages render their content (511, 126014 and 1598 characters respectively). 708 tests pass, ruff and mypy clean. --- .gitignore | 2 ++ docs/how-to/manage-units.rst | 4 ++-- docs/project/authors.rst | 2 +- docs/project/changelog.rst | 2 +- docs/project/license.rst | 3 ++- docs/reference/index.rst | 1 + docs/reference/installation.rst | 2 +- src/nwp500/mqtt_events.py | 2 +- 8 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index d82c8693..c164dd6d 100644 --- a/.gitignore +++ b/.gitignore @@ -49,6 +49,8 @@ build/* dist/* sdist/* docs/api/* +docs/reference/api/* +.obsidian/ docs/_rst/* docs/_build/* cover/* diff --git a/docs/how-to/manage-units.rst b/docs/how-to/manage-units.rst index 6dd038f1..c1458916 100644 --- a/docs/how-to/manage-units.rst +++ b/docs/how-to/manage-units.rst @@ -239,7 +239,7 @@ Temperature Conversions **Celsius to Fahrenheit** -.. code-block:: python +.. code-block:: text fahrenheit = (celsius * 9/5) + 32 @@ -248,7 +248,7 @@ Temperature Conversions **Fahrenheit to Celsius** -.. code-block:: python +.. code-block:: text celsius = (fahrenheit - 32) * 5/9 diff --git a/docs/project/authors.rst b/docs/project/authors.rst index cd8e0913..24b97763 100644 --- a/docs/project/authors.rst +++ b/docs/project/authors.rst @@ -1,2 +1,2 @@ .. _authors: -.. include:: ../AUTHORS.rst +.. include:: ../../AUTHORS.rst diff --git a/docs/project/changelog.rst b/docs/project/changelog.rst index 871950df..9d6b6b33 100644 --- a/docs/project/changelog.rst +++ b/docs/project/changelog.rst @@ -1,2 +1,2 @@ .. _changes: -.. include:: ../CHANGELOG.rst +.. include:: ../../CHANGELOG.rst diff --git a/docs/project/license.rst b/docs/project/license.rst index 3989c513..3325f3f3 100644 --- a/docs/project/license.rst +++ b/docs/project/license.rst @@ -4,4 +4,5 @@ License ======= -.. include:: ../LICENSE.txt +.. include:: ../../LICENSE.txt + :literal: diff --git a/docs/reference/index.rst b/docs/reference/index.rst index e4cb0b19..e6b38347 100644 --- a/docs/reference/index.rst +++ b/docs/reference/index.rst @@ -25,6 +25,7 @@ Protocol Reference .. toctree:: :maxdepth: 1 + protocol/quick_reference protocol/rest_api protocol/mqtt_protocol protocol/device_status diff --git a/docs/reference/installation.rst b/docs/reference/installation.rst index a0ba2f0a..5bc2d6ff 100644 --- a/docs/reference/installation.rst +++ b/docs/reference/installation.rst @@ -152,6 +152,6 @@ To upgrade to a specific version: Next Steps ========== -* :doc:`quickstart` - Get started with your first script +* :doc:`../tutorials/getting-started` - Get started with your first script * :doc:`configuration` - Configure credentials and options * :doc:`python_api/auth_client` - Learn about authentication diff --git a/src/nwp500/mqtt_events.py b/src/nwp500/mqtt_events.py index 3ab6ac00..09986d99 100644 --- a/src/nwp500/mqtt_events.py +++ b/src/nwp500/mqtt_events.py @@ -224,7 +224,7 @@ class MqttClientEvents: print(f"Available events: {events}") See Also: - :doc:`../guides/event_system` - Comprehensive event handling guide + :doc:`/reference/python_api/events` - Comprehensive event handling guide """ # Connection lifecycle events