refactor(givtcp): move GivTCP REST handling into its own component - #4649
Draft
chalfontchubby wants to merge 6 commits into
Draft
refactor(givtcp): move GivTCP REST handling into its own component#4649chalfontchubby wants to merge 6 commits into
chalfontchubby wants to merge 6 commits into
Conversation
Moves the ~30 REST HTTP methods (rest_readData/rest_set*/rest_enable*) out of Inverter into a new GivTCPRest class composed as self.givtcp, cutting inverter.py by ~400 lines. Mechanical extraction only - GivTCPRest still reads and writes the owning Inverter's rest_api/rest_data attributes directly rather than owning its own state, since update_status/adjust_* still branch on those directly in several places (removing that is a later phase). First step towards pulling GivTCP-specific REST handling out of Inverter entirely, per Trefor's suggestion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extends GivTCPRest with read-side properties/methods (charge_enable_time, discharge_enable_time, soc_kwh, target_soc, power_readings, charge_window_times, discharge_window_times) that return plain values instead of requiring callers to walk GivTCP's raw JSON and handle rest_v3 version differences by hand. update_status now reads through these instead of indexing self.rest_data directly - branch shapes (including the "Power" key present but nested Power.Power missing" edge case) are unchanged. The equivalent raw-JSON reads in adjust_* methods (old_start, current_reserve, old_inverter_mode, etc.) are a separate follow-up, not included here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…omponent
Adds GivTCPComponent (givtcp.py), an async ComponentBase component registered in
components.py's COMPONENT_LIST. It polls GivTCP's REST API in the background and
publishes each inverter's controls and status as plain HA entities - the same
shape Fox/Solax/Ohme already use - then points Predbat's standard entity-based
apps.yaml keys at them via automatic_config().
It reads the existing givtcp_rest apps.yaml key directly (same scalar-or-list
shape Inverter used), so it auto-activates with no config changes for existing
users, per review feedback on the design doc.
GivTCPRest (extracted in earlier commits) is reused unchanged as the REST client.
Two things this needed:
- InverterRestState, a small stand-in for the subset of Inverter that GivTCPRest
reads/writes, since the component has no real Inverter to hand it.
- _run_blocking(), wrapping every GivTCPRest call in run_in_executor - its
requests/time.sleep calls would otherwise stall the shared event loop.
Write events are applied inline (as fox.py does), not queued for the next run().
Inverter.write_and_poll_value/option() polls the entity back within seconds to
decide whether a write landed, and the HA service call itself does not update the
entity - only publish_data() does - so deferring writes to run()'s 60s cadence
would have every rate/window/reserve write judged failed before it was attempted.
run() reports failure until at least one endpoint has returned data, so
ComponentBase's retry/backoff applies and, importantly, automatic_config() is held
back: it only runs once, and running it against entities that were never published
would replace a user's working apps.yaml config with unavailable entities.
Two bindings are deliberate:
- soc_kw, not soc_percent. Inverter prefers soc_percent when both are set, but
GivTCP reports SOC only as a whole percent (~0.1kWh steps on a 9.5kWh battery)
while SOC_kWh carries 3 decimals.
- power/voltage keys are skipped when givtcp_rest_power_ignore is set - that is
the documented opt-out for setups whose GivTCP power readings are wrong, and
claiming those keys would override exactly the config it protects.
Time selects publish all 1440 minutes: adjust_charge_window() writes whatever
minute the plan lands on, shifted again by inverter_clock_skew_*, so a coarser
option list would not contain the entity's own value.
Not published in this pass, deliberately: inverter_mode/pause_mode (their entity
paths auto-detect GE-Cloud vs local naming from the live value, which needs its
own careful look) and soc_max/battery capacity discovery (one-time startup
discovery, not part of the live control surface).
23 unit tests in tests/test_givtcp_component.py.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…now covers via entities
GivTCPComponent (previous commit) publishes charge/discharge rate, reserve, target
SoC, and charge/discharge window+schedule-enable as HA entities and auto-configures
Inverter's existing entity-based args to point at them. This removes the matching
`if self.rest_data: ... else: <entity path>` branches from Inverter, since both
REST-configured and entity-configured installs now take the same entity path:
- update_status(): charge/discharge enable, SoC, power/voltage readings,
charge/discharge window times, current_charge_limit
- get_current_charge_rate/get_current_discharge_rate
- adjust_charge_rate/adjust_discharge_rate
- adjust_reserve, adjust_battery_target
- adjust_charge_window, disable_charge_window
- adjust_force_export's window/schedule-enable writes (the redundant direct REST
slot write is removed; the entity write already covers it)
givtcp_rest_power_ignore's check goes with the power block deleted here; the
component honours it in automatic_config() instead, leaving those keys to the
user's own apps.yaml as documented.
Three REST-only behaviours remain deliberately untouched, because they have no
entity equivalent published yet (documented inline in Inverter.__init__ and at
each remaining call site):
1. Battery/capacity discovery in __init__ (soc_max, nominal_capacity,
calibration detection, max charge/discharge rate) - reads raw REST fields the
component doesn't publish.
2. adjust_pause_mode/adjust_inverter_mode - GivTCP-native pause/mode strings have
no entity equivalent.
3. adjust_force_export's discharge-target write for
DISCHARGE_TARGET_UNSUPPORTED_MODELS (#4517) - needs the raw REST model field.
Because of these, GivTCPRest/self.rest_data/self.rest_api stay alive in Inverter -
"zero REST code in inverter.py" is not reachable without first extending the
component to publish battery discovery, mode, and model-info entities, which is
bigger than this slice.
Updated the test_inverter.py assertions that checked REST command sequences or
REST-sourced values for the fields above; those now either go via entities
(covered by test_inverter_update) or moved to test_givtcp_component.py. Removed
one test for a REST cold-start race that can no longer happen: automatic_config()
now completes before any Inverter is constructed, and is held back until GivTCP
has actually returned data.
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.
Moves GivEnergy/GivTCP REST handling out of
inverter.pyinto a proper component, per your suggestion. Built as several small commits for review but intended to land as one unit.What it does
GivTCPComponent(givtcp.py) polls GivTCP's REST API in the background and publishes each inverter's controls and status as plain HA entities — the same shape Fox/Solax/Ohme already use — then points Predbat's standard entity-based apps.yaml keys at them viaautomatic_config().Inverterthen takes the ordinary entity path for those controls, so theif self.rest_api: ... else: ...special-casing goes away for everything the component covers.It reads the existing
givtcp_restkey directly (same scalar-or-list shape), so it auto-activates and existing users need no apps.yaml changes — that was your backward-compatibility point on the design doc.Commits
d24b29ae— extract the 17 REST HTTP methods intogivtcp_rest.py: GivTCPRest(mechanical, no behaviour change)3ae9f1ff— normalise the read side (SoC, power, window times) intoGivTCPRest, rewireupdate_status()off raw JSON indexing2762f0c1— the component itself, + 23 unit testsb3d8b6b7— collapse the now-redundant REST branches ininverter.py, updatetest_inverter.pyWhat still uses REST directly
inverter.pyis not REST-free, and I don't want to overstate it. Three things have no entity equivalent yet, each documented inline:Inverter.__init__(soc_max, nominal capacity, calibration detection, max rates)adjust_pause_mode/adjust_inverter_mode— these auto-detect GE-Cloud vs local naming (Pause ChargevsPauseCharge) from the entity's live value, which is more delicate than the plain W/%/time controls and deserves its own passadjust_force_export's discharge-target write forDISCHARGE_TARGET_UNSUPPORTED_MODELS(GivTCP discharge target written every cycle even when unchanged (regression from #4492) #4517) — needs the raw RESTmodelfieldSo
GivTCPRest/rest_datastay alive. Getting to zero REST code ininverter.pyneeds the component to publish battery-discovery, mode and model-info entities too — happy to do that next if you want it, but it's bigger than this slice and I'd rather land this first.Two judgement calls worth a look
soc_kwis bound, notsoc_percent.Inverterpreferssoc_percentwhen both are set, but GivTCP reports SOC only as a whole percent (~0.1kWh steps on a 9.5kWh battery) whereasSOC_kWhcarries 3 decimals. Bindingsoc_kwkeeps the precision the old direct-REST path had.givtcp_rest_power_ignoreis now honoured inautomatic_config()rather than inupdate_status(). Same intent (leave power/voltage to the user's own sensors), enforced one layer up — otherwise auto-config would claim exactly the keys that option exists to protect.Testing
23 new component tests,
test_inverter.pyupdated, fullrun_all --quickandrun_pre_commitgreen.But this has had no validation against real hardware — I don't have a GivEnergy system. The component's timing in particular (writes must apply inline, because
write_and_poll_*polls the entity back within seconds) is the sort of thing only a live inverter really proves. Draft until someone dogfoods it.🤖 Generated with Claude Code