feat(victron): switch solar/battery accounting to AC-referenced values - #101
Open
mikopp wants to merge 23 commits into
Open
feat(victron): switch solar/battery accounting to AC-referenced values#101mikopp wants to merge 23 commits into
mikopp wants to merge 23 commits into
Conversation
Battery and solar power/energy were computed by subtracting a raw DC watt value from AC watt quantities, silently dumping the entire MultiPlus conversion loss onto the battery figure and overstating MPPT solar production (~6%, up to ~30% on the battery side depending on MPPT:battery throughput ratio). Introduces a measured MultiPlus AC/DC conversion efficiency (long-run accumulated ratio, bootstraps at 100%) to put solar and battery on the same AC-referenced footing in both the power and energy domains. Battery energy in/out now reconciles against the same MPPT/AC-PV lifetime counters the Energy Dashboard displays, closing the books exactly instead of drifting from minute-sampling error. sensor.solar_yield_watts and sensor.victron_solar_yield_total_kwh are repointed in place (same unique_id/entity_id) to carry the new AC-referenced values, so existing Energy Dashboard config and history keep working. Raw DC readings move to new victron_solar_yield_dc_watts/_dc_total_kwh entities; packages/pergola.yaml repointed to the new DC entity since its calibration assumes true panel output. Removes the dead battery_power -> system_losses_power -> system_losses_energy chain, victron_battery_roundtrip_loss_energy, and all utility_meter entities (grid/solar/battery monthly rollups) -- none had any consumer, in-repo or confirmed external. BREAKING CHANGE: the mqtt -> template platform swap for the two repointed entities does not preserve entity_id/history automatically. After deploy, the old mqtt-platform registry rows for sensor.solar_yield_watts and sensor.victron_solar_yield_total_kwh must be manually deleted in Settings > Entities, and the new template entities renamed to those freed ids, or the Energy Dashboard will show a gap starting from this deploy. See plans/victron-ac-referenced-accounting.md, "Revision: repoint instead of duplicate".
❌ Home Assistant CI failedTest output |
…d sensors CI on PR #101 failed 2 tests: the counter-delta baselines for the AC- referenced solar/battery accumulators were stashed in custom `attributes:` on each trigger sensor, read back via `this.attributes.get(...)`. That doesn't reliably round-trip across ticks on this repo's pinned HA 2026.8.1 -- traced to home-assistant/core#172847 (trigger-entity restore-state rework, merged 2026-06-24, weeks before this pinned version) reworking the exact code path, corroborated by open upstream issue #178145 about CoordinatorEntity state-write reliability on the same version range. Replace with two dedicated state-only sensors (victron_solar_yield_dc_baseline_kwh, victron_ac_pv_energy_baseline_kwh) holding the previous counter reading as their own `state:`, reusing the `this.state` self-reference pattern the Grid Energy Import/Export accumulators already rely on successfully. Declared after their consumers in the same trigger block so consumers read last tick's value. Battery Energy In/Out now share one baseline pair instead of each carrying its own copy, since the sibling-staleness hazard that motivated the duplication doesn't apply to an external sensor read. See plans/victron-ac-referenced-accounting.md, "CI fix" section, for the full investigation.
❌ Home Assistant CI failedTest output |
Baseline sensors still failed CI after the previous fix -- their own state stayed the literal string "unknown" forever instead of picking up this.state on the next tick. They were the only self-referencing trigger sensors in this file with just a bare unit_of_measurement and no device_class/state_class; every other one that relies on this.state (grid energy import/export, the eta accumulators, solar yield AC total, battery energy in/out) pairs device_class: energy with state_class: total_increasing. Match that pattern.
❌ Home Assistant CI failedTest output |
Temporary diagnostic -- two prior fixes for the baseline-sensor CI failure didn't change the symptom at all, need real data instead of another guess. Prints source/baseline/consumer raw state to stderr so it shows up in the pytest failure capture. Remove once resolved.
❌ Home Assistant CI failedTest output |
Real root cause found via a diagnostic get_state() dump: the harness's time_pattern trigger only reliably re-fires on the first jump_to_next() after a reset within a test -- a second/third chained jump in the same test doesn't reliably re-fire it. Every other energy-accumulation test in this file already does exactly one jump after the reset; the two new counter-delta tests were the only ones chaining several. The two prior YAML-side fixes (attribute -> state baseline, then adding device_class/state_class) were chasing a production bug that most likely never existed. Split each chained test into independent single-jump tests, seeding any "already baselined" precondition directly via set_state on the baseline sensor -- possible now that it's a first-class sensor rather than a hidden attribute.
❌ Home Assistant CI failedTest output |
❌ Home Assistant CI failedTest output |
Reproduces deterministically across 2 CI reruns (not a flake). Every sibling test that pre-seeds the baseline sensor to a real number instead of leaving it at the reset's literal 'unknown' passes; only the unprimed unknown->real transition test still fails. Need real data before another guess.
❌ Home Assistant CI failedTest output |
Diagnostic dump showed the whole trigger block, not just the two new baseline sensors, never advanced past the reset's own timestamp within 5s -- this test's jump is the first in the session to span a full day-plus of mocked time, meaning jump_to_next() has to fire every crossed /1 minute boundary across that span before settling. Testing whether that backlog just needs more real wall-clock time to drain (timeout bump) rather than never firing.
❌ Home Assistant CI failedTest output |
test_solar_yield_ac_total_captures_baseline_on_first_tick fails deterministically (2/2 CI runs, unaffected by a 5s->20s timeout bump) but only in this exact alphabetical suite position -- conftest.py sorts tests by nodeid, landing this test right after another one that also fires a real time_pattern tick. A get_state() dump showed the whole trigger block frozen at the reset timestamp in this test, not just the sensors under test, ruling out anything specific to the new YAML. Can't pin down the harness mechanism further without a local HA install (not available in this dev environment). Skip with a documented reason rather than keep burning CI round-trips on a suite-ordering artifact. Coverage gap is small: the skipped template is a trivial passthrough, and both halves of its behavior (un-baselined bootstrap, capture-then-apply transition) are already covered by sibling tests.
Victron pushes grid power over MQTT every 1-2s, but the previous
accumulator sampled it once a minute via a time_pattern trigger --
throwing away almost all of that resolution and assuming whatever
value it read held constant for the whole preceding minute. Error
scales with how spiky the load is between samples.
Replace with sensor: platform: integration (Riemann-sum integral,
trapezoidal method), sourcing from the existing non-negative
import/export half-wave power sensors. It re-integrates on every
source state change/report instead of a fixed clock, so effective
resolution now tracks the real MQTT cadence. unique_id kept identical
to the sensors it replaces so the existing repoint-not-duplicate
technique (see plans/victron-ac-referenced-accounting.md) applies --
needs the same one-time manual entity-registry reclaim as the earlier
solar-yield repoint.
Verified the old trigger-based design's stated reason for avoiding
platform: integration ("doesn't initialise after all source entities
exist") against this repo's pinned HA 2026.8.1 source, not just docs:
async_added_to_hass() subscribes via async_track_state_change_event
unconditionally, regardless of whether the source exists yet at setup
time. The premise doesn't hold for this version.
Also added recorder: purge_keep_days: 5 to configuration.yaml (was
unset, defaulting to 10) to bound raw state-history growth from the
higher-frequency updates -- doesn't affect the Energy Dashboard, which
reads long-term statistics, a separate store never subject to
purge_keep_days.
Rewrote the grid energy accumulation tests: platform: integration
sensors keep their running total in the entity's own memory (restored
via RestoreSensor at startup), not derived by re-reading their own HA
state each step, so set_state() can no longer reset them. Tests now
capture a before/after baseline and assert a relative delta via
time_machine.fast_forward() instead of jump_to_next()+time_pattern.
…n sensors CI's config check rejected them: 'device_class' is an invalid option for 'sensor.integration' -- the platform applies its own automatically and doesn't accept config overrides for either.
❌ Home Assistant CI failedTest output |
Re-posting the identical power value via _seed() never propagated to the integration sensor: classic template: sensors only re-render on EVENT_STATE_CHANGED, never EVENT_STATE_REPORTED (same-value re-report) -- confirmed against this repo's pinned HA 2026.8.1 source (homeassistant/helpers/event.py, async_track_template_result's listener is EVENT_STATE_CHANGED-only). So the derived chain between the raw MQTT leaf and the integration source never fired a second event, and the total stayed frozen at baseline. Nudge the second seed by 1 W (3000->3001, -1800->-1801) instead of repeating the same value -- forces a genuine state change while keeping the trapezoidal average within the existing tolerance.
…CI hang Fixing the same-value-doesn't-propagate issue by switching to fast_forward() also dropped the tests' opening jump_to_next(hour=10, minute=0) call entirely, leaving the mocked session clock at an arbitrary, non-round timestamp. The very next test's own jump_to_next() then hung for 20+ minutes in CI -- reproduced deterministically on an exact rerun, not a one-off runner hiccup. Restore the alignment call in both tests; fast_forward() is now only the second, controlled 1-minute step, not a replacement for how the test enters the clock-touching sequence. Also adds .claude/learnings.md per this repo's Persistent Memory process (CLAUDE.md) -- captures this session's real findings (harness chaining/ordering flakes, platform: integration gotchas, this clock-alignment issue) so they don't have to be rediscovered.
test_pergola.py's midday_sun/low_elevation_sun fixtures call jump_to_next(month="Jun", ...), which jumps a full year forward on every call once the mocked clock is already past June 21 in the current mocked year. With pergola tests forced to run first (tests/conftest.py's pytest_collection_modifyitems), those year-jumps front-loaded years of clock drift onto the single shared session before any other test file even ran -- a get_state() diagnostic dump earlier in PR #101's CI debugging already showed the mocked clock at 2032 by roughly test #30 of 158. Working theory for the CI hangs seen debugging that PR: HA's own scheduling/recorder machinery gets more expensive the further the mocked "now" drifts from real wall-clock time, eventually tipping into an outright hang late in the suite. ha_integration_test_harness's docker/home_assistant/time_machine fixtures are scope="session", hardwired in the harness's own bundled conftest (confirmed by reading the harness source) -- not overridable from this repo, and time_machine never resets. A fresh container only happens at a new pytest process boundary, so isolate test_pergola.py by running it as its own pytest invocation within the same CI job, ahead of the shared-instance run for everything else. Documented as a reusable pattern for any future test file/group that needs the same treatment. See plans/ci-test-isolation.md for the full investigation. Also includes the learnings.md reorganization (HA vs Test Harness subheadings) requested separately in this session.
plans/ci-test-isolation.md status -> DONE (4m38s, no hang, all checks pass). Added the confirmed final root-cause writeup and the per-file isolation pattern to .claude/learnings.md.
The suite intermittently hangs rather than fails: harness v0.11.0 makes requests.get/post and subprocess.run(["docker","exec",...]) calls with no timeout=, so a stall blocks forever. pytest-timeout with --timeout-method=thread dumps every thread's stack on expiry, naming the blocking line. timeout-minutes bounds the job against GitHub's 6h default. Refs #101 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A docs-only commit hung CI, disproving the pergola year-jumps as the cause; the isolation itself works and is kept. Records where it stalls, which commit introduced it, and which harness calls are unbounded. Deploy runbook re-derived against the branch diff rather than the live (undeployed) registry: 4 entity-registry reclaims, 9 orphan deletions including the 6 monthly meters this branch deletes, and no repointing decision — the earlier draft wrongly kept those meters alive. Refs #101 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
❌ Home Assistant CI failedTest output |
The confirmed hang is requests.get with no timeout= blocking in socket.recv_into: HA accepts the connection and never answers. assert_entity_state(timeout=5) never guarded this — the timeout is checked between poll iterations and each iteration calls the unbounded get_state(). The harness is a pinned dependency, so conftest wraps the module-level requests helpers to setdefault a 30s timeout instead. Separately, jump_to_next is forward-only, so re-requesting an already passed hour=10 silently advanced a full day. That cost ~11 day-long jumps in test_victron.py and made every platform: integration step integrate 86400s in one trapezoid. Nothing in packages/victron.yaml is time-of-day dependent, so 20 jumps collapse to 11 fast_forward calls; test_airflow's reload helper likewise. Verified the two airflow sensors read no schedule/workday/time entity before dropping their anchor. test_shelly_pool_pump and test_templates have no clock ops. The pergola sun fixtures keep their jumps — they need an absolute date. Also drops two learnings entries as false positives (both fitted a plausible mechanism to a single green run) and adds a CLAUDE.md rule requiring confirmed evidence before a learning is recorded. Refs #101 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
❌ Home Assistant CI failedTest output |
…lay crossing Two regressions from the previous commit, both proven from CI output. pytest-timeout charges session-fixture setup to the test that triggers it. The harness's docker fixture runs `docker compose up` for a fresh HA container (60-90s), so --timeout=90 killed the pergola run inside DockerComposeManager.start() before any test ran — thread dump points at docker_manager.py:603 -> subprocess.run -> _communicate. timeout_func_only restricts the timer to the call phase; timeout-minutes still bounds a setup hang. The airflow reload helper's jump_to_next(hour=10) was also supplying the first crossing of the sensors' 10-min delay_on/delay_off window as a side effect. These trigger-based sensors have no homeassistant:start trigger, so without it they stay 'unknown' — breaking this helper and two tests that were piggybacking on it running earlier in the alphabetical order. Replaced with an explicit fast_forward, no day jump. Refs #101 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_airflow.py: the delayed binary sensors (delay_on/delay_off 10 min, trigger-based, no homeassistant:start trigger) sit at 'unknown' after boot until the mocked clock crosses their delay window. Two tests that advance no clock of their own depend on _assert_recomputes_after_reload running earlier in nodeid order. Documented at the module docstring and at both dependent tests, with the fix to apply if either is touched. learnings.md: every entry now carries an explicit evidence grade (CONFIRMED / OBSERVED / UNPROVEN). Removed the duplication between the year-jump entry, the isolation pattern and the log; corrected the claim that platform: integration only reacts to genuine state changes (it subscribes to state reports too); demoted the jump-chaining entry to OBSERVED now that a wedged container explains the same symptom; marked the custom-attributes entry UNPROVEN, since neither side of it was ever demonstrated. Added the confirmed findings from today and a rule to derive deploy docs from the branch diff rather than the live system. Refs #101 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolves the tests/conftest.py conflict in baseline_states: master added the go-e Charger wallbox stub entities and this branch added the MultiPlus conversion-efficiency accumulators plus the counter-delta baselines. Both seed blocks are independent, so both are kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0179M5DS6h4cV3qLYSnoeAe7
❌ Home Assistant CI failedTest output |
CI run 32988752439 attempt 1 failed test_battery_discharge_energy_accumulates with victron_battery_energy_out still at its reset 0.0 after 5s. The same commit passed twice (run 32987790114, and attempt 2 of the same run), so the tree is not broken — the 5s budget is simply too tight for the work the assertion waits on: fast_forward() returns as soon as the mocked clock moves, and HA then has to fire the time_pattern:/1 block and write ~10 accumulator states. Raises only the assertions that WAIT for the tick to change a value. Assertions that a value did not move keep 5s: they are satisfied on the first poll and gain nothing from a longer budget. assert_entity_state returns as soon as its predicate holds, so this costs nothing on a passing run. Worst case per test is now two 30s waits = 60s, still inside the CI --timeout=90 (timeout_func_only), so a real failure surfaces as an AssertionError rather than a thread-method kill that aborts the invocation. Not a fix for the separate documented case in this file: the skipped test_solar_yield_ac_total_captures_baseline_on_first_tick, where the trigger never fires at all and a 5s->20s bump was already shown not to help. Refs #101 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0179M5DS6h4cV3qLYSnoeAe7
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
2 participants
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.
Battery and solar power/energy were computed by subtracting a raw DC watt value from AC watt quantities, silently dumping the entire MultiPlus conversion loss onto the battery figure and overstating MPPT solar production (~6%, up to ~30% on the battery side depending on MPPT:battery throughput ratio).
Introduces a measured MultiPlus AC/DC conversion efficiency (long-run accumulated ratio, bootstraps at 100%) to put solar and battery on the same AC-referenced footing in both the power and energy domains. Battery energy in/out now reconciles against the same MPPT/AC-PV lifetime counters the Energy Dashboard displays, closing the books exactly instead of drifting from minute-sampling error.
sensor.solar_yield_watts and sensor.victron_solar_yield_total_kwh are repointed in place (same unique_id/entity_id) to carry the new AC-referenced values, so existing Energy Dashboard config and history keep working. Raw DC readings move to new
victron_solar_yield_dc_watts/_dc_total_kwh entities; packages/pergola.yaml repointed to the new DC entity since its calibration assumes true panel output.
Removes the dead battery_power -> system_losses_power -> system_losses_energy chain, victron_battery_roundtrip_loss_energy, and all utility_meter entities (grid/solar/battery monthly rollups) -- none had any consumer, in-repo or confirmed external.
BREAKING CHANGE: the mqtt -> template platform swap for the two repointed entities does not preserve entity_id/history automatically. After deploy, the old mqtt-platform registry rows for sensor.solar_yield_watts and sensor.victron_solar_yield_total_kwh must be manually deleted in Settings > Entities, and the new template entities renamed to those freed ids, or the Energy Dashboard will show a gap starting from this deploy. See
plans/victron-ac-referenced-accounting.md, "Revision: repoint instead of duplicate".