Compose EV chargers across components via a registry - #4928
Conversation
Predbat has no charger concept - only a flat, index-keyed list of "cars" (car_n, num_cars) where each index is really a car-charger pair. Five integrations autoconfigure into the same shared args, and they collide. Two distinct defects: 1. List replacement. Gateway, GivEnergy Cloud, Ohme and myenergi each assign their OWN discovered list to car_charging_planned/_energy/ _power. The last component to run wins; every earlier component's chargers vanish from the plan. 2. num_cars is a max, not a sum. Even the raise-only discipline in gecloud cannot compose: one GivEnergy charger plus one Ohme charger yields num_cars=1, and both claim slot 0. This adds apps/predbat/charger_registry.py. Components stop assigning car_charging_* directly and call register_chargers(source, entries) - an atomic, idempotent per-source replace. The registry keys chargers on (source, device_id), allocates a slot each, and materialises the flat args once. It also exposes slot_for(source, device_id), and that is why the control loops are here too. Every one of them previously assumed its own charger was car 0, or that its Nth charger was car N (myenergi's controlled_zappis docstring stated the invariant outright). Allocating slots across components without fixing those loops would start or stop a physical charger from another car's plan, so discovery and control are migrated together for all four components. Slot identity is carried through to the plan. A charger reassigned to a different slot could otherwise consume the previous occupant's published plan, so the registry's allocation version travels from configuration fetch through to plan publication and every control loop waits for a plan built from the current mapping. Each car's published window list is also kept separate; it was shared across loop iterations, so later cars inherited earlier cars' windows. Behaviour changes worth calling out in review: - num_cars is no longer raise-only. It is derived from registered chargers and floored at any value an external source claims, via a new set_external_num_cars() that Octopus and Kraken use for their IOG car counts. It can decrease when a charger source empties. - Charger N is no longer car N. Slots are allocated across all components by (source, device_id) sort, legacy slots first. Adding a source renumbers later chargers, and per-car apps.yaml settings are slot-addressed, so they follow the position. - Hand-written car_charging_* config now composes with autodiscovery instead of being overwritten: it is pre-registered as legacy slots, preserved verbatim (interior holes included), and discovered chargers append after it. Installs on the stock template are unchanged - an unresolved "re:" entry is treated as unconfigured. - Measurements are composed rather than clobbered. Ohme keeps its energy and power contribution when another component registers first or discovery repeats, and legacy and discovered measurements of the same charger are counted once, matched on exact entity or a bounded manufacturer serial. - set_arg_auto's apps.yaml-override note no longer fires for charger keys, because config is merged rather than discarded. - gateway_evc_control without gateway_evc_automatic now warns once rather than silently doing nothing. - EV commands carry charge_point_id. Older gateway firmware falls back to firstConnected(), so it degrades to the previous behaviour. Known limitations, documented rather than fixed here: the gateway's 6-character entity slug is not collision-proof, so two charge points with matching slug tails share an entity namespace; alphaess still writes car_charging_energy/_power directly and is not migrated; component restart leaves stale registrations, as it left stale args before. Tests: 62 registry regressions registered in unit_test.py's TEST_REGISTRY so they run under --quick, plus regressions in the ohme, myenergi, ge_cloud, gateway, kraken and octopus suites covering each control loop waiting after reassignment and rejecting a plan published for an older allocation.
0b7720e to
569f325
Compare
|
Updated: the review fixes that were in #4934 are now folded into this PR ( Overlap worth deciding before either lands: #4880 addresses They aren't incompatible, but merging both as-is would leave two mechanisms for one problem, and |
|
Thanks for this — the core design holds up well. Identity on Your test selection is green on the branch for me too. The findings below are things the tests don't cover; I reproduced each by driving the registry directly. 1. Hand-written
|
… dedupe, unlocked config exposure Addresses maintainer review on the charger registry PR. 1. Hand-written car_charging_soc / car_charging_now were being deleted. The all-or-omit rule wrote set_arg(arg, None) - which removes the key - whenever any component slot had no value for a slot-aligned field. But myenergi supplies neither soc nor now, ohme supplies no now, and gateway supplies no now under gateway_evc_control, so the gap is permanent on those installs. Before the registry existed none of those components ever wrote these keys, so a user's apps.yaml value simply survived discovery; after it, discovering a Zappi silently removed their car_charging_soc. fetch.py:1383 then read the SoC as 0.0 and Predbat planned a full charge into a possibly-full car, and with car_charging_now gone fetch.py:2400 fell back to "no". It also dropped the gateway car's own car_charging_now - gateway being the only source that populates it - as soon as an ohme registered alongside, which is essentially every mixed install. The key is now left exactly as it stands: not written, and never cleared. That reproduces the pre-registry outcome - a list that is short for the new car count, plus fetch.py's own out-of-range warning for the cars past its end - without the registry inventing or destroying anything. This holds equally when the standing value is one the registry itself wrote for an earlier composition: a stale value is recoverable, a deleted one is not. The Warn line is kept but now says the key is being left as it is, and is latched per field so a permanent gap costs one line rather than one per rediscovery. docs/car-charging.md documents the rule and the log line, which it did not before. 2. The gecloud half of the legacy dedup never fired. _names_a_discovered_charger matched the raw device_id case-sensitively. GivEnergy Cloud serials are upper case (EVC123456, WE1913G005) while HA entity ids never are, and async_automatic_config_evc lower-cases the serial when it builds an entity name - so a gecloud serial could never match and the duplicated charger stayed as its own car. myenergi serials are numeric, which is why the tests passed and the bug was invisible. The search is now case insensitive; the word-boundary rule is unchanged and still keeps WE1913G005 from claiming a WE1913G0055 charger. 3. A blocked control loop said nothing. charger_plan_ready() gates all four control loops and every wait on it is a bare return, so "my charger stopped responding" left nothing in the log to explain it. The wait now logs one line naming the component, latched the same way as the gateway's _ev_no_slot_warned: cleared when a plan for the current allocation arrives, so a later recurrence is reported afresh and a plan that never comes costs one line, not one per cycle. 4. The registry lock was held across blocking HA I/O. materialise() held the lock through expose_config() for car_charging_rate, which reaches ha.set_state and a synchronous HTTP POST. replace_source() runs on component threads - the gateway's straight from an MQTT callback - while the main loop waits on the same lock in snapshot_generation(), so every gateway registration blocked the main loop for as long as HA took to answer. The (item, value) pairs are now composed under the lock and written after it is released. 5. Moving plan = [] inside the per-car loop in publish_car_plan fixes a real multi-car bug that predates this branch: the window list was built once outside the loop, so car 1's published "planned" attribute carried car 0's windows as well as its own and car 2's carried both. It matters more now because the new control loops read exactly that attribute to decide whether to charge, so a leaked window starts a charger outside its own plan. It now has a regression test of its own, independent of the registry. Tests: eight new regressions - hand-written soc/now surviving a myenergi registration, a second source not deleting the first's now, the key being written again once the gap closes, the gap log firing once per gap, an uppercase gecloud serial deduping (and still respecting the word boundary), the control-loop wait line firing once and clearing, and the per-car plan isolation. Seven of the eight fail against the previous commit. test_car_charging_now_omitted_when_controlling now asserts the key is left alone rather than nulled, which is what the pre-registry gateway path did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — that's a sharp review, and #1 was a real regression. All four are fixed in 1. Hand-written This one also contradicted the PR's own headline claim, so thank you for catching it — the description overstated what composition preserved. 2. gecloud dedupe never fired. Correct: uppercase GivEnergy serials against lowercase entity ids. Now matched case-insensitively, with an uppercase-serial fixture to pin it. The boundary-collision guard stays green — the numeric myenergi serials were exactly why the tests didn't show this. 3. Shared gate, undiagnosable. Each control loop now emits a one-shot line while it waits for a plan built for the current allocation, cleared when the plan arrives, so a stalled charger says why rather than returning silently. No per-cycle spam. 4. Lock held across HA I/O. The 5. Verification: 261 targeted tests, 297 gateway, and The #4880 question from my earlier comment is still open whenever you want to decide it. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical charger-slot and Gateway routing issues could control or plan against the wrong vehicle.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces a central EV charger registry so multiple integrations can compose charger configuration and control distinct car slots.
Changes:
- Adds deterministic charger registration, legacy configuration merging, and external car-count floors.
- Migrates Gateway, GivEnergy Cloud, myenergi, and Ohme discovery/control.
- Adds regression tests and documentation.
Review comments:
- Critical (2 votes) —
charger_registry.py:500: Slot-aligned lists can retain stale positions when sorting introduces a component gap, potentially assigning telemetry such as SoC to the wrong car. Preserve only unchanged slots or rebuild/invalidate affected lists. - Critical (1 vote) —
gateway.py:549: Older firmware may ignorecharge_point_id, causing multiple per-device commands to target the first charger. Detect addressing support and restrict multi-charger control when unsupported. - Moderate (1 vote) —
gateway.py:1421: Empty telemetry does not clear prior Gateway registrations, and removal is not detected by_needs_reconfigure(). Detect charger-set changes and register an authoritative empty set. - Moderate (1 vote) —
ohme.py:490: A captured legacy aggregate prevents Ohme energy and power from being registered, omitting its charging data. Distinguish legacy registry aggregates from runtime direct writers.
File summaries
| File | Description |
|---|---|
docs/components.md |
Documents shared charger slots. |
docs/car-charging.md |
Explains allocation, composition, and legacy behavior. |
docs/apps-yaml.md |
Updates charger configuration guidance. |
apps/predbat/utils.py |
Excludes the registry from debug serialization. |
apps/predbat/unit_test.py |
Registers the charger registry tests. |
apps/predbat/tests/test_ohme.py |
Tests Ohme registration and slot-aware control. |
apps/predbat/tests/test_octopus_misc.py |
Tests Octopus car-count claims. |
apps/predbat/tests/test_myenergi.py |
Tests slot-aware Zappi control. |
apps/predbat/tests/test_kraken.py |
Tests Kraken claim lifecycle. |
apps/predbat/tests/test_ge_cloud.py |
Tests GivEnergy registry integration. |
apps/predbat/tests/test_gateway.py |
Tests Gateway multi-charger behavior. |
apps/predbat/tests/test_charger_registry.py |
Adds registry and composition coverage. |
apps/predbat/predbat.py |
Initializes and seeds the registry. |
apps/predbat/output.py |
Publishes isolated per-car plans. |
apps/predbat/ohme.py |
Registers and controls Ohme by allocated slot. |
apps/predbat/octopus.py |
Manages Octopus car-count claims. |
apps/predbat/myenergi.py |
Registers and controls Zappis by allocated slot. |
apps/predbat/mock_base.py |
Adds registry support to test mocks. |
apps/predbat/kraken.py |
Manages Kraken car-count claims. |
apps/predbat/gecloud.py |
Registers GivEnergy chargers centrally. |
apps/predbat/gateway.py |
Adds multi-charger registration and routing. |
apps/predbat/fetch.py |
Snapshots charger-map generation. |
apps/predbat/component_base.py |
Adds shared registry helpers. |
apps/predbat/charger_registry.py |
Implements charger composition and slot allocation. |
.cspell/custom-dictionary-workspace.txt |
Adds test vocabulary. |
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| missing = [entry.key() for entry, value in zip(entries, values) if not value and entry.source != LEGACY_SOURCE] | ||
| if missing: | ||
| # A component-caused gap cannot be expressed: None fails validation and a | ||
| # placeholder would be read as a real entity. So the key is left exactly as it | ||
| # stands - not written, and above all not cleared. |
| await self._send_ev_command({"action": "SetChargingProfile", "current_a": int(current_a), "charge_point_id": cp_id}) | ||
| await self._send_ev_command({"action": "RemoteStartTransaction", "id_tag": "predbat", "charge_point_id": cp_id}) | ||
| else: | ||
| self.log(f"Info: GatewayMQTT: EVC stop charging for '{cp_id}' (car {slot})") | ||
| await self._send_ev_command({"action": "RemoteStopTransaction", "charge_point_id": cp_id}) |
| chargers = list(status.ev_chargers) | ||
| if not chargers: | ||
| return |
| existing = self.get_arg("car_charging_energy", default=None, indirect=False) | ||
| if isinstance(existing, list) and len(existing) == 1: | ||
| existing = existing[0] | ||
| # Ohme already owns the energy figure when the entity configured is its own, whether that | ||
| # was set here on a previous run or written into apps.yaml by hand (#4715 review) | ||
| if (not existing) or (isinstance(existing, str) and existing.startswith("re:")) or existing == ENERGY_TODAY_ENTITY: | ||
| self.set_arg_auto("car_charging_energy", ENERGY_TODAY_ENTITY) | ||
| # Live charge power, for the web power flow diagram and the predbat.car_charging_power | ||
| # sensor. Deliberately tied to the same decision as the energy sensor above: the two | ||
| # have to describe the same charger, so when another charger owns the energy figure | ||
| # Ohme's own power reading is left out rather than reported beside it. | ||
| self.set_arg_auto("car_charging_power", POWER_WATTS_ENTITY) | ||
| else: | ||
| owns_energy = self.base.charger_registry.owns_aggregate("energy") or (not existing) or (isinstance(existing, str) and existing.startswith("re:")) or existing == ENERGY_TODAY_ENTITY |
…way command keys, release removed chargers, compose ohme with legacy aggregates
|
All four Copilot findings addressed in Positionally stale slot lists. @springfall2008's prescription was to leave the key alone when it holds only legacy values; I broadened that to retain registry-owned lists too, and the broadening was unsafe. Ohme registering alone writes Fixed by trimming a composed list at the first gap rather than keeping or clearing it wholesale. A prefix is always positionally valid, and because legacy slots sort first they are always inside the kept head — so the trim drops exactly what the registry invented and never the user's own config. Both of your earlier reproductions stay green, which a blanket clear would have broken. Both gateway command keys. Rather than feature-detecting and disabling multi-charger control, the commands now carry Removed chargers are now released. Ohme composes with a legacy aggregate. It was backing off whenever any
|
The problem
Predbat has no charger concept — only a flat, index-keyed list of "cars" (
car_n,num_cars) where each index is really a car-charger pair. Five integrations autoconfigure into the same shared args, and they collide in two distinct ways:1. List replacement. Gateway, GivEnergy Cloud, Ohme and myenergi each assign their own discovered list to
car_charging_planned/_energy/_power. The last component to run wins; every earlier component's chargers vanish from the plan._register_ev_car()ingateway.pyadmits it in its own comment: "we overwrite the first charger only; multi-charger support needs work".2.
num_carsis a max, not a sum. Even the raise-only discipline ingecloud.py— whose comment correctly worries about "another component [that] may already have registered cars of its own" — cannot compose: one GivEnergy charger plus one Ohme charger yieldsnum_cars = 1, and both claim slot 0.The change
apps/predbat/charger_registry.py. Components stop assigningcar_charging_*directly and callregister_chargers(source, entries)— an atomic, idempotent per-source replace that covers add, update and removal. The registry keys chargers on(source, device_id), allocates a slot each, and materialises the flat args once.It also exposes
slot_for(source, device_id), and that is why the control loops are in this PR too. Every one of them previously assumed its own charger was car 0, or that its Nth charger was car N —myenergi.controlled_zappis()'s docstring stated the invariant outright ("Zappi N is the same car as auto-config's Nth"). Reallocating slots without fixing those loops would start or stop a physical charger from another car's plan, so discovery and control are migrated together for all four components.Behaviour changes to review carefully
num_carsis no longer raise-only. It is derived from registered chargers and floored at any value an external source claims, via a newset_external_num_cars()that Octopus and Kraken call for their IOG car counts. It can decrease when a charger source empties.docs/car-charging.mdupdated accordingly.(source, device_id)sort, legacy slots first. Adding a source renumbers later chargers, and per-car apps.yaml settings are slot-addressed, so they follow the position rather than the car. Pinned bytest_adding_an_earlier_sorting_charger_shifts_later_slots.car_charging_*config now composes with autodiscovery instead of being overwritten — pre-registered as legacy slots, preserved verbatim (interior holes included), with discovered chargers appended after. Installs on the stock template are unchanged: an unresolvedre:entry is treated as unconfigured.set_arg_auto's apps.yaml-override note ([Solis Cloud] batteryHealthSoh of 0 from the API silently zeroes soc_max and causes unbounded grid charging #4494 / fix(solis): guard battery_scaling against a documented 0% SOH API response #4500) no longer fires for charger keys, because config is now merged rather than discarded — the note would mislead.gateway_evc_controlwithoutgateway_evc_automaticnow warns once rather than silently doing nothing. Theinitialize()docstring already declared control requires the automatic flag; nothing enforced or reported it.charge_point_id. Older gateway firmware falls back tofirstConnected(), so it degrades to the previous behaviour rather than misrouting.Known limitations (documented, not fixed here)
alphaess.pystill writescar_charging_energy/_powerdirectly and is not migrated — last-writer-wins with the registry on those two keys, as before this PR.Testing
unit_test.py'sTEST_REGISTRYso they run under--quick.auto_config()regex-resolution sequence before pre-registration.unit_test.py --quickgreen:charger_registry,ohme,myenergi,ge_cloud,gateway,kraken,octopus_misc,web_ifall PASSED.