Conversation
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
|
|
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 |
|
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
|
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. Review follow-ups from #4928: hand-written car_charging_soc/_now are preserved rather than deleted when a component supplies no value, and a composed list is trimmed at the first gap so a reallocated slot cannot inherit the previous car's sensor; the gecloud legacy dedupe matches serials case-insensitively; EV commands carry both target and charge_point_id so older firmware routes them to the right charge point; removed charge points are released after a debounce; ohme composes with a legacy aggregate instead of backing off; and each control loop logs once while it waits for a plan built for the current allocation.
d547739 to
87e21e2
Compare
|
Rebased onto current main ( Worth noting #4959, because it arrives at the same conclusion this PR does from the other end: "an explicit apps.yaml entry is kept exactly as written". That's the preserve-rather-than-clobber principle behind the legacy pre-registration here, so the two are aligned rather than competing. I did look at whether our aggregate handling could just use
Still happy to take direction on the #4880 overlap whenever you want to settle it — that's the only open question here. |
|
No rush on this — just flagging that it's ready whenever you have time. Rebased onto current main, CI green, and everything from your review and Copilot's is addressed. The only thing I can't resolve from this side is the #4880 overlap, and I don't want it to be a blocker for you. Unless you'd rather decide it up front, I'll work to whichever lands first:
Either order is fine by me and neither needs a decision now. Say the word if you'd prefer a different shape. |
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. Review follow-ups from #4928: hand-written car_charging_soc/_now are preserved rather than deleted when a component supplies no value, and a composed list is trimmed at the first gap so a reallocated slot cannot inherit the previous car's sensor; the gecloud legacy dedupe matches serials case-insensitively; EV commands carry both target and charge_point_id so older firmware routes them to the right charge point; removed charge points are released after a debounce; ohme composes with a legacy aggregate instead of backing off; and each control loop logs once while it waits for a plan built for the current allocation.
87e21e2 to
ab52f9f
Compare
|
Rebased onto current main ( One conflict, in Nothing else needed touching. |
|
#4880 has been closed by its author, so the overlap I raised here no longer needs a decision. This PR is rebased, mergeable and green, and nothing is waiting on another PR. That thread did raise two setups worth answering here, since this is where they're handled (thanks @gcoan, @chalfontchubby):
@chalfontchubby's suggestion of registering hand-written chargers "via the same mechanism that discovered chargers do, perhaps with checks for duplication" is close to what One thing this doesn't address: chargers split across the CT clamp. |
|
Thought a mix of some chargers visible to the ct clamp and some not might be a corner case you'd appreciate :-) |
At the moment its a global switch for all car chargers. No-one has reported an issue with that, so suggest it remains an existing documented limitation. Otherwise would have to start introducing multiple switches per charger. Of course do-able but yet more config complexity. |
|
It would not be worth implementing a per charger split - and I didn't ask for that. But having a feature like that in mind while designing the basic case is worthwhile. Don't unnecessarily add features but don't exclude the possibility of them either. |
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.