Correct tank energy scale (2.5x) and reversed field semantics - #119
Conversation
There was a problem hiding this comment.
Pull request overview
This PR corrects tank energy telemetry interpretation in DeviceStatus (unit scale and reversed semantics), updates CLI/examples/docs accordingly, and introduces a new nwp500.energy module with pure, first-principles hot-water availability calculations.
Changes:
- Replace the old ×10 “energy capacity” conversion with a fixed ~4 Wh/count quantum and rename the two energy fields to reflect their true meaning (with deprecation aliases).
- Add
nwp500.energywithTankModel+ helper functions for stored/usable energy, deliverable volume, and setpoint targeting. - Update CLI labels, examples, and protocol/Python API docs; add extensive unit tests.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_models.py | Adds deprecation/alias behavior tests for renamed energy fields. |
| tests/test_model_converters.py | Updates converter tests for the new energy quantum conversion. |
| tests/test_energy.py | New test suite validating nwp500.energy calculations and invariants. |
| tests/conftest.py | Updates fixture values to use raw device energy counts (not Wh). |
| src/nwp500/models/status.py | Renames energy fields, fixes conversion, improves descriptions, and adds deprecated properties with warnings. |
| src/nwp500/models/init.py | Re-exports the new annotated converter type alias. |
| src/nwp500/energy.py | New pure-functions module for tank energy and deliverable hot-water modeling. |
| src/nwp500/converters.py | Introduces WH_PER_ENERGY_COUNT and energy_count_to_wh, removing mul_10. |
| src/nwp500/cli/presentation.py | Updates CLI labels/field selection to the corrected energy fields. |
| src/nwp500/init.py | Exposes tank-energy helpers at the top-level package API. |
| examples/intermediate/periodic_requests.py | Updates example output to reflect the corrected semantics/name. |
| docs/reference/python_api/models.rst | Fixes Python API docs to match corrected field meaning + adds deprecation warning. |
| docs/reference/protocol/device_status.rst | Updates protocol reference text and conversion notes for energy fields and dhwTemperature. |
| docs/reference/protocol/data_conversions.rst | Corrects energy conversion/scaling documentation and usage guidance. |
| docs/project/history.rst | Corrects historical documentation statement about energy capacity being a percentage. |
| docs/how-to/track-energy.rst | Updates guidance/examples for the corrected energy deficit semantics and points users to nwp500.energy. |
| docs/how-to/manage-units.rst | Updates units guide for the renamed energy fields. |
| docs/explanation/tank-energy.rst | New deep-dive explanation + migration guidance. |
| docs/explanation/index.rst | Adds the new explanation page to the docs index. |
| CHANGELOG.rst | Documents the breaking semantics/scale corrections and the new energy module. |
Suppressed comments (2)
src/nwp500/init.py:202
energy_to_setpointis missing from__all__, so it won’t be included in star-imports and docs that rely on__all__to reflect the public API. If it’s intended to be a top-level export, it should be listed alongside the other tank-energy helpers.
# Tank energy calculations
"TankModel",
"tank_mean_temperature",
"stored_energy",
"usable_energy",
"deliverable_volume",
"draw_energy",
"setpoint_for_energy",
docs/how-to/track-energy.rst:154
- Same line-block formatting issue for the
full_recovery_energydescription: continuation lines should keep the leading|so the block renders correctly.
| **Description:** Cost of a full recovery to the *current setpoint*,
measured from the device reference temperature of 104.9 degF. It moves
with the setpoint, so it is not a fixed tank capacity.
| from nwp500.energy import ( | ||
| TankModel, | ||
| deliverable_volume, | ||
| draw_energy, | ||
| setpoint_for_energy, | ||
| stored_energy, | ||
| tank_mean_temperature, | ||
| usable_energy, | ||
| ) |
| def test_observed_full_recovery(self): | ||
| """A real reading: 1580 counts at a 145.4 degF setpoint. | ||
|
|
||
| The tank is 65 gallons and the device's reference temperature is | ||
| dhw_temperature_min (104.9 degF), so a full recovery spans 22.5 K: | ||
| 246.05 kg * 4.186 kJ/kg/K * 22.5 K = 23170 kJ = 6436 Wh. | ||
| """ | ||
| assert energy_count_to_wh(1580) == pytest.approx(6320.0) |
There was a problem hiding this comment.
Valid — fixed in b4b0f0e.
The docstring computed 6436 Wh from a nominal 65 gallons while the assertion expected 6320 Wh, so it read as asserting a value its own reasoning contradicted.
6320 Wh is 1580 counts × 4 Wh, and implies 241.5 kg of water over the 22.5 K recovery span — just under the 246.05 kg a nominal 65 gallons would weigh, which is expected since a "65 gallon" tank does not hold 65 gallons. The docstring now derives that and explains why anchoring on nominal volume is the wrong move: it would give a quantum of 4.07, which is not a round number, unlike every other conversion in this protocol.
| | **Description:** Energy the device still has to add to reach the | ||
| setpoint. It *falls* as the tank heats and reaches zero when charged. | ||
| Despite the protocol name, it is the inverse of available energy. |
There was a problem hiding this comment.
False positive — no change made.
Indented continuation lines in a reStructuredText line block are valid: the continuation is folded into the preceding line. Parsed with docutils, this block produces one line_block node containing four line nodes and zero system messages.
It is also the established style of this file and predates the PR — the same pattern is on main:
| **Field:** ``current_inst_power``
| **Type:** ``float``
| **Units:** Watts (W)
| **Description:** Total instantaneous power consumption of the entire
unit, including heat pump compressor and electric heating elements.Prefixing the continuation lines with | would change the rendering, splitting one logical line into two.
The two tank energy fields were wrong in two independent ways, and the published docs described them three mutually incompatible ways. Unit scale. totalEnergyCapacity/availableEnergyCapacity were multiplied by 10 on the assumption the device reports 10 Wh units. It reports about 4 Wh per count. Measured across 183 heating recoveries by comparing the device's reported change against the tank's sensible-heat gain from its thermistors, which is independent of heat pump efficiency: 4.11 Wh/count, p10 3.47, p90 4.45. The library now uses 4.0. The old scale is refuted without any physical assumption: integrating current_inst_power over the same recoveries, the reported energy gain divided by electrical input gives a COP of 7.02, which no heat pump can achieve. At 4 Wh/count it is 2.89, an ordinary figure. That argument needs no tank volume, specific heat or mixing model. Reported tank energy is now 2.5x smaller. Series logged from earlier versions need rescaling by 0.4 to compare. Semantics. availableEnergyCapacity is not available energy; it is the energy still needed to reach the setpoint. It falls as the tank heats and reaches zero when fully charged, so consumers treating it as stored energy had the signal inverted (regression against mean tank temperature: negative slope, R-squared 0.93, zero crossing at the setpoint). totalEnergyCapacity is not a fixed tank size but the cost of a full recovery to the current setpoint, measured from the device minimum setpoint; it moves ~143 Wh per 0.5 degC of setpoint change. total_energy_capacity -> full_recovery_energy available_energy_capacity -> energy_to_setpoint Old names are removed rather than aliased, per the project's backward compatibility policy. A missed rename fails with AttributeError instead of silently returning a value 2.5x too large. Wire field names are unchanged. converters.mul_10 is removed. Docs. The published docs claimed Wh with no conversion (protocol reference), Wh with a x10 scale (the code), and a 0-100 percentage (track-energy, models, history) - the last was never true of any version. Also corrects four field names in track-energy.rst that do not exist on DeviceStatus, and documents that dhwTemperature is measured inside the tank rather than in the outlet pipe despite its name. Note the 4 Wh quantum was calibrated on a single 65-gallon unit and is applied to all volume codes. That is correct if the quantum is a fixed firmware constant, which is the natural design, but is unverified for the 50 and 80 gallon variants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9709743 to
fd8cbd7
Compare
The 4 Wh quantum was justified by dividing per-recovery thermistor heat gains by the device's reported change, which gave 4.11 Wh/count with a p10-p90 spread of 3.47-4.45. That route depends on (upper + lower) / 2 approximating the true mean tank temperature, which is the weakest assumption available. totalEnergyCapacity is a whole-tank quantity, so regressing it against the setpoint measures the same thing with no stratification assumption at all: 70.25 raw counts per Kelvin, R-squared 0.99999 over ten setpoints. Converting that to Watt-hours still needs a water mass, so rather than assume nominal volume and derive an odd quantum, the docs now assume the quantum is round - as every other conversion in this protocol is - and show that 4 Wh/count is the only candidate implying a water volume below the nameplate. Also promotes the reference-temperature result, which was previously derived through the quantum and so was partly circular. Extrapolating the setpoint regression to zero gives 104.92 degF against the device's dhwTemperatureMin of 104.9 degF, using only the device's own two numbers. That is the strongest evidence on the page and was buried. Demotes the COP cross-check to what it actually establishes: it rules out the pre-10.0 scale without any physical assumption, but cannot identify the quantum. No functional change - WH_PER_ENERGY_COUNT stays 4.0, which all three methods support. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The setpoint regression was presented as R-squared 0.99999 without saying it was a modal fit. The field is not a function of the setpoint alone: at a fixed setpoint it takes one of two values, flipping between them several times a day, separated by a constant 2 degC of setpoint. Fitted separately the branches are 68%/32% and parallel: primary 390.56 units/degF, zero at 104.95 degF secondary 389.81 units/degF, zero at 108.48 degF This cuts both ways. The quantum gets stronger - two independent populations agree on the slope to within 0.2%, which is better evidence than either alone. The reference temperature gets weaker: only the primary branch lands on dhwTemperatureMin, and what selects between the branches is unknown. hpUpperOnTemperatureSetting correlates with the choice, but on 22 paired samples that is a lead, not a finding. Adds a warning against deriving tank heat capacity from a live full_recovery_energy reading, which lands on the wrong branch about a third of the time and is then off by 9%. Also states plainly in the summary that both fields are measured from the setpoint and so describe potential rather than content - neither is a state of charge. No functional change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both device energy fields are measured from the setpoint, so neither is
a state of charge: move the setpoint and both change while the water in
the tank does not. Subtracting them cancels the setpoint.
full_recovery_energy - energy_to_setpoint
= k * (setpoint - reference) - k * (setpoint - tank_temp)
= k * (tank_temp - reference)
The reference is dhw_temperature_min, 104.9 degF. A shower runs about
105 degF, so water below the reference still holds heat but not heat you
can wash with, and excluding it is the behaviour wanted. A mixing valve
does not move this floor - it caps how hot water can be delivered, and
below its setting it passes through, so water stays usable down to the
temperature actually wanted at the tap.
Robust despite full_recovery_energy being bimodal (it takes one of two
values 2 degC apart at a fixed setpoint): both fields shift together, so
the difference is unaffected. Against the tank thermistors over 12275
samples the implied tank temperature agrees to a standard deviation of
0.57 degF, with 97.5% of samples inside 2 degF.
Rendered by the CLI as "Usable Energy". Verified against a live device:
5284 Wh at a 139 degF tank, consistent with the 156 Wh/degF heat
capacity of the 65-gallon unit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The docstring computed 6436 Wh from a nominal 65 gallons while the assertion expected 6320 Wh, so it read as asserting a value its own reasoning contradicted. 6320 Wh is 1580 counts x 4 Wh, and implies 241.5 kg of water over the 22.5 K recovery span - just under the 246.05 kg nominal 65 gallons would weigh, which is expected. The docstring now shows that and explains why the nominal-volume figure is the wrong one to anchor on. Reported by Copilot review on #119. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Re: Stale. This review ran against an earlier revision. The |
The tables in tank-energy.rst listed totalEnergyCapacity and
availableEnergyCapacity at ten times their raw wire values, under column
headings naming the raw protocol fields. The series had been logged
through the pre-fix library, which multiplied by 10, and that scaling was
never undone when the numbers were tabulated.
The prose beside the tables was already in true raw counts - "70.25 raw
counts per Kelvin", "35 counts, about 140 Wh" - so the page contradicted
itself by exactly the factor it exists to correct. A reader fitting the
printed table gets 700 counts/K, then 4 Wh/count implies 2400 L of water,
and concludes the derivation is wrong.
Tables are now in raw counts. 1580 counts at a 145.4 degF setpoint
matches tests/conftest.py and the 6320 Wh the CLI reports, so the table
is self-verifying against the fixtures. Branch slopes are restated as
39.06 and 38.98 counts/degF and the separation as 140-141 counts. The
deficit table is labelled as binned means, since those values are not
whole counts, and gains a check against the 156 Wh/degF heat capacity.
No numeric conclusion changes.
Also corrected while checking:
- hpUpperOnTemperatureSetting -> hpUpperOnTempSetting, the real
protocol name
- 143 Wh per 0.5 degC -> 140 Wh, which is what the documented slope
gives at 4 Wh/count, in the changelog, the model field description
and the protocol reference
- "overstated by 2.43x" -> 2.5x where the sentence is about what the
library reported; 2.43 is 10/4.11, the measured estimate, not the
10/4.0 the code actually applied
- the data_conversions note led with the per-recovery method that the
explanation page had already demoted; it now leads with the setpoint
slope
Adds a section recording that Navien's own NaviLink app reads neither
field. Decompiling the current release - 2.03.00, versionCode 141,
March 2026 - gives 8101 sources and neither name appears in any of them,
nor in the raw dex string pool, while dhwChargePer and
tankUpperTemperature do appear as controls. The app requests no field
subset, so the device sends both and the app discards them. There is
therefore no vendor label, scale or formula to check this page against,
which is worth stating explicitly since "just look at the app" is the
obvious next question.
The app does corroborate three surrounding facts, now cited inline: its
volume table gives the 246.0 L nominal the quantum candidates are judged
against, it reads dhwTemperatureMin as half-degrees C confirming the
104.9 degF reference, and it labels dhwTemperature "DHW Temp." beside the
tank thermistors while giving dischargeTemperature its own row.
#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.
Summary
Two independent bugs in the tank energy fields, plus docs that described them three mutually incompatible ways.
The functional change is one line:
10.0→4.0. Everything else is the rename it forces, the doc corrections, and tests.On a 65-gallon unit the CLI goes from
Total Capacity 15800.0 Wh/Available Capacity 11660.0 WhtoFull Recovery 6320.0 Wh/Energy to Setpoint 4664.0 Wh.1. Unit scale
totalEnergyCapacity/availableEnergyCapacitywere multiplied by 10, assuming the device reports 10 Wh units. It reports about 4 Wh per count.totalEnergyCapacityis a whole-tank quantity, so regressing it against the setpoint measures the quantum with no stratification assumption. The modal value at each setpoint forms an arithmetic sequence. These are raw wire counts:totalEnergyCapacitytotalEnergyCapacityR² = 0.99999, slope 70.25 raw counts per Kelvin of whole-tank rise. Endpoints alone: (1685 − 1369) / 4.5 K = 70.2. The 1580 at 145.4 °F is the value in
tests/conftest.py, and 1580 × 4 = the 6320 Wh above.Converting that to Watt-hours needs a water mass, and a "65 gallon" tank does not hold 65 gallons. Rather than assume nominal volume and derive an odd-looking quantum, assume the quantum is round — as every other conversion in this protocol is (half-degrees, tenths) — and check which implies a sensible volume:
Two further checks agree. 183 individual heating recoveries give 4.11 Wh/count by a noisier route (p10 3.47, p90 4.45 — it depends on
(upper+lower)/2approximating mean tank temperature). And integratingcurrent_inst_powerover those recoveries:That last argument needs no tank volume, specific heat or mixing model at all — it rules out the old scale independently of everything else here, though it cannot identify the quantum.
The reference temperature is the strongest result
Extrapolating the setpoint regression to
total = 0gives 104.92 °F, against the device'sdhw_temperature_minof 104.9 °F. That uses only the device's own two numbers — no mass, no specific heat, no thermistors, no assumption about the quantum — and establishesfull_recovery_energy = k · (setpoint − dhw_temperature_min)as fact rather than inference.Reported tank energy is now 2.5x smaller. Historical series need rescaling by 0.4 to compare.
2. Field semantics
availableEnergyCapacityis not available energy — it is the energy still needed to reach the setpoint. It falls as the tank heats and hits zero when fully charged, so anything treating it as stored energy had the signal inverted. Regressed against mean tank temperature: negative slope, R² = 0.93, zero crossing at the setpoint.totalEnergyCapacityis not a fixed tank size either. It is the cost of a full recovery to the current setpoint, measured from the device minimum setpoint (104.9 °F), and moves ~140 Wh per 0.5 °C of setpoint change. Seven months of history shows 16 distinct values.total_energy_capacityfull_recovery_energyavailable_energy_capacityenergy_to_setpointRemoved rather than aliased, per the backward compatibility policy. A missed rename fails with
AttributeErrorinstead of silently returning a value 2.5x too small — the better failure mode, since the scale correction lands in the same change. Wire field names are unchanged.3. Documentation
Three incompatible published descriptions: Wh with no conversion (protocol reference), Wh with a ×10 scale (the code), and a 0–100 percentage (
track-energy.rst,models.rst,history.rst). The percentage claim was never true of any version.Also fixes four field names in
track-energy.rstthat do not exist onDeviceStatus(dhw_tank_upper_temp,dhw_tank_lower_temp,comp_temp,dhw_heatex_out_temp), and documents thatdhwTemperatureis measured inside the tank rather than in the outlet pipe despite its name — it trackstankUpperTemperatureto within one 0.5 °C step, and the device has no sensor downstream of itself.New
docs/explanation/tank-energy.rstrecords the evidence and a migration table. It is the largest file here; the scale change makes every reported number 2.5x smaller, so the justification seemed worth keeping somewhere.The vendor app reads neither field
Navien's own NaviLink app was decompiled to look for confirmation. It never touches either field.
The current release — 2.03.00, versionCode 141, published March 2026 — yields 8,101 Java sources, and neither
totalEnergyCapacitynoravailableEnergyCapacityappears in any of them, nor in the raw dex string pool (so this is not obfuscation). The app's status modelKDResponseMgppStatus.Statusdeclares ~140 fields — includingdhwChargePer,tankUpperTemperature,tankLowerTemperature,currentInstPowerandmixingRate— and neither energy field is among them. It requests no field subset, so the device sends both and the app drops them on deserialization. Controls:dhwChargePerandtankUpperTemperaturedo appear in the dex strings, so the absence is a real result.So there is no vendor label, scale factor or formula to check any of this against — the analysis above is the only account of these two fields that exists. It also explains the misleading protocol names: nothing Navien ships ever has to act on them.
The app does corroborate three surrounding facts, now cited inline in the docs:
MgppStatusFragment.getVolume()mapsvolumeCode1/2/3 to 189.2 / 246.0 / 302.8 L, labelled "Volume"MgppControlFragment.makeTempMap()readsdhwTemperatureMin / 2.0fas °CdhwTemperature"DHW Temp." beside "Upper Temp."/"Lower Temp.", with a separate "Discharge Temp." row fordischargeTemperaturedhwTemperatureas an outlet readingCorrections since first review
tests/conftest.py. No numeric conclusion changed.hpUpperOnTemperatureSetting→hpUpperOnTempSetting, the real protocol name.data_conversionsnote led with the per-recovery method the explanation page had already demoted; it now leads with the setpoint slope.Reviewer notes
volumeCodebut says nothing about the firmware quantum.dhw_charge_perremains unreconciled. It agrees with neither corrected field (48-point mean gap over two weeks). Documented as an opaque vendor heuristic rather than guessed at. The app prints it unmodified as "DHW Charge", so it is device-computed and is the number users actually see.Not in this PR
A
nwp500.energymodule computing usable energy and deliverable volume from tank geometry is parked onfeat/nwp500-energy. It was originally included here and removed to keep this PR to the fix.Testing
tests/conftest.pycapacity fixtures were0.0; now carry real raw countsruff check src/ tests/andruff format --checkclean; mypy clean across 54 filestank-energy.rstrenders with no warnings and the new cross-reference resolvesenergy_count_to_wh(1580) == 6320.0, endpoint slope (1685 − 1369)/4.5 = 70.2 counts/K, zero crossing 140.0 − 1369/39.06 = 104.95 °F, 70.25 × 0.5 × 4 = 140 WhTwo environment notes, neither caused by this branch and both present on
main:setup.cfg). On 3.13 the whole package fails to import —mqtt_events.pyannotates dataclass fields withDeviceStatus, imported only underTYPE_CHECKING, and withoutfrom __future__ import annotationsthat is evaluated eagerly. 3.14's lazy annotations (PEP 649) make it fine. A stale 3.13.venvtherefore looks like a total breakage.ruff check .reports 15 findings, all inscripts/andsetup.py— the same newer-ruff sweep already applied toexamples/in b6af787. Untouched by this PR.