refactor(givtcp): finish moving GivTCP REST handling into the component - #4739
Draft
chalfontchubby wants to merge 3 commits into
Draft
refactor(givtcp): finish moving GivTCP REST handling into the component#4739chalfontchubby wants to merge 3 commits into
chalfontchubby wants to merge 3 commits into
Conversation
First slice of the follow-on: takes adjust_inverter_mode and adjust_pause_mode off the direct REST client, leaving battery/capacity discovery and the #4517 discharge-target model check as the last two REST users in inverter.py. GivTCPComponent now publishes inverter_mode (Control.Mode) plus, on v3 only, pause_mode/pause_start_time/pause_end_time (Control.Battery_pause_mode and the Battery_pause_*_time_slot timeslots), and auto-configures the matching apps.yaml keys - all four already existed as writable sensor_list keys, so the entity paths in Inverter were already there and the REST branches simply go. Pause is gated on every configured inverter reporting v3, mirroring the existing givtcp_rest_power_ignore "any opts out, leave them all" rule: v2 has no /setBatteryPauseMode, which is why the old REST path was v3-gated too. Two details worth review: - _handle_write had no branch for a select that is not a time slot, so a plain-select write was silently dropped and then republished unchanged, which reads as a successful no-op. Added, with a test that fails without it. - adjust_inverter_mode's 30s changed_start_end sleep stays gated on rest_api. It works around GivTCP's own HA integration lagging behind a window write; a GivTCP-REST inverter has no such lag because GivTCPComponent applies the write and republishes inline before returning, so sleeping 30s every window change would be pure cost. Dropping the guard would have quietly added that to every GivTCP user. The pause mode entity carries GivTCP's native vocabulary (Disabled/PauseCharge /...), not the GE Cloud spelling - adjust_pause_mode picks between the two from the value it reads back, so this keeps it on the same side of that branch as the REST path did. Also removed a now-orphaned queue_rest_data in the force-export test: it fed setBatteryMode's read-back verification, and with that call gone the entry leaked into the next test's runAll and made an unrelated REST test retry 5x. Tests: 4 new component tests (27 total), test_inverter updated for the three mode assertions that were REST-shaped. Full run_pre_commit green. Still unvalidated against real hardware, same as the base PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e component Second slice: takes adjust_force_export off the direct REST client, leaving battery/capacity discovery in Inverter.__init__ as the last REST user. Rather than plumbing the raw inverter model out to a new apps.yaml key so Inverter could keep making this decision, the component simply does not publish a discharge_target_soc entity for a model whose Discharge_Target_SOC_1 register does not work. Inverter.adjust_force_export already leaves a target it cannot read alone, so an absent entity is the whole mechanism - and inverter.py no longer knows about inverter models at all. DISCHARGE_TARGET_UNSUPPORTED_MODELS moves to givtcp.py with its provenance comment intact. The unsupported-model notice is logged once per inverter rather than every 60s poll. A mixed fleet still works per inverter: the key is claimed for all of them and the unsupported one just has no entity behind it, which is exactly the "cannot read it" case. Claiming the key only when every inverter supports it would have stopped the supported ones writing too. Tests: the #4517 regression test moves to test_givtcp_component.py, reframed around whether the entity is published rather than whether a REST command is issued (28 component tests). test_inverter's force-export and reserve-tracking tests updated - the REST phase now asserts the entity write and that no REST command is issued at all. The direct GivTCPRest set/read_discharge_target tests stay as they are, since the component still uses those. Full run_pre_commit green. Still unvalidated against real hardware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third and last slice: Inverter.__init__ no longer parses the REST blob for battery/capacity discovery, so that path is an ordinary entity read for every inverter type. GivTCPRest gains normalised accessors (inverter_details, battery_capacity_kwh, nominal_capacity, battery_temperature, inverter_time, max_battery_rate, max_inverter_rate, in_calibration) which absorb the version differences that were open-coded in Inverter.__init__: v2's Invertor_Details block vs v3 renaming it to the inverter's serial number, nominal capacity in raw register units vs kWh, per-pack temperature averaging across three different field shapes, and the two ways calibration is reported. The 19.53125 divisor is carried over verbatim - it was back-calculated rather than derived, and this does not pretend to explain it. GivTCPComponent publishes these and claims soc_max, battery_temperature, inverter_time, inverter_limit and battery_calibration. Each is only published when GivTCP actually reports it, so a missing value falls through to the user's own apps.yaml rather than being published as an authoritative-looking zero. battery_capacity_nominal is honoured by choosing which value goes into soc_max, since Inverter multiplies by battery_scaling either way. Fixes a latent bug this would otherwise have exposed: Inverter derives battery_rate_max_raw for a GE inverter from the charge_rate entity's "max" attribute, and the component was publishing the generic 20000 ceiling from GIVTCP_CONTROLS. Harmless while REST discovery still overrode it, but it would have told Predbat every GivTCP battery could take 20kW the moment discovery moved. The rate entities now advertise Invertor_Max_Bat_Rate. New optional apps.yaml key battery_calibration (documented): a calibration cycle drives the battery outside its normal SoC range, so Predbat disables itself for that inverter while one runs. Absent means never calibrating, so nothing changes for inverters that do not report it. Tests: the two real captures (rest_v2/rest_v3) move from test_inverter to the component tests and now assert the accessors rather than Inverter's internals, plus per-version calibration coverage and a regression test for the rate max attribute (verified failing without the fix). 31 component tests. Deliberately left on REST in inverter.py: the version/serial/firmware metadata read, update_status's refresh, and the reserve read. That last one differs between the branches - REST does not apply the battery_min_soc floor that the entity path does, and reserve_percent_current is a reading of what the inverter is actually set to, so collapsing them is a behaviour change rather than a refactor and wants deciding on its own. Full run_pre_commit green. Still unvalidated against real hardware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Follow-on to #4649, per your ask there. Stacked on that branch rather than
main- review/merge that one first.Takes the three things #4649 explicitly left behind off the direct REST client, so
inverter.pyno longer parses GivTCP's REST blob.Commits
6c83993c- inverter mode and pause controls as entities.inverter_mode(Control.Mode) plus, on v3 only,pause_mode/pause_start_time/pause_end_time. All four already existed as writablesensor_listkeys, so the entity paths inInverterwere already there and the REST branches simply go. Pause is gated on every configured inverter reporting v3, mirroring the existinggivtcp_rest_power_ignore"any opts out, leave them all" rule - v2 has no/setBatteryPauseMode, which is why the old REST path was v3-gated too.d614c316- the GivTCP discharge target written every cycle even when unchanged (regression from #4492) #4517 discharge-target model check. Rather than plumbing the raw model string out to a new apps.yaml key soInvertercould keep making the decision, the component just doesn't publish adischarge_target_socentity for a model whose register doesn't work.adjust_force_exportalready leaves a target it can't read alone, so an absent entity is the whole mechanism - andinverter.pyno longer knows about inverter models at all. A mixed fleet still works per-inverter.97ad0a0e- battery/capacity discovery.GivTCPRestgains normalised accessors that absorb the version differences previously open-coded inInverter.__init__: v2'sInvertor_Detailsvs v3 renaming it to the serial number, nominal capacity in raw register units vs kWh, per-pack temperature averaging across three field shapes, and the two calibration representations. The19.53125divisor is carried over verbatim - it was back-calculated rather than derived, and this doesn't pretend to explain it.Two bugs found on the way
Both mutation-tested (confirmed failing without the fix):
_handle_writehad no branch for a select that isn't a time slot. A plain-select write was silently dropped and the entity then republished unchanged, which reads as a successful no-op. Would have bitten the momentinverter_mode/pause_modelanded.maxof 20000W.Inverterderivesbattery_rate_max_rawfor a GE inverter from exactly that attribute. Harmless while REST discovery still overrode it - but the moment discovery moved, every GivTCP user's battery rate limit would silently have become 20kW. They now carryInvertor_Max_Bat_Rate.Judgement calls worth a look
adjust_inverter_mode's 30schanged_start_endsleep stays gated onrest_api. It works around GivTCP's own HA integration lagging behind a window write; a GivTCP-REST inverter has no such lag, because the component applies the write and republishes inline before returning. Dropping the guard would have quietly added 30s to every window change for GivTCP users.battery_calibration(documented). A calibration cycle drives the battery outside its normal SoC range, so Predbat disables itself for that inverter while one runs. Absent means never calibrating, so nothing changes for inverters that don't report it.Disabled/PauseCharge/...), not the GE Cloud spelling -adjust_pause_modepicks between the two from the value it reads back, so this keeps it on the same side of that branch as the REST path was.Deliberately still on REST in
inverter.pyThe version/serial/firmware metadata read,
update_status's refresh, and the reserve read. That last one differs between the branches - REST doesn't apply thebattery_min_socfloor the entity path does, andreserve_percent_currentis a reading of what the inverter is actually set to, so collapsing them is a behaviour change rather than a refactor. Left for a separate decision rather than changed silently.Testing
31 component tests (up from 23). The two real captures (
rest_v2.json/rest_v3.json) move fromtest_inverterto the component tests and now assert the accessors rather thanInverter's internals, plus per-version calibration coverage.test_inverter's REST-shaped mode/target assertions updated - the REST phase now asserts the entity write and that no REST command is issued. Fullrun_pre_commitgreen.Also fixed a test-isolation bug: a
queue_rest_dataentry fedsetBatteryMode's read-back verification, and with that call gone it leaked into the next test'srunAlland made an unrelated REST test retry 5x.Same caveat as #4649: no validation against real hardware. Draft until someone dogfoods it. The write/republish timing is the part only a live inverter really proves.
🤖 Generated with Claude Code