Skip to content

Preserve the device's unknown state on nine status flags - #121

Merged
eman merged 4 commits into
mainfrom
feat/unknown-flag-support
Aug 3, 2026
Merged

Preserve the device's unknown state on nine status flags#121
eman merged 4 commits into
mainfrom
feat/unknown-flag-support

Conversation

@eman

@eman eman commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Replaces #69.

The device encodes certain flags as 0 = unknown, 1 = OFF, 2 = ON. The library collapsed 0 to False, so a field the device explicitly declined to answer was reported as a definite OFF. Nine status flags now surface that third state as None.

Why this is the device's sentinel, not our guess

Navien's own client settles it. NaviLink 2.03.00 (versionCode 141) decodes exactly this set of status fields through KDEnum.MgppOnOFFFlag:

public enum MgppOnOFFFlag {
    UNKNOWN(0, "Unknown"),
    OFF(1, "OFF"),
    ON(2, "ON");
}

Zero is reserved and real values start at 1. Eight of the app's enums do this, and two render their zero as something a user reads as absent — HPWHHeatSource shows "-" and HPWHDREvent shows "Not Applied".

What changed

operation_busy, comp_use, anti_legionella_use, anti_legionella_operation_busy, heat_upper_use, heat_lower_use, air_filter_alarm_use, recirc_reservation_use become bool | None via a 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, leaving the reserved value unrepresented.

What deliberately did not change

The rule is not global, because zero is not a global sentinel. #69 assumed it was, and that is why it kept producing bugs in both directions. Three other cases are now documented rather than guessed at:

Zero is a real state in five of the app's enumsMgppOperationMode/OperationMode = STANDBY, HydroOperationMode = STOP, HydroFsmState = INIT, FilterChange = NORMAL. A device idling reports 0 constantly; a blanket rule would blank the operating mode most of the time.

Capability flags mean "not fitted". The app hides a feature's entire UI when its DID Use flag reads 0 — if (feature.getRecirculationUse() == 0) { ...setVisibility(8); return; }. That is a definite answer, so device_bool_to_python keeps collapsing those to False, now documented as correct rather than incidental.

Temperatures carry no sentinel at all. No out-of-band constant anywhere in status handling (no 0xFFFF, -999, -1 — the only 65535s in the app are CRC16 masks and IR remote codes). No zero-guard in any display path: the only == 0 checks on the status screen are errorCode, minorCode, waterSprayStatus and airFilterAlarmPeriod, none of them a temperature. getTempText() formats whatever arrives, so a reported 0 renders as 32 °F.

This one carries a warning in the docs, because it is a mistake already made and reverted once on #69 — zero-as-none on deci_celsius_to_preferred reported working sensors as missing during cold-weather operation. Ambient and tank temperatures reach 0 °C legitimately.

Two things worth knowing

The vendor can't distinguish absent from zero either. All 126 numeric fields in the app's status model are primitive int — not one boxed type — so a field missing from the JSON deserializes to 0. There is no protocol-level "unreported" concept to recover.

"Always 0 in a capture" is not evidence. outsideTemperature, mixingRate, currentInletTemperature, dhwTemperature2, recircTemperature, recircFaucetTemperature and heLowerOnTempSetting all exist as getters with no UI call site — the app never renders them for this device type. #69 read their constant zeros in HAR captures as "sensor not present"; it is equally consistent with the field being unused on this model. The app makes no determination, so neither does this library.

Migration

None is falsy, so truthiness is unaffected:

if status.comp_use:      # unchanged

The risk is identity and negative checks, and anything doing arithmetic or formatting:

if status.comp_use is None:   # device is not reporting
elif status.comp_use:         # compressor running

For Home Assistant this is the wanted shape: None renders as "Unknown" instead of writing a fabricated OFF into the recorder database and skewing history. The CLI renders these as Unknown rather than No.

Testing

  • 670 passed, 4 skipped (was 638) on Python 3.14 — 32 new tests covering all three states per field, the falsy-but-distinguishable case, and that capability flags are unaffected
  • Existing fixtures all carry 1, so no existing assertion changes meaning
  • ruff check + ruff format --check clean, mypy clean across 54 files
  • Docs build with no new warnings; explanation/unknown-values.rst records the evidence and the migration

Note the repo requires Python ≥ 3.14 (setup.cfg) — on 3.13 the package does not import at all, unrelated to this branch.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the status-model decoding pipeline to preserve the device’s “unknown” sentinel (0) for a specific set of on/off status flags by surfacing it as None (instead of collapsing it to False), aligning the library’s behavior with Navien’s own client decoding.

Changes:

  • Added a tri-state converter (device_tristate_to_python) and a corresponding model annotation (DeviceTriState) to map 0 → None, 1 → False, 2 → True for selected status flags.
  • Updated DeviceStatus field types for the affected flags from bool to bool | None, and extended the CLI to render “Unknown” for tri-state output (partially).
  • Added targeted unit tests and documentation (including a new “Unknown Values” explanation page) plus changelog entries.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/test_models.py Adds tests asserting 0/1/2 tri-state behavior on selected DeviceStatus flags and that capability flags remain boolean.
