refactor(givtcp): move GivTCP REST handling into its own component - #4864
refactor(givtcp): move GivTCP REST handling into its own component#4864springfall2008 wants to merge 29 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>
The merge of main into this branch (edbdd97) landed the comment update from 4fbacb2 (main, "treat a failing ge_cloud_direct as transient, and name it") but not the message string literal it was attached to, leaving the ValueError text stale while the test's expectation (also from that commit) came through correctly - a partial merge-conflict resolution, not a fresh regression. Everything else from that commit (the ge_cloud_direct transient branch, the per-source Warn naming) was already present and correct. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
…component # Conflicts: # apps/predbat/inverter.py
…st-component-2 # Conflicts: # apps/predbat/inverter.py
Resolved conflicts in inverter.py and test_inverter.py: - adjust_reserve: kept main's GH#4826 device min/max clamp but dropped its 'if not self.rest_data' guard, since the PR removes the REST write branch entirely and routes all reserve writes through the component-published entity. - test_inverter.py: kept the PR's removal of the REST-mode adjust_charge_window assertions (main's only change there was an 'is not True' lint fix to code this PR deletes).
… URLs
givtcp_rest is the key that enables the component, but its length says nothing
about how many inverters exist. The shipped apps.yaml pairs num_inverters: 1
with a two-entry givtcp_rest list - the same over-provisioning every other
per-inverter key in that template uses - so counting the list made a stock
single-inverter user get num_inverters: 2. Predbat then built Inverter(id=1)
against a host with nothing behind it and planned against a phantom battery.
set_arg_auto("inverter_type", ...) clobbered a mixed fleet's types the same way.
run() now probes every configured URL once to discover what is really there,
and settles the discovered set on the first pass that finds anything. A pass
where nothing answers leaves discovery open so the next cycle re-probes, rather
than locking in an empty fleet because GivTCP was still starting up.
automatic_config() is driven off that set and claims nothing when it is empty,
matching SolisAPI.automatic_config's existing shape.
Two consequences fall out of the same change:
- Endpoints that never answered are no longer re-polled. read_data() retries
with 20s then 40s sleeps, so re-probing the template's placeholder URL spent
longer failing than the 60s poll interval it runs on.
- The all-v3 pause gate now considers discovered inverters only. An endpoint
that never answered has rest_v3 False by default and would otherwise veto
pause control for a fleet whose live inverters are all v3.
Entity ids stay pinned to the REST endpoint index rather than being renumbered,
because _parse_entity feeds self.rest[n] on every write - renumbering would
route a surviving inverter's writes at a dead client.
5 new tests; full run_all and run_pre_commit green.
automatic_config() only ever ran once, so an inverter that was unreachable during Predbat startup - GivTCP still booting, a network blip - was lost until the user restarted Predbat. run() now re-probes endpoints that have never answered every GIVTCP_REDISCOVER_SECONDS and reconfigures when the fleet grows. Predbat already absorbs this: fetch_inverter_data() re-reads num_inverters every cycle (execute.py:881) and rebuilds its inverter list whenever the count changes (execute.py:912), so no restart is needed for the new size to take effect. Three constraints the implementation holds to: - Append, never insert. The order of self.discovered is Predbat's inverter numbering, so slotting a late endpoint 0 ahead of the endpoint already running as inverter 0 would repoint that inverter at different physical hardware, with its SoC, rates and windows following the wrong battery. - Never shrink. A discovered inverter that stops answering is a health problem, not a reconfiguration: dropping it would rebuild Predbat's list for a smaller fleet, leave a real battery uncontrolled at whatever settings it last had, and thrash when it returned. Leaving it in makes Inverter.__init__ fail visibly. - Re-probe cheaply. Startup discovery keeps read_data()'s 20s/40s retry ladder because GivTCP may be booting, but the hourly re-probe passes retry=False so a placeholder URL that will never answer costs one GET rather than ~100s. Known edge: the fleet can grow between a cycle's plan and its execute, since fetch_inverter_data(create=False) re-reads num_inverters too. Worst case is one cycle of inconsistency that self-corrects on the next replan. 5 new tests; the dead-endpoint test is narrowed to the normal poll cadence now that the re-probe boundary legitimately probes it. Full run_all and run_pre_commit green.
…answering read_data() returns None on failure but run() only assigns on success, so rest.inverter.rest_data kept the last good snapshot indefinitely. publish_data() then republished every frozen value, and dashboard_item -> set_state_wrapper refreshed each entity's HA last_updated, so nothing about the entities revealed that GivTCP had died - Predbat carried on planning charge and export against an SoC that stopped moving hours earlier. Two independent signals now cover that: - A warning per unresponsive inverter, naming it and saying what it means (entities stale, component heading for error). read_data already calls record_status, but that says the read failed, not that Predbat is now acting on frozen data. - run() withholds update_success_timestamp() while the last poll did not read every managed inverter. ComponentManager treats a component whose last_updated_time is over 60 minutes old as not alive (components.py:852), so a persistent failure surfaces there on its own. run() still returns True: a failed poll should not tear the component down and restart it over a transient blip, which is what returning False would do. The staleness timeout is the intended path. An endpoint that was never adopted does not count against health - the shipped apps.yaml over-provisions givtcp_rest, so a placeholder entry is the normal case and holding the component in error for it would make the default config permanently unhealthy. Only inverters in self.discovered count. 4 new tests; full run_all and run_pre_commit green.
…ion) main's adjust_battery_target called rest_enableChargeTarget(True) before rest_setChargeTarget(soc). This refactor carried enable_charge_target() into givtcp_rest.py but left it with no callers, so the enable step was silently lost for GivTCP REST users. That step is load-bearing. cf92d8c (#4141) added it because GivTCP's setChargeTarget writes CHARGE_TARGET_SOC (reg 116) but never enables ENABLE_CHARGE_TARGET (reg 20); with reg 20 off - GivTCP's default - the inverter ignores the SOC limit and charges to 100%, which was the root cause of Hold Charge not holding on AIO inverters. The failure is silent: set_charge_target verifies its write by reading Target_SOC back, and reg 116 genuinely is written, so Predbat logs the write successful while the inverter disregards the limit. Fixed the way the rest of this component works, rather than by reaching back into inverter.py: the register is published as a charge_limit_enable switch and that key is auto-configured. Inverter.adjust_battery_target already writes charge_limit_enable straight after charge_limit - the key was simply never populated - so no inverter.py change is needed, and the ordering matches what #4141 established for the non-REST path. The switch is withheld, and the key left to the user's apps.yaml, when GivTCP does not report Enable_Charge_Target: enable_charge_target() verifies its write by reading that field back, so publishing a control for an unreported register would burn the full retry ladder on every charge limit change. 4 new tests; full run_all and run_pre_commit green.
…of discharge Two related fixes to InverterRestState, which carried three fields copied from Inverter that either could not be right or were not needed. Battery sizing (found while reviewing the rate tolerance below) --------------------------------------------------------------- Inverter computes soc_max = nominal_capacity * battery_scaling and measures degradation as (nominal_capacity - trimmed_mean) / nominal_capacity, so nominal_capacity has to be the design capacity. On main it was, set unconditionally from raw.invertor.battery_nominal_capacity; the battery_capacity_nominal expert switch only decided whether soc_max used it too. This component published one soc_max sensor carrying the reported (already degraded) capacity, and Inverter takes nominal_capacity straight from it - so trimmed_mean/nominal collapsed to ~1.0 and degradation to ~0, and battery_scaling_auto could no longer see what it exists to measure. Now follows GE Cloud's model: soc_max carries the design capacity, battery_soh is reported/design clamped at 1.0, battery_dod is the depth of discharge, and battery_dod_soh is their product with battery_scaling pointed at it. Net planned size is unchanged - design * (reported/design) == reported - but degradation is visible and nominal_capacity is the nameplate figure again. GivTCP does not report depth of discharge, so it defaults to 1.0 and can be set per inverter with the new givtcp_battery_dod key (documented). Where no design capacity is reported soc_max falls back to the reported figure and no scaling is claimed, rather than displacing the user's own with a fabricated 1.0. set_arg_auto is used so a manually set battery_scaling logs the displacement. The expert switch keeps its meaning: with it on the health derate is suppressed, which reproduces main's soc_max = nominal. Rate write tolerance -------------------- set_charge_rate accepted a write within battery_rate_max_charge * MINUTE_WATT / 12 of the target. InverterRestState was constructed with a placeholder of 1.0, making that 5000W where a real 2600W inverter gives 217W - so a rate the inverter never applied verified as successful, logged as such, and counted as a register write. Less severe than it first appears: _handle_write discards the return value and Inverter.write_and_poll_value re-verifies against the republished entity with fuzzy = rate/20, so the failure is still caught. The costs were skipped REST-level retries, a false success log and an inflated register-write count. Fixed by deleting battery_rate_max_charge/_discharge outright rather than feeding them from discovery: they were only ever read by these two checks, and max_battery_rate() is already on the object. Removing the fields means the bug cannot recur. battery_scaling stays 1.0 and is now commented - Inverter applies it itself on reading soc_kw, so plumbing the real value would double-apply it. 10 new tests; full run_all and run_pre_commit green.
…ign figures
Corrects the SoH source added in the previous commit. Battery_Capacity_kWh and
raw.invertor.battery_nominal_capacity are the SAME design figure in different
units - the real captures in coverage/cases/rest_v{2,3}.json show 186.0 Ah /
19.53125 == 9.5232 kWh - so deriving SoH from their ratio yielded a constant 1.0
and, worse, bound battery_scaling to a sensor permanently reading 1.0, displacing
any value the user had set by hand.
GivTCP does report real state of health, per battery module under Battery_Details
as Battery_Capacity against Battery_Design_Capacity in Ah: flat on v2, nested
under Battery_Stack_N on v3. battery_soh() now sums those across modules and
clamps at 1.0. The v2 capture gives 184.82/186.0 = 0.9937 - the same battery and
ratio GE Cloud's own SoH test uses - and v3 gives 187.61/186.0, which clamps.
Both are now asserted directly against the real capture files rather than only
against hand-built blobs, and a regression test pins that SoH is not derived from
the two equivalent design figures.
Also confirms a full readData dump reports no depth of discharge anywhere, so
givtcp_battery_dod remains the way to supply one.
19.53125 is 1000/51.2, the Ah to kWh conversion at these packs' 51.2V nominal:
186 Ah x 51.2 V / 1000 == 9.5232 kWh == the reported Battery_Capacity_kWh. That
answers the XXX main carried on the constant, and is now in the docstring along
with the 51.2V assumption that makes it not a general conversion.
3 new tests (v3 stacked layout, the clamp, and the design-figure regression
guard); full run_all and run_pre_commit green.
Inverter derives battery_rate_max_raw for a GE inverter from the charge_rate
entity's "max" attribute, and battery_rate_max_charge/discharge/export are all
sized from that. publish_data only overrode the attribute when GivTCP reported a
rate, otherwise leaving GIVTCP_CONTROLS' generic 20000 - so an install that
reports no rate told Predbat it had a 20kW battery. Confirmed by test before
fixing: the published attribute really was 20000.
main had a REST-specific chain here (inverter.py:389-395): Invertor_Max_Bat_Rate,
then Invertor_Max_Rate, and only then get_arg("charge_rate", attribute="max",
default=2600.0). This branch keeps only that last line, and charge_rate now
resolves to the component's own entity rather than the user's, so the generic
ceiling became the answer instead of the 2600W default.
Now publishes no max at all when the rate is unknown. ha.py:804-808 returns the
caller's default for a missing attribute, so Inverter falls back to its own
2600W - precisely main's behaviour in that case.
Both committed captures do report Invertor_Max_Bat_Rate (2600 on v2, 3600 on v3),
so this only fires when inverter_details() resolves empty: on v3 that happens
when raw.invertor.serial_number is missing or its named block is absent, which
loses capacity, inverter limit and inverter time at the same time.
1 new test, confirmed red first; full run_all and run_pre_commit green.
publish_data emits soc_max, battery_temperature, inverter_time and inverter_limit only when GivTCP reports them, and its comment said a missing one "falls back to the user's own apps.yaml value". It could not: automatic_config claimed every discovery key unconditionally, so the arg pointed at an entity that was never created and get_arg fell through to its own default instead of the user's value. For soc_max that default is 0.0, which takes Inverter through nominal_capacity 0, into battery_scaling_auto, and on to the final "Unable to determine battery size for inverter N, using 8 kWh default ... you must set soc_max in apps.yaml" fallback - advice the user may well have already followed, before auto-config overwrote it. Confirmed by test first: a configured soc_max: 12.0 really was replaced by an unpublished entity. publish_data now records which discovery sensors each inverter published, and automatic_config claims each key only where every managed inverter reported it, logging the ones it leaves alone. This is the same "claim only what you publish" rule already applied to charge_limit_enable and battery_scaling. battery_calibration is deliberately exempt: in_calibration() returns a definite False when GivTCP reports nothing, so the sensor is always published and "not calibrating" is the correct default - unlike soc_max, where a default is actively wrong. A test pins that distinction. Note automatic_config runs once per discovered fleet, so a sensor that only appears on a later poll stays unclaimed and the user's apps.yaml value keeps working - the safe direction. 2 new tests, confirmed red first; full run_all and run_pre_commit green.
… an end
GivTCP's slot endpoints take both ends of a window, but an entity write only
carries one, so the component filled the other in from the last status snapshot -
defaulting to "00:00:00" when there was no snapshot. That programs a zero-length
or midnight-terminated charge, export or pause window on real hardware. There is
no safe default for the half of a window you do not know.
Confirmed reachable by test first: with rest_data None the handler really did
call set_charge_slot1("09:00:00", "00:00:00").
The route is narrower than first reported. The earlier discovery fix removed the
"second inverter of a fleet" case - undiscovered inverters are never
auto-configured, and a discovered one whose poll later fails keeps its stale
snapshot rather than reverting to None. What remains is a component restart from
the web UI (web.py -> restart() -> initialize()), which resets rest_data to None
while Predbat's args still point at these entities from the previous
automatic_config. That is also the moment the window is widest, since a restart
is most likely when GivTCP is already unwell and the first re-read is slow.
All three slot handlers now share _window_for_write, which returns None and logs
when the other end is unknown. Inverter's write-and-poll then notices the entity
never changed and reports the failure through its usual path, which is the
correct visible outcome.
Also records findings 8 and 9 as accepted limitations of the refactor.
2 new tests, confirmed red first; full run_all and run_pre_commit green.
main gated its whole export-target block on "self.rest_data and self.rest_v3". The refactor kept the model half of that check but dropped the version half, so a v2 install published and auto-configured discharge_target_soc. That is not theoretical: the committed rest_v2.json capture carries raw.invertor.discharge_target_soc_1 (value 0), and read_discharge_target() returns 0 rather than None, so publish_data really did emit number.predbat_givtcp_0_discharge_target_soc with rest_v3 False - verified by test before fixing. Every force-export cycle would then POST to a /setDischargeTarget endpoint that does not exist on v2, burn INVERTER_MAX_RETRY_REST attempts with their sleeps, and record an error: the every-cycle rewrite loop #4517 was raised to end. Both the publish and the auto-config claim are now gated on v3, the claim requiring v3 on every discovered inverter as the pause keys already do. Two existing tests set no version and were relying on the ungated behaviour; they now set rest_v3 explicitly, which is also the only configuration where the model check they exercise can apply. Also removes GIVTCP_REVIEW_FINDINGS.md from the branch - it was committed by mistake in the previous commit, is a working document rather than part of this change, and fails markdownlint and cspell. 3 new tests, confirmed red first; full run_all and run_pre_commit green.
…Type The #4517 guard matched raw.invertor.model against ("Ac", "Hybrid_gen1", "Hybrid_gen2"). On GivTCP v3 that field is a numeric code - '2' on the captured system - so the guard never fired at all on the version most users run, and those inverters kept rewriting the export target every cycle. The readable model is in Invertor_Type: 'Gen 1 Hybrid' on v2, 'Gen2 Hybrid' on v3. It lives in the detail block, which inverter_details() already normalises across both versions, so no new plumbing was needed. Matching is on the set of alphanumeric tokens rather than the literal string, because the two vocabularies disagree on word order and spacing - 'Gen 1 Hybrid' and 'Hybrid_gen1' both reduce to {gen, 1, hybrid}. raw.invertor.model matching is untouched, so builds that do report names there behave exactly as before; tests pin that, and that a numeric code identifies nothing. Gen2 Hybrid is deliberately NOT in the Invertor_Type set. "Hybrid_gen2" was inferred from a firmware archive rather than observed on hardware, and since most users are on v3, matching it there would remove export target control from the largest group of users on the strength of that inference. It is still honoured where a build reports that exact raw model, so nobody's current behaviour is reversed. Add it once an affected Gen2 is actually confirmed. Pre-existing on main, which used the same expression - not a regression from the refactor, but worth fixing while the surrounding code is being moved. 3 new tests, one confirmed red first (the other two pin that existing behaviour is unchanged); full run_all and run_pre_commit green.
The GH#4826 clamp reads the reserve entity's min/max attributes so Predbat never asks for a value the device silently clamps and confirms. Enabling it for GivTCP REST users (done while resolving the merge with main) meant it started reading GIVTCP_CONTROLS' hard-coded min of 4 rather than anything the device reported, while the comment claimed it was respecting "the inverter's own register bounds". The clamp itself is protective and the 4 is right for GE: set_reserve() verifies result == target exactly, so asking below the device floor produces exactly the GH#4826 retry loop - on main's REST path too, which had no clamp at all. There is no device bound to discover. Nothing in a full readData dump reports one. battery_discharge_min_power_reserve reads 4 in both captures and looks like a candidate, but it is the discharge floor setting rather than a limit on it, so using it as the minimum would pin reserve to wherever it already sits and stop it ever coming down. The real loss was a user who set battery_min_soc below 4 - set_reserve_min allows 0 - being silently floored at 4 with no way back. battery_min_soc now lowers the advertised minimum. It never raises it: keeping a target above a policy floor is Inverter's own reserve_percent job, and duplicating that here would put the same rule in two places that can disagree. A test pins each direction. The inverter.py comment no longer claims these bounds are always read from the device. Note MockBase.get_arg ignores index= and returns the raw value, so the tests use a scalar battery_min_soc; the per-inverter list path real get_arg indexes is not covered by the mock. 3 new tests, one confirmed red first; full run_all and run_pre_commit green.
power_readings() fell back to get_arg("battery_voltage") when GivTCP reported
none, which is what main did too - but on main that key resolved to the user's
own sensor. Here automatic_config points it at the sensor power_readings feeds,
so the component reads back its own last publication.
publish_data runs before automatic_config on the first poll, so the reading
freezes at whatever that first cycle produced and never tracks the battery again:
52.0 where the user had configured nothing.
Only GivTCP v3 reports Battery_Voltage. v2's Power block has none at all -
confirmed absent in cases/rest_v2.json, where v3 reports 53.65 - so there is
nothing for v2 to read and no reason to publish a sensor.
power_readings() now returns None rather than consulting an arg, publish_data
publishes no voltage sensor when it is None, and battery_voltage is split out of
GIVTCP_AUTO_CONFIG_POWER_KEYS so it is claimed only where one was actually
published. That is the same "claim only what you publish" rule already applied to
the charge target enable switch, the discovery keys and battery_scaling. A v2
fleet keeps the user's own voltage sensor working, which is what main did.
givtcp_rest_power_ignore still covers it, so the documented opt-out is unchanged.
3 new tests, two confirmed red first (the third pins that v3 is unaffected); full
run_all and run_pre_commit green.
read_data() only checks that a top-level Control block exists, not its contents, while target_soc, charge_enable_time and discharge_enable_time indexed into it directly. A GivTCP version or partial response missing one field raised a KeyError straight out of publish_data: every entity after that point went unpublished for that inverter - the reserve, the window selects, soc_kw, the power block - and so did every entity of every later inverter in the fleet. Both committed captures carry all the Control fields, so this needs a GivTCP that omits one. The blast radius is what makes it worth fixing regardless. Three parts: - The three Control reads use .get() and return None when the field is absent. - The schedule switches are no longer published when unknown. "on" if rest.charge_enable_time else "off" turned an unreported flag into a confident "off", telling Predbat the schedule was disabled and having it write to enable it. They are now claimed per key on publication, like the discovery keys, so an unreported flag leaves the user's own switch in play. - Each inverter's publish is wrapped, so anything unexpected raising part-way through one inverter no longer starves the others, and the log names the inverter and its URL. Note the review's claim that the failure was "logged as a generic component error rather than pointing at the missing field" was wrong: ComponentBase.start() logs str(e) and a KeyError's string is the key name. What was missing was which inverter it came from, which the new message supplies. One existing test called automatic_config() without publishing first and relied on the schedule keys being claimed unconditionally; it now publishes first, as the other gated-key tests do. 3 new tests, all confirmed red first; full run_all and run_pre_commit green.
update_status() re-read the whole GivTCP blob every cycle and nothing consumed the result. rest_data is only read in __init__ - for the version, firmware and serial metadata, and the raw Battery_Power_Reserve - and __init__ does its own read before update_status ever runs. The refresh cost a blocking HTTP GET per inverter on every 5-minute cycle, on top of the component's own 60s poll. Worse, when GivTCP is slow read_data()'s retry ladder puts 20s + 40s + 40s of Inverter.sleep() inside the main planning loop and records an error, all for data that is then discarded. The block comment in __init__ is corrected too. It claimed REST was kept alive for battery/capacity discovery, calibration, pause_mode/inverter_mode and the #4517 model check - all of which the component publishes as entities now. What actually remains is the metadata read, and the raw reserve read that deliberately differs from the entity path by reporting what the inverter is set to without applying battery_min_soc. Inverter.rest_v3 is write-only after this, but left alone: it is trivial dead state and removing it buys nothing. 1 new test, confirmed red first. It matches the call pattern rather than the bare word so the explanatory comment does not trip it, and is a guard against reintroduction rather than a behavioural test - driving a real update_status needs the full Inverter harness in test_inverter.py. Full run_all and run_pre_commit green.
Code review: correctness of this refactor vs
|
| # | Finding | Location | Status |
|---|---|---|---|
| 1 | num_inverters from configured list length |
givtcp.py |
✅ Fixed — bd15cf0c, 15fe7ece |
| 2 | Stale rest_data republished as healthy |
givtcp.py |
✅ Fixed — 28c2c61f |
| 3 | enable_charge_target() never called |
givtcp_rest.py:342 |
✅ Fixed — see below |
| 4 | Rate write tolerance ~23× too wide | givtcp.py:173 |
✅ Fixed — see below |
| 5 | max: 20000 becomes battery_rate_max_raw |
givtcp.py:73 |
✅ Fixed — see below |
| 6 | inverter_limit overrides the user's AC limit |
givtcp.py:349 |
⛔ Won't fix here — see below |
| 7 | soc_max apps.yaml fallback destroyed |
givtcp.py:339 |
✅ Fixed — see below |
| 8 | REST-failure fallback path eliminated | givtcp.py:349 |
📌 Accepted limitation |
| 9 | Window written as two non-atomic writes | inverter.py:2954 |
📌 Accepted limitation |
| 10 | Window other-end defaults to 00:00:00 |
givtcp.py:367 |
✅ Fixed — see below |
| 11 | rest_v3 gate dropped on discharge target |
givtcp.py:247 |
✅ Fixed — see below |
| 12 | GH#4826 clamp reads a hard-coded min: 4 |
givtcp.py:76 |
✅ Fixed — see below |
| 13 | battery_voltage read-back loop |
givtcp_rest.py:128 |
✅ Fixed — see below |
| 14 | Unguarded rest_data indexing aborts publish |
givtcp.py:234 |
✅ Fixed — see below |
| 15 | Dead REST read every cycle in update_status |
inverter.py:1313 |
✅ Fixed — see below |
| 16 | Design capacity discarded, neutering SoH | givtcp.py:381 |
✅ Fixed — see below |
| 17 | DISCHARGE_TARGET_UNSUPPORTED_MODELS matches no real capture |
givtcp.py:62 |
✅ Fixed — see below |
Three themes account for most of them:
automatic_config()claims apps.yaml keys unconditionally (6, 7, 8) — including the very keys
the shipped template tells users to keep as the REST-failure fallback.- The entity layer publishes hard-coded attributes that
Inverterreads back as device truth
(4, 5, 12). - Guards were dropped that
mainhad for stated reasons (3, 9, 11).
3. enable_charge_target() is never called — the inverter ignores the target SOC ✅ FIXED
Verified real, and a regression of #4141.
cf92d8caadded the enable specifically because
"when reg 20 is off (the GivTCP default), the inverter ignores the SOC limit and charges to 100%,
which is the root cause of Hold Charge not holding on AIO inverters". Fixed by publishing the
register as acharge_limit_enableswitch and auto-configuring that key, so
adjust_battery_target's existing entity path performs the enable. Withheld when GivTCP does not
report the register.
Location: apps/predbat/givtcp_rest.py:342
main's adjust_battery_target REST branch called rest_enableChargeTarget(True) immediately before
rest_setChargeTarget(soc), with the comment "Enable charge target, without it the inverter ignores
the target SOC."
GIVTCP_CONTROLS maps charge_limit → set_charge_target only. The component publishes no
charge_limit_enable switch and does not auto-configure that key. grep shows zero production
callers of enable_charge_target — it was carried into the new client but orphaned.
Failure: a REST-only user (Docker / Predbat.com, or anyone without the GivTCP HA integration
entities in apps.yaml — which the PR says is supported since "existing users need no apps.yaml
changes") writes the target SOC to a register the inverter ignores, and the battery charges past the
planned limit.
Fix direction: either publish a charge_limit_enable switch and auto-config it, or have the
component's set_charge_target write path call enable_charge_target(True) inline before the target.
4. Rate write-verification tolerance is ~23× too wide ✅ FIXED
Real, but less severe than first reported.
_handle_writediscards the return value and
Inverter.write_and_poll_valuere-verifies against the republished entity withfuzzy = rate/20,
so an unapplied write is caught — the review's "never recorded" claim was wrong. The real costs
were skipped REST-level retries, a false success log, and an inflated register-write count. Fixed
by deleting the redundantbattery_rate_max_charge/_dischargefields entirely and sizing the
tolerance frommax_battery_rate(), which the object already had.
Location: apps/predbat/givtcp.py:173
InverterRestState is constructed with placeholder battery_rate_max_charge / battery_rate_max_discharge
of 1.0. GivTCPRest.set_charge_rate() accepts a write when:
abs(new - rate) < inverter.battery_rate_max_charge * MINUTE_WATT / 12
With the real Inverter value (kW/min — e.g. 2600/60000 = 0.0433) that threshold is 217 W. With
the hard-coded 1.0 it is 5000 W (and 2400 W for discharge, via /25).
Failure: Predbat writes charge_rate 200 W to hold the battery; the inverter ignores it and stays
at 3000 W. |3000-200| = 2800 < 5000, so set_charge_rate logs "successful on retry 0", increments
count_register_writes, and returns True. The real failure is never recorded.
Fix direction: feed the discovered per-inverter max rate into InverterRestState once known
(max_battery_rate() already exists), rather than leaving the placeholder in place.
5. A missing max rate publishes a 20 kW battery ✅ FIXED
Confirmed by test: with no reported rate the published attribute really was
20000.
mainhad a REST-specific chain (inverter.py:389-395) tryingInvertor_Max_Bat_Rate, then
Invertor_Max_Rate, and only then falling back toget_arg("charge_rate", attribute="max", default=2600.0). The branch keeps only that last line, andcharge_ratenow resolves to the
component's own entity. Fixed by publishing nomaxattribute when the rate is unknown:
ha.py:804-808returns the caller's default for a missing attribute, soInvertergets its own
2600 W — exactly main's fallback. Both real captures do reportInvertor_Max_Bat_Rate(2600 v2,
3600 v3), so this fires only wheninverter_details()resolves empty — which on v3 also means
capacity, inverter limit and time are lost at the same time (see finding 7).
Location: apps/predbat/givtcp.py:73
publish_data() overrides charge_rate_attributes['max'] only when rest.max_battery_rate() is
truthy; otherwise the generic GIVTCP_CONTROLS ceiling of 20000 is published. inverter.py:310
then does get_arg("charge_rate", attribute="max", default=2600.0) and gets 20000.
On main there was no such entity to hit, so the same call fell back to 2600 W or the user's real
GivTCP entity max.
Failure: a GivTCP install whose inverter_details() lacks Invertor_Max_Bat_Rate /
Invertor_Max_Rate (v3 where the serial-named block is missing, or v2 firmware that omits it) has
Predbat planning a 20 kW charge/discharge battery. battery_rate_max_charge, discharge and export
are all sized off it.
Fix direction: withhold the max attribute entirely when the real rate is unknown, so Inverter
falls back to its own default instead of trusting a placeholder ceiling.
6. inverter_limit silently overrides the user's hand-set AC limit ⛔ WON'T FIX HERE
Real, but not a defect of this PR, and two claims in the review are wrong. It is not
silent — the branch usesset_arg_auto, which logs a note naming the displaced value. And it is
not aberrant: ten components already auto-setinverter_limit(AlphaESS, Deye, Fox, GE Cloud,
Solax, Solis, Sunsynk, Sigenergy, Teslemetry, gateway), eight of them with bareset_argwhich
logs nothing. GivTCP was the outlier; this brings it into line. Where the user set nothing,
behaviour is identical to main.The concern underneath is legitimate — an AC limit often encodes a site constraint (G98/G99, DNO)
that the inverter cannot know, unlikesoc_maxorbattery_temperaturewhich are device facts. But
that applies to all ten components today. It is a repo-wide policy question forset_arg_auto
(whose docstring states "auto-discovery always wins currently"), and belongs in its own change
applied uniformly. Note there is no escape hatch:inverter_limit_overridecaps
inverter_limit_charge/dischargebut neverself.inverter_limit.
Location: apps/predbat/givtcp.py:349 (GIVTCP_AUTO_CONFIG_DISCOVERY_KEYS)
main assigned self.inverter_limit from REST Invertor_Max_Inv_Rate first, then at
inverter.py:419 let if "inverter_limit" in self.base.args override it with the apps.yaml value —
the user always won. The template documents it as a user value: "Inverter max AC limit (one per
inverter). E.g for a 3.6kw inverter set to 3600" (apps.yaml:219-221).
automatic_config now replaces that arg with sensor.<prefix>_givtcp_N_inverter_limit.
Failure: a user with a 3.6 kW export limit whose GivTCP reports Invertor_Max_Inv_Rate 6000 gets
planned at 6 kW AC, and clips or trips on export.
Fix direction: do not auto-claim inverter_limit when the user has set it explicitly, or drop it
from the discovery keys entirely and let Inverter keep its existing precedence.
7. soc_max fallback is destroyed, and can leave soc_max = 0 ✅ FIXED
Confirmed by test — a user's
soc_max: 12.0really was replaced by an entity that was never
published. Not the same issue as finding 6: that displaces a key whose entity works, this claims
a key with nothing behind it. Fixed by tracking which discovery sensors each inverter actually
published and claiming each key only where every managed inverter reported it.battery_calibration
is deliberately exempt:in_calibration()returns a definiteFalsewhen unreported, so it is
always published and "not calibrating" is the correct default.
Location: apps/predbat/givtcp.py:339
automatic_config() claims all of GIVTCP_AUTO_CONFIG_DISCOVERY_KEYS (soc_max,
battery_temperature, inverter_time, inverter_limit, battery_calibration) unconditionally —
including sensors publish_data() may never publish.
publish_data() only emits sensor.<prefix>_givtcp_N_soc_max when rest.battery_capacity_kwh() is
truthy, and its comment claims a missing one "falls back to the user's own apps.yaml value". It
cannot: automatic_config has already replaced args['soc_max'] with the entity id.
Failure: GivTCP omits Battery_Capacity_kWh (or inverter_details() returns {}) → resolve_arg
resolves a non-existent entity → get_arg returns default 0.0 → Inverter.soc_max = 0
(inverter.py:306-307). main had an explicit rescue for exactly this ("Warn: REST data does not
report Battery Capacity kWh, attempting to use soc_max apps.yaml instead…") which this PR deletes.
Fix direction: claim a discovery key only when its sensor was actually published — the same
discovery-gated principle already applied to num_inverters.
8. The documented REST-failure fallback path no longer exists 📌 ACCEPTED LIMITATION
Acknowledged as a known limitation of this refactor rather than something to fix here. It is
inherent to moving REST behind entities: once Predbat's keys point at the component, the
GivTCP-HA-integration entities are no longer consulted.Partially mitigated by finding 7's fix — a key whose sensor was never published now leaves the
user's own apps.yaml entity in place. Findings 2's warning and health timeout mean a REST outage is
at least surfaced rather than silent. Keys that are published still displace the fallback, so the
outage path itself remains. Worth stating plainly in the PR description.
Location: apps/predbat/givtcp.py:349
apps/predbat/config/apps.yaml:102-103 says verbatim:
If not using REST then instead set the Control here (one for each inverter)
You should keep this section even when using REST as a fallback if it fails and for charge curve calculations
main honoured that: every REST branch was if self.rest_data: … else: <entity>, so a REST outage
fell through to sensor.givtcp_<serial>_*, which the GivTCP HA integration still populates over MQTT.
After this PR those args are replaced by the component's own entities — which are exactly the ones
that go stale when REST fails (see finding 2). A GivTCP REST outage now has no recovery path.
Fix direction: architectural, and worth a decision rather than a patch. Either leave the
GivTCP-HA-integration keys unclaimed, or have the component mark its entities unavailable when stale
so Inverter fails visibly rather than acting on frozen values.
9. Windows are written as two non-atomic writes, tripling register writes 📌 ACCEPTED LIMITATION
Verified real, for both the charge and export windows:
mainwrote the whole window in one
rest_setChargeSlot1(new_start, new_end)and explicitly skipped both the per-end entity writes
("REST will be written as start/end together") and the disable step ("for REST no need as we
change start and end together anyhow"). Where both ends move, a REST user now does 4 register
writes (disable, 2x setChargeSlot1, re-enable) against main's 1.Accepted: this is simply how the normal entity control path works, and REST users are now on
it like every other inverter. Three things temper it: the disable/re-enable is correct now
rather than a bug (it exists to avoid a blip during non-atomic writes, so it cannot be removed
without restoring atomicity); when only one end changes the caller writes only that end, giving
parity with main; andcount_register_writesis observability only (logged plus the
inverter_register_writes_totalmetric), with no threshold or enforcement.A proper fix would be an atomic "write both ends" capability on the Inverter/component interface,
benefiting GE Cloud and the others too. That is a design addition, not a patch, and belongs in its
own change.
Location: apps/predbat/inverter.py:2954
main wrote the whole window with one rest_setChargeSlot1(new_start, new_end) and explicitly
skipped the disable step: "Disable charging if required, for REST no need as we change start and end
together anyhow."
Now write_and_poll_option fires charge_start_time and charge_end_time separately. Each
select_event drives _set_charge_slot, which reissues /setChargeSlot1 with the other end taken
from rest_data — so the inverter is briefly programmed with (new_start, old_end), which can be an
inverted or overlapping window if the new start is later than the old end.
Separately, the not self.rest_data guard at line 2954 was dropped, so every window change for a REST
user now also issues enableChargeSchedule(False) + adjust_idle_time + enableChargeSchedule(True).
Failure: transient invalid window on real hardware, plus ~3× the register writes per change.
GivEnergy register writes are flash-backed, and Predbat counts them for a reason.
Fix direction: give the component a combined "set window" write that takes both ends, and restore
the disable-step skip for the REST path.
10. Window other-end defaults to 00:00:00 before the first poll ✅ FIXED
Not the same as finding 9 — 9 is inherent to using the normal control path, this is the
component fabricating a value it does not have. Confirmed reachable by test: with no snapshot the
handler really did callset_charge_slot1("09:00:00", "00:00:00").The reachable path is narrower than the review suggested. Finding 1's fix removed the
"second inverter of a fleet" route (undiscovered inverters are never auto-configured, and a
discovered one that later fails keeps its stale snapshot rather than reverting toNone). What
remains is a component restart from the web UI (web.py:5088->restart()->initialize()),
which resetsrest_datatoNonewhile Predbat's args still point at these entities.Fixed by refusing the write and logging, for all three slot types. There is no safe default for
the half of a window you do not know, and refusing leaves Inverter's write-and-poll to report the
failure through its usual path.
Location: apps/predbat/givtcp.py:367
timeslots = rest.inverter.rest_data.get("Timeslots", {}) if rest.inverter.rest_data else {}
... .get("Charge_end_time_slot_1", "00:00:00")Failure: a write event arriving before the first successful poll — or after a component restart —
writes only the end the caller asked for and sets the other to 00:00:00: a zero-length or
midnight-terminated charge window on real hardware. _set_discharge_slot and _set_pause_slot do the
same.
Fix direction: refuse the write and log, rather than defaulting. There is no safe default for the
half of a window you do not know.
11. rest_v3 gate dropped: v2 gets a discharge-target retry loop ✅ FIXED
Confirmed against the real v2 capture:
publish_datareally did emit
number.predbat_givtcp_0_discharge_target_socwithrest_v3 = False, because
raw.invertor.discharge_target_soc_1is present (value0) andread_discharge_target()returns
0, notNone. Fixed by gating both the publish and the auto-config claim on v3 — the claim
requires v3 on every discovered inverter, mirroring the pause-key rule.
Location: apps/predbat/givtcp.py:247
main gated the whole export-target block on if self.rest_data and self.rest_v3. Here
discharge_target_soc is in GIVTCP_AUTO_CONFIG_KEYS unconditionally, and publish_data emits the
entity whenever read_discharge_target() finds raw.invertor.discharge_target_soc_1 (present in v2
raw dumps) and the model is not in DISCHARGE_TARGET_UNSUPPORTED_MODELS.
Failure: on a v2 install, every force-export cycle POSTs to a non-existent /setDischargeTarget,
burns INVERTER_MAX_RETRY_REST × (1s + 2s) of blocking sleep plus a runAll each, then
record_status(had_errors=True) — reviving the permanent every-cycle rewrite loop that #4517 was
meant to end.
Fix direction: gate the discharge_target_soc publish and auto-config on rest_v3, as main did.
12. The GH#4826 clamp reads a hard-coded min: 4, not a device bound ✅ FIXED
Real, but the clamp itself is protective and the 4 is right for GE.
set_reserve()verifies
result == targetexactly, so asking below the device floor produces precisely the GH#4826 retry
loop — onmain's REST path too. Clamping prevents that.There is no device bound to discover. Nothing in a full readData dump reports one;
battery_discharge_min_power_reserve(4 in both captures) looks like a candidate but is the
discharge floor setting, not a limit on it — using it asminwould pin reserve to wherever it
already sits and stop it ever coming down.The genuine loss was a user who set
battery_min_socbelow 4 (set_reserve_minallows 0) being
silently floored. Fixed by lettingbattery_min_soclower the advertised minimum — never raise
it, since keeping a target above a policy floor isInverter.reserve_percent's job and duplicating
it would put the same rule in two places. Theinverter.pycomment no longer claims these are
always device-reported.
Location: apps/predbat/givtcp.py:76 (GIVTCP_CONTROLS['reserve']), consumed at inverter.py:1709-1725
⚠️ This one is a consequence of the merge resolution made while rebasing this PR ontomain, not
of the original contributor's work. Theif not self.rest_data:guard was deliberately dropped so
the clamp would run for GivTCP REST users — correct in itself, but the entity'sminwas not checked.
The clamp's whole purpose is to respect the inverter's real register bounds. The min it now reads
is the literal 4 in GIVTCP_CONTROLS['reserve'], and the max the literal 100.
Failure: a user with battery_min_soc: 0 and set_reserve_min: 0 (both allow 0, config.py:613)
who could previously reach a 0–3% reserve over REST is silently floored at 4%. Any GE model whose
reserve register really does go below 4 can no longer be driven there. The clamp's comment still
claims it is respecting "the inverter's own register bounds".
Fix direction: publish the inverter's real register bounds on the reserve entity, or publish no
min/max at all so the clamp is a no-op rather than a fabricated constraint.
13. battery_voltage reads back its own published entity ✅ FIXED
Confirmed.
mainhad identical code, but therebattery_voltageresolved to the user's own
sensor; hereautomatic_configpoints it at the sensorpower_readings()feeds, closing the loop.
publish_dataruns beforeautomatic_configon the first poll, so the value freezes at whatever
that first cycle produced — 52.0 where the user had configured nothing.v2's Power block genuinely has no
Battery_Voltage(absent incases/rest_v2.json; v3 reports
53.65), so there is nothing to read. Fixed by returningNoneon v2, publishing no sensor, and
splittingbattery_voltageout of the power keys so it is claimed only where one was actually
published — the same "claim only what you publish" rule as findings 3, 7 and 16. A v2 fleet keeps
the user's own voltage sensor.
Location: apps/predbat/givtcp_rest.py:128
For non-v3 (rest_v3 False), power_readings() returns get_arg("battery_voltage", default=52.0).
GIVTCP_AUTO_CONFIG_POWER_KEYS includes battery_voltage, so after automatic_config that key
resolves to sensor.<prefix>_givtcp_N_battery_voltage — the entity publish_data writes
power['battery_voltage'] into.
Failure: from the second poll onwards the component reads back its own last publication. The value
freezes at whatever the first cycle produced (52.0 if the user's own entity had already been claimed),
and the user's real voltage sensor named in apps.yaml is never read again.
Fix direction: read the user's original apps.yaml value, captured before automatic_config
rebinds the key — or exclude battery_voltage from auto-config on v2.
14. Unguarded rest_data indexing aborts the whole publish ✅ FIXED
Confirmed by test — deleting
Target_SOCreally did raise a bareKeyErrorout of
publish_data. Both real captures carry everyControlfield, so this needs a GivTCP version or
partial response that omits one; the blast radius is what makes it worth fixing regardless.One correction: the review said the failure is "logged as a generic component error rather than
pointing at the missing field".ComponentBase.start()logsstr(e)plus a traceback, and a
KeyError's string is the key name — so the field was named. What was missing was which
inverter it came from.Fixed in three parts: the three
Controlreads use.get()and returnNonewhen absent;
scheduled_charge_enable/scheduled_discharge_enableare no longer published as a fabricated
"off"when unknown, and are claimed per key on publication like the discovery keys; and each
inverter's publish is wrapped so one bad snapshot cannot starve the rest of the fleet, logging the
inverter and its URL.
Location: apps/predbat/givtcp.py:234
rest.target_soc does float(rest_data["Control"]["Target_SOC"]); rest.charge_enable_time and
discharge_enable_time do rest_data["Control"]["Enable_Charge_Schedule"] — all unguarded.
read_data() only checks that a top-level "Control" key exists, not its contents.
Failure: a GivTCP version or partial /readData response that omits Target_SOC raises out of
publish_data, up through run(), into ComponentBase.start()'s catch-all. Everything published
after line 234 for that inverter — the reserve, all four window selects, soc_kw, soc_max,
battery_calibration, the power block — and every entity of inverters n+1..N is left at its
previous or never-published value, while the exception is logged as a generic component error rather
than naming the missing field.
Fix direction: .get() with explicit handling, and wrap each inverter's publish so one bad
snapshot cannot starve the rest of the fleet.
15. update_status() still does a full REST read nothing consumes ✅ FIXED
Confirmed.
rest_datais consumed only in__init__— which runs first and does its own read
— so theupdate_statusrefresh fed nothing. Removed, taking a blocking GET per inverter per
5-minute cycle out of the main planning loop, along withread_data()'s 20s + 40s + 40s retry
ladder whenever GivTCP is slow.The stale block comment is corrected. What genuinely remains on REST is narrower than it claimed:
the GivTCP version/firmware/serial metadata, and the rawBattery_Power_Reserveread that
deliberately differs from the entity path by reporting what the inverter is actually set to without
applyingbattery_min_soc.
rest_v3onInverteris indeed write-only now, but left in place — it is trivial dead state and
removing it buys nothing.
Location: apps/predbat/inverter.py:1313
After the diff, self.rest_data is only consumed at inverter.py:400 (reserve_percent_current, in
__init__, before update_status runs). grep shows no other reader in the tree.
Failure: an HTTP GET per inverter per 5-minute cycle on top of the component's own 60s poll. When
GivTCP is slow it burns up to 20+40+40+40s of Inverter.sleep() inside the main planning loop
and fires record_status(..., had_errors=True) for data that is then discarded. self.rest_v3
(lines 186/284) is likewise now write-only in inverter.py.
The block comment at lines 264-270 is also stale — it claims REST is "kept alive for … pause_mode /
inverter_mode … and the #4517 discharge-target unsupported-model check", all of which the component
now publishes as entities.
Fix direction: drop the read (keeping whatever __init__ genuinely needs) and correct the
comment. Lowest risk of the remaining set, and removes blocking I/O from the planning loop.
16. Battery state of health was never used ✅ FIXED (as a feature, not a regression)
Not from the original review — found while discussing finding 4.
My first write-up of this was wrong. I assumed
Invertor_Details.Battery_Capacity_kWhwas the
battery's current capacity andraw.invertor.battery_nominal_capacityits design capacity, and
reported that the refactor had collapsed the two and neuteredbattery_scaling_auto. The real
captures incoverage/cases/rest_v{2,3}.jsondisprove that: they are the same design figure in
different units — 186.0 Ah / 19.53125 == 9.5232 kWh, and 19.53125 is 1000/51.2, the Ah→kWh
conversion at these packs' 51.2 V nominal. Their ratio is always exactly 1.0.nominal_capacity
was the design capacity on bothmainand the branch, so nothing was broken.
The actual gap: GivTCP reports real state of health and Predbat has never read it. Each battery
module carries Battery_Capacity and Battery_Design_Capacity (Ah) under Battery_Details — flat
on v2, nested under Battery_Stack_N on v3. The v2 capture shows 184.82 of a 186.0 design (SoH
0.9937); GE Cloud's own test uses that same battery and ratio. Predbat left battery_scaling at the
user's manual value and relied on battery_scaling_auto to infer degradation from history instead.
Fixed by adopting GE Cloud's model (gecloud.py:623-650, :1157, :1169):
soc_maxcarries the design capacity (Battery_Capacity_kWh, which already is that).battery_soh=min(Σ Battery_Capacity / Σ Battery_Design_Capacity, 1.0), walked across modules
and both version layouts. Asserted against both real captures.battery_dod= depth of discharge — GivTCP does not report it, so it defaults to1.0and can be
set per inverter with the newgivtcp_battery_dodapps.yaml key.battery_dod_soh=soh × dod, andbattery_scalingis auto-configured (viaset_arg_auto, so a
manual value logs a displacement note) to point at it.
nominal_capacity stays the nameplate figure, so battery_scaling_auto and degradation are
unaffected; the BMS's own SoH now feeds battery_scaling instead of that being left at 1.0. Where no
per-module capacities are reported, no scaling is claimed rather than displacing the user's own value
with a fabricated 1.0 — which is exactly what deriving SoH from the two design figures would have done.
The expert switch keeps its meaning: with it on the health derate is suppressed.
This is a new feature, not refactor parity — planned battery size will drop by the measured SoH
(~0.6% on the captured battery) for GivTCP users who have not set battery_scaling themselves. Worth
stating in the PR, since the rest of the branch argues behaviour preservation.
A side benefit: nominal_capacity()'s docstring now answers main's XXX: Where does 19.53125 come from? — it is 1000/51.2, assuming a 51.2 V pack, which the v2 capture confirms.
17. The #4517 unsupported-model guard matches neither real capture ✅ FIXED
Not from the original review — found while verifying finding 11. Pre-existing on main, which uses
the identical expression, so this is not a regression introduced by the refactor.
DISCHARGE_TARGET_UNSUPPORTED_MODELS = ("Ac", "Hybrid_gen1", "Hybrid_gen2") is tested against
rest_data["raw"]["invertor"]["model"]. What the committed captures actually contain:
| capture | key | value |
|---|---|---|
rest_v2.json |
raw/invertor/inverter_model |
'Hybrid' |
rest_v3.json |
raw/invertor/model |
'2' |
So on v2 the key the code reads is absent (a different key holds the model), and on v3 it is the
string '2'. Neither matches the tuple, so the #4517 guard never fires on either capture.
The constant's own comment says the values come from github.com/DJBenson/giv-firmware and are "not
independently confirmed on real Gen2 hardware yet", so they were inferred rather than observed. This
needs a capture from an affected inverter (an AC or Gen1/Gen2 Hybrid) before it can be fixed —
guessing at the mapping risks disabling the export target for people it currently works for, or
leaving it enabled for the people #4517 was raised by.
Finding 11's v3 gate narrows the exposure regardless: v2 no longer publishes the control at all.
2 participants
Supersedes #4649 and #4739, combining both into a single branch based on
mainrather than a stack.All the implementation work here is @chalfontchubby's (Rik Allen) — the 8 original commits are carried over unchanged, with authorship intact. This PR only adds a merge of current
mainon top, plus the conflict resolution that required.Why a new PR
#4739 was based on #4649's branch, not
main. Both branches had fallen ~217 commits behind, and because GitHub computes a stacked PR's diff against its base branch, bringingmainin made #4739 render as 121 files / +36,867 instead of its real 7 files / +660. Flattening the stack ontomainkeeps the diff honest and lets the whole refactor be reviewed and merged as one unit — which is how #4649 said it was intended to land anyway.What it does
Moves GivEnergy/GivTCP REST handling out of
inverter.pyinto a proper component.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, so theif self.rest_api: ... else: ...special-casing goes away.It reads the existing
givtcp_restkey directly, so it auto-activates and existing users need no apps.yaml changes.Combined, the two PRs take
inverter.pyfrom parsing GivTCP's REST blob to not knowing about inverter models at all. See #4649 and #4739 for the full per-commit rationale, the judgement calls, and the two bugs found on the way — that write-up is worth reading and is not repeated here.Merge conflict resolution
Two conflicts, both in the
mainmerge:inverter.py,adjust_reserve.mainadded the GH#4826 device min/max register clamp behind anif not self.rest_data:guard. This branch deletes the REST write path entirely and routes reserve writes through the component-published entity, so the guard was dropped and the clamp kept. Left in, it would have skipped the clamp and passedreserve_entity=Noneintowrite_and_poll_valuefor GivTCP REST users, sincerest_datastill exists here as a read path.test_inverter.py. Kept this branch's removal of the REST-modeadjust_charge_windowassertions;main's only change there was anis not Truelint fix to code this PR deletes.Testing
Full
run_allgreen (298 tests, 159s) andrun_pre_commitclean on the merged tree, includingmain's fouradjust_reserve_device_*tests that cover the resolution above, and all 31givtcp_componenttests.Still no validation against real hardware — carried over from #4649. Draft until someone dogfoods it on a live GivEnergy system. The reserve-clamp path is the thing to exercise first, since it is the one behaviour that changed in the merge.