fix: stop fabricating TOU mode labels for VPP/period-list inverters (#415) - #424
Open
johanzander wants to merge 29 commits into
Open
fix: stop fabricating TOU mode labels for VPP/period-list inverters (#415)#424johanzander wants to merge 29 commits into
johanzander wants to merge 29 commits into
Conversation
Generalizes the #415 root cause (TOU mode vocabulary forced onto non-TOU control models) across all six inverter controllers instead of a Growatt-VPP-only patch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up from #415 design work: SolaxController never received the #355 SOLAR_EXPORT grid-first-hold or #413 LOAD_SUPPORT release fixes that SolaxModbusGrowattController(vpp) has. Out of scope for the display-only #415 fix, tracked here for future hardware validation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reading the actual source revealed SolaxController and SolaxModbusGrowattController(vpp) have genuinely diverged behavior (#355, #413 fixes are Growatt-only), not just duplicated code as the initial draft assumed. Updates the design to keep them as separate per-controller display methods behind a shared field shape, and corrects the get_all_tou_segments() claim (6 independent overrides, not one mergeable implementation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
get_detailed_period_groups() (core/bess/inverter_controller.py:635-760) is the actual data source behind issue #415's screenshot -- a third, previously-missed static INTENT_TO_MODE lookup on top of get_period_settings()/get_all_tou_segments(). Being a single un-overridden base-class method, fixing it is straightforward and now folded into the same _mode_display_fields() mechanism. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
) 15-task TDD plan implementing docs/superpowers/specs/2026-07-29-control-model-display-design.md: classifies all 6 InverterController subclasses by real hardware model (tou_register/vpp_power/period_list), adds a shared _mode_display_fields() helper wired into get_period_settings()/get_detailed_period_groups()/ get_all_tou_segments(), updates the API and three frontend components (InverterStatusDashboard, SystemStatusCard, PredictionAnalysisView), and adds regression tests throughout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds tou_register/vpp_power/period_list classification per design spec docs/superpowers/specs/2026-07-29-control-model-display-design.md, section 1. No behavior change yet — this is the foundation the display fix (get_period_settings/get_detailed_period_groups/get_all_tou_segments) branches on in the following tasks.
Own VPP display computation for SolaX, deliberately NOT shared with SolaxModbusGrowattController._intent_to_vpp() -- the two controllers' real hardware behavior has diverged (see design spec section 1 correction, and TODO.md's SolaX VPP gap note).
Single choke point for batt_mode/vpp_power_pct/vpp_remote_control field computation, branching on CONTROL_MODEL. Not yet wired into get_period_settings/get_detailed_period_groups/get_all_tou_segments -- next tasks.
Root-cause fix for issue #415: SOLAR_EXPORT on a VPP-controlled Growatt install no longer reports batt_mode=load_first when the hardware is actually doing a grid-first hold. Wires the CONTROL_MODEL-driven _mode_display_fields() helper (Task 3) into the one shared get_period_settings() implementation.
This is the actual data source behind issue #415's Schedule Overview screenshot (backend/api.py period_groups/tomorrow_period_groups), not get_all_tou_segments() as originally assumed -- see design spec section 1, 'Second correction'. Also renames the group field from 'mode' to 'batt_mode' for consistency with get_period_settings().
…es on control_mode VPP-mode segments now carry vpp_power_pct/vpp_remote_control instead of a fabricated batt_mode label. TOU mode unchanged.
Reflects SolaX's own real (self-use passthrough) SOLAR_EXPORT behavior instead of a fabricated batt_mode label -- see TODO.md gap note for why this differs from Growatt VPP's grid-first hold.
GrowattSphController, SolisModbusController, HuaweiController have no per-period mode or power concept -- their static 'load_first' stub (and Huawei's charge/discharge-sign approximation) is dropped entirely rather than replaced, per design spec section 1.
/api/inverter/status and /api/inverter/schedule now surface controlModel so the frontend can branch display logic instead of inferring TOU-vs-not from the platform string (InverterStatusDashboard.tsx's isTouBased bug).
Plain interval["batt_mode"] indexing (no default) would KeyError once Task 6-8 made batt_mode conditionally absent. Now branches on which fields the interval actually carries.
Replaces the isTouBased platform-string heuristic (blind to VPP-mode Growatt) with a proper three-way branch (tou_register/vpp_power/ period_list), adding a VPP Power column for vpp_power installs and dropping TOU-only columns entirely for both non-tou_register cases.
…ister The hard-throw on missing batteryMode now only applies when controlModel === 'tou_register'; the metric itself is omitted from the card (not shown as 'Unknown' or a stale value) for vpp_power/period_list installs. ControlModel moved to frontend/src/types.ts as a shared export; InverterStatusDashboard.tsx (Task 11) now imports it from there instead of defining its own local copy.
…label Previously a binary battery_first/else-'Load First' ternary that would mislabel any VPP interval. Now branches on whether the interval carries battMode or vppPowerPct/vppRemoteControl.
…nalysisView Task 13's review flagged that no test exercised the matchingA/hasChanges/ modeCell.changed path added by renderModeCell(), since both existing tests left growattScheduleB empty. Add a test with populated growattScheduleA and growattScheduleB sharing start/end times but differing vppPowerPct, asserting the "Changed" badge and yellow highlight appear on the Snapshot B entry. Verified the test fails if the VPP comparison logic in renderModeCell is reverted to the pre-fix behavior (no vppPowerPct/vppRemoteControl comparison), confirming it exercises the actual bug fix rather than being vacuously true.
…h style Wraps "Inverter schedule display no longer shows a fictional TOU mode label" in bold to match the convention used by all other entries in the ### Fixed section. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 5 (5695b88) renamed get_detailed_period_groups()'s 'mode' field to 'batt_mode' for tou_register controllers, but log_detailed_schedule() in GrowattMinController still read group['mode'], raising KeyError('mode') on every schedule update. This slipped past per-task review because the method is a pure logging/debug consumer never exercised by unit tests - it only surfaced during the plan's final full-suite (slow) quality gate, where it caused 22 integration test failures. Audited every other get_detailed_period_groups() consumer (solax_controller.py, solax_modbus_growatt_controller.py, backend/api.py) for the same class of bug; all other call sites already use the correct batt_mode/intent/vpp_power_pct keys. The remaining group["mode"] hits in growatt_min_controller.py (lines 246, 279, 499) belong to the unrelated _group_periods_by_mode() internal helper and are untouched.
- Remove unused pytest import from test_control_model_classification.py - Remove unnecessary noqa: N802 comment from solax_modbus_growatt_controller.py Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… the stack Fixes all four findings from the whole-branch final review of the control-model-display work (issue #415): 1. Inverter page's "Current Mode" metric and both /api/inverter/status and /api/inverter/schedule backend endpoints still defaulted battery_mode to "load_first" for vpp_power/period_list installs. Gate the frontend metric on controlModel (VPP power/remote-control state for vpp_power, omitted for period_list) and make schedule_data/mode_distribution/current_battery_mode conditional on what get_period_settings() actually returns, mirroring the period_groups fix already in place. 2. Removed the leftover "batt_mode": "load_first" stub from SolaxController/SolaxModbusGrowattController's empty-groups get_all_tou_segments() default (only kept for SolaxModbusGrowattController in its tou_register mode, which does have a real register). 3. Replaced the hasattr()-based vpp display dispatch in _mode_display_fields() with a unified _vpp_display_state() interface implemented by both vpp_power controllers, and made the CONTROL_MODEL fallback raise ValueError instead of silently returning {}. 4. Restored the real touIntervals hardware panel for period_list installs (SPH/Solis/Huawei) -- only vpp_power now shows the "no stored schedule" placeholder; the mode badge is branched per-interval instead. Also loosens e2e/tests/api-contracts.spec.ts's batteryMode assertions to be controlModel-conditional, since they previously enforced the fabrication being removed here. Backend: 1404 passed, 15 skipped (was 1398/15; +6 new tests covering findings 1 and 2). Frontend: 108 passed, 0 type errors, 0 lint errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Mode test (#415) CI (PR #424) failed api-contracts.spec.ts because /api/growatt/inverter_status returned controlModel: 'tou_register' with batteryMode undefined. Root cause: the endpoint derives batteryMode from InverterController.get_period_settings(), which raises ValueError("No strategic intents available") until the background startup optimization thread has produced a schedule. backend/api.py correctly catches that and omits the field rather than fabricating a value (the whole point of this PR) -- but the CI workflow's startup wait only polls /api/settings, which responds before startup_complete/schedule readiness, same race /api/dashboard already guards against with its "initializing" response. Reproduced live via podman-compose (docker-compose.ci.yml, SCENARIO= ci-normal-day): while /api/dashboard returned {"error":"initializing"}, /api/growatt/inverter_status logged "Failed to get current battery mode: No strategic intents available" and returned controlModel: 'tou_register' with batteryMode absent -- confirming the race is real, not a code bug. Relaxed the test to accept batteryMode being absent for tou_register when a schedule isn't computed yet, consistent with the existing "initializing" tolerance pattern used elsewhere in this same test file for /api/dashboard. No backend behavior changed -- the backend's no-fabrication contract from #415 stands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rol-model-display
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
1 participant
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.
Summary
Fixes #415: the Schedule Overview table (and other views) showed a fictional
load_first/battery_first/grid_firstTOU-register mode label for control models that don't have one. A VPP-controlled Growatt inverter'sSOLAR_EXPORTperiod displayed as "Load First" when the hardware was actually doing a grid-first-style hold.Rather than patching just the reported Growatt VPP case, this generalizes the fix across all 6
InverterControllersubclasses, since the same root cause (a staticINTENT_TO_MODElookup built for TOU-register hardware, applied unconditionally) affected native SolaX (VPP) and the three period-list controllers (Growatt SPH, Solis, Huawei) too — see the design spec for the full survey.CONTROL_MODELclassification (tou_register|vpp_power|period_list) reflecting each controller's real hardware model._mode_display_fields()helper, wired into the three places that were independently fabricating a mode label:get_period_settings(),get_detailed_period_groups()(the actual data source behind the reported bug), and each controller'sget_all_tou_segments().SolaxControllerandSolaxModbusGrowattController's VPP behavior is deliberately not unified — their real hardware behavior has diverged (SolaX never received the Has it lost sense of battery wear cost? #355/VPP mode: LOAD_SUPPORT forces fixed grid_first discharge rate instead of releasing control #413 Growatt VPP fixes) and unifying them would misrepresent or silently change real SolaX hardware behavior. Tracked as a separate follow-up inTODO.md.controlModel/vppPowerPct/vppRemoteControl; three frontend components (InverterStatusDashboard,SystemStatusCard,PredictionAnalysisView) branch display on it instead of inferring TOU-vs-not from a platform-name string.Design spec:
docs/superpowers/specs/2026-07-29-control-model-display-design.mdImplementation plan:
docs/superpowers/plans/2026-07-29-control-model-display.mdImplemented via subagent-driven-development: 15 plan tasks, each with its own implementer + task-scoped reviewer, plus a mid-plan regression caught by the full slow-test-suite run (a logging consumer of a renamed field that no task's scope covered), plus a final whole-branch review that caught one Critical + three Important cross-task findings (most notably: the Inverter page's "Current Mode" card still fabricated "Load First" via an untouched backend hourly-compat shim) — all fixed and re-reviewed clean.
Test plan
e2e/tests/inverter-schedule-control-model.spec.ts) — could not run against the real chromium binary in the sandboxed dev environment (browser install extraction hangs after a successful download, confirmed reproducible 3x, not a network/code issue). Verified 3/3 passing via a manual system-Chrome substitution instead. Needs a real CI run before merge.🤖 Generated with Claude Code