tests/test_model_converters.py Adds unit tests for the new device_tristate_to_python converter.
src/nwp500/models/status.py Introduces DeviceTriState annotation and applies it to specific DeviceStatus flags.
src/nwp500/enums.py Extends OnOffFlag with UNKNOWN = 0 and documents the meaning.
src/nwp500/converters.py Adds device_tristate_to_python and documents the distinction between capability vs status flags.
src/nwp500/cli/presentation.py Adds _yes_no_unknown() and uses it for operation_busy display.
docs/explanation/unknown-values.rst Adds a detailed explanation page documenting where 0 is a sentinel vs a real value.
docs/explanation/index.rst Adds the new explanation page to the docs index.
CHANGELOG.rst Documents the breaking type change and related behavioral updates.

Comment thread docs/explanation/unknown-values.rst Outdated
----------------------

:class:`~nwp500.enums.OnOffFlag` carries the vendor's ``UNKNOWN = 0``
member, and nine status fields - exactly the set the app decodes through

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, fixed in 825cfdf.

Nine is the app's count — the set NaviLink decodes through MgppOnOFFFlag. The library types eight of them, because the ninth (drOverrideStatus) is exposed here as a raw int rather than a flag. The sentence conflated the two counts. Reworded so they cannot be read as contradicting:

The app decodes nine status fields through MgppOnOFFFlag; eight of them exist here as flags and are typed DeviceTriState […] The ninth is drOverrideStatus, which this library exposes as a raw int rather than a flag, so it is left alone.

The migration section said "Nine fields change type" and is now "Eight".

Comment thread src/nwp500/converters.py
try:
if int(value) == 0:
return None
except TypeError, ValueError:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not making this change — the syntax is valid here.

except TypeError, ValueError: without parentheses is PEP 758, which landed in Python 3.14. This project requires 3.14 (setup.cfg: python_requires = >=3.14), so it does not raise SyntaxError on any interpreter that can install the package:

$ python3.14 -c "import ast; ast.parse(open(f).read())"   # parses
$ python3.13 -c "import ast; ast.parse(open(f).read())"
SyntaxError: multiple exception types must be parenthesized

The full suite imports and passes on 3.14 (702 tests), and ruff 0.16.1 actively normalises the parenthesized form to this one — writing except (TypeError, ValueError): and running ruff format rewrites it back, so keeping parens would mean fighting the formatter that CI enforces.

Worth noting this line has now misled two readers, myself included — I flagged it as corruption before checking PEP 758. But the resolution is the version floor, not a code change.

Comment on lines +32 to +40
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"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and this was a real bug — thank you. Fixed in 825cfdf.

build_device_status_rows had a second call site I missed: anti_legionella_operation_busy under ANTI-LEGIONELLA still used the two-way ternary, so an unknown rendered as "No" — exactly the failure this branch exists to remove, reintroduced one screen over. I had grepped only the narrower pattern.

examples/advanced/combined_callbacks.py had the same shape ('On' if status.comp_use else 'Off') and is fixed too.

Added tests/test_cli_tristate_presentation.py so this stops recurring — it exercises all three states end-to-end through build_device_status_rows, plus a structural guard that fails if any tri-state field is rendered with a two-way ternary anywhere in presentation.py. Both were verified to fail when the bug is reintroduced and pass when it is not.

The other two rendered references are the CLI-free device_capabilities.py (that reads DeviceFeature.recirc_reservation_use, a CapabilityFlag, still bool) and examples that print the raw value, where None prints informatively as None.

Comment thread CHANGELOG.rst Outdated
eman added 3 commits August 3, 2026 11:01
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.
#119 landed on main after this branch was cut and touched the same
files. Resolutions:

CHANGELOG.rst - both sides opened an Unreleased section. Combined into
one, with a banner covering both breaking changes and the Changed /
Added / Removed / Fixed subsections merged in that order.

docs/explanation/index.rst - both sides added a page to the toctree.
Kept both, tank-energy then unknown-values. Restored the cross-reference
from unknown-values to tank-energy that was dropped when the target did
not yet exist on main.

tests/test_models.py - both sides appended a test class at the end of the
file. Kept both: TestTriStateFlags and #119's TestEnergyFields /
TestUsableEnergy.

converters.py, models/status.py, cli/presentation.py and
test_model_converters.py auto-merged, and the results were checked rather
than assumed: energy_count_to_wh and device_tristate_to_python coexist,
as do the EnergyCountToWh and DeviceTriState annotations.

Also fixes a misplacement of my own from the first commit, surfaced while
reading the merged file. TestDeviceTriStateConverter had been inserted
into the middle of TestDeviceBoolConverter, orphaning six
device_bool_to_python tests into the tri-state class - including
test_invalid_value_zero, which asserts 0 is False and read as
contradicting the tri-state behaviour it was now filed under. The tests
always passed, since they name their converter explicitly; the class they
sat in was wrong. Moved the tri-state class below them.

Verified on the merged tree: 708 pass, ruff check and format clean, mypy
clean across 54 files, docs build with no new warnings and the
tank-energy cross-reference resolving. Spot-checked one DeviceStatus
carrying both changes - full_recovery_energy 6320.0 Wh alongside
comp_use None from a raw 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/<file> 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.
@eman
eman force-pushed the feat/unknown-flag-support branch from a52443c to d6c95fd Compare August 3, 2026 18:20
@eman
eman merged commit 595da5f into main Aug 3, 2026
7 checks passed
@eman
eman deleted the feat/unknown-flag-support branch August 3, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants