feat(config): add num_chargers so a repeated charger is caught rather than silently summed - #4880
feat(config): add num_chargers so a repeated charger is caught rather than silently summed#4880chalfontchubby wants to merge 4 commits into
Conversation
…mmed car_charging_power is the one car_charging_* key that is not one entry per car: it lists chargers and update_car_charging_power() sums them. Sitting among five keys that ARE indexed by num_cars, it reads as per-car, so a household with two cars sharing one charger naturally lists that charger twice - and Predbat then reports double the power the charger is drawing (#4879). We cannot detect this. Deduplicating identical entity ids catches the literal case but not two template sensors that both read the same charger, and there is no invariant to check the sum against - car_energy_reported_load exists because the charger may sit outside the house CT clamp. A check that caught one shape and stayed silent on the other would be worse than none, since the silence would read as validated. So ask instead. num_chargers names the concept Predbat was missing entirely - it models cars and has no notion of chargers - and lets the existing entries validation compare it against the length of car_charging_power. Unset means no check, so no existing apps.yaml changes behaviour; the shipped template sets 1, which is the common case and the one that gets this wrong. Two supporting changes: entries validation only ever rejected too FEW entries. Extra entries have always been tolerated, and newly rejecting them everywhere would fail working installs - but for a list that is summed rather than indexed an extra entry inflates the total instead of being ignored. Hence entries_exact, set only on this key. The item type check trims a list to required_entries before validating it, so a count key that is simply unset (0) trimmed the list to nothing and type-checked no items at all - which silently disabled the existing "a nonsense charger sensor is rejected" behaviour the moment this key gained an entries rule. Guarded with "if required_entries" rather than "is not None"; the same latent hole applied to every entries key whenever its count was 0. Docs say the missing part out loud: this list describes chargers, not cars, list each one once however many cars you have, and two cars sharing a charger is a single entry. Fixes #4879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tolerating a short list silently made num_chargers mean "an upper bound" rather than "how many chargers I have". A charger with no live power sensor is a real and documented setup, so a short list must not force a false count - but an entry deleted by accident under-reports the Car figure just as quietly as a repeated one over-reports it, which is the whole problem this key exists to stop. Log it without recording an arg_error, so a legitimate setup is not left showing "apps.yaml has N errors" forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
validate_config() still misses warning-on-short-count when optional_entries is used with a scalar (non-list) value, and the integer-type validation chain contains unreachable duplicated logic that should be addressed for correctness/maintainability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new configuration concept, num_chargers, to prevent EV charging power from being accidentally double-counted when car_charging_power (a summed list) contains repeated entries in multi-car / shared-charger setups.
Changes:
- Add
num_chargersand apply it tocar_charging_powerviaentriesvalidation, with a newentries_exactflag to reject extra entries for this specific summed list. - Fix
validate_config()list-truncation logic soentriesrules with a count of0don’t silently skip per-item type validation. - Update docs and add unit tests covering duplicated entries, exact matching, short-list warnings, and type-checking when
num_chargersis unset.
File summaries
| File | Description |
|---|---|
| docs/car-charging.md | Clarifies that car_charging_power describes chargers (summed), not cars, and documents num_chargers. |
| docs/apps-yaml.md | Adds num_chargers and notes car_charging_power is charger-based (not per-car). |
| apps/predbat/tests/test_web_power_flow.py | Adds regression tests for duplicated charger entries, exact-count validation, short-list warnings, and type-checking. |
| apps/predbat/predbat.py | Extends validate_config() with entries_exact, short-list warning behavior, and fixes the “count=0 truncates away validation” hole. |
| apps/predbat/config/apps.yaml | Updates the shipped template to include num_chargers. |
| apps/predbat/config.py | Adds num_chargers schema entry and applies entries_exact/optional_entries rules to car_charging_power. |
Review details
Suppressed comments (1)
apps/predbat/predbat.py:1485
- When optional_entries is true and required_entries > 1, a non-list value (single sensor) currently passes with no warning. That contradicts the intent of optional_entries here (warn-but-not-error when fewer entries are provided) and can let an under-specified config go by silently (e.g., num_chargers=2 with a single car_charging_power sensor). Consider emitting the same warning used for short lists in this branch.
elif required_entries > 1:
if not optional_entries:
self.log("Warn: Validation of apps.yaml found configuration item '{}' is not a list, but requires {} entries based on {}".format(name, required_entries, entries))
self.arg_errors[name] = "Invalid type, expected list"
errors += 1
continue
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -1556,7 +1577,7 @@ | |||
| value = self.get_arg(name, [], indirect=False) | |||
There was a problem hiding this comment.
Fixed in 6976adf — merged the second, unreachable integer/integer_list block's allowed-list check into the one branch the elif chain actually reaches, and deleted the dead duplicate. Added a regression test (threads=21) confirming an out-of-range integer is now rejected.
Posted by Claude on behalf of @chalfontchubby.
| self.log("Warn: Validation of apps.yaml found configuration item '{}' has {} entries, expected exactly {} based on {}".format(name, len(value), required_entries, entries)) | ||
| self.arg_errors[name] = "Too many entries, expected {}".format(required_entries) | ||
| errors += 1 |
There was a problem hiding this comment.
Fixed in 6976adf — arg_errors now reads "Too many entries, expected exactly N" to match the log line.
Posted by Claude on behalf of @chalfontchubby.
…lowed-list check Addresses two Copilot review findings on #4880. validate_config()'s optional_entries handling warned when a list was shorter than required_entries, but silently accepted a bare scalar the same way even though that is exactly as short (a de-facto list of 1) - e.g. num_chargers=2 with a single car_charging_power sensor passed without the "lists 1 of the 2 declared" warning a too-short list gets. Now warns the same way. Separately, the integer/integer_list branch of the expected_types elif chain was declared twice. Since the outer loop only runs the first elif whose condition matches, the second occurrence - the only one that checked spec's "allowed" list - was unreachable dead code, so an integer value outside a key's allowed set (e.g. threads: 21, only "auto" or 0-20 are declared) passed validation silently. Merged the allowed-list check into the one reachable block and deleted the dead duplicate. Tests: a scalar-vs-num_chargers case alongside the existing short-list one in test_web_power_flow.py, and a threads=21 case in test_validate_config.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…line Third Copilot review finding on #4880: entries_exact's log line says "expected exactly N" but the recorded arg_errors message just said "expected N", losing the detail that this is an exact-count check rather than an at-most one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Written by Claude, posted on behalf of @chalfontchubby.
Fixes #4879
car_charging_poweris the onecar_charging_*key that is not one entry per car: it lists chargers, andupdate_car_charging_power()sums them. It sits among five keys that are indexed bynum_cars, so a household with two cars sharing one charger naturally lists that charger twice — and Predbat reports double the power it is drawing. The reporter's diagram showed 14140 W against a Zappi reading 7.1 kW.Why not just detect it
Deduplicating identical entity ids catches the literal
[zappi, zappi]case, but not this, which is the shape a careful user is more likely to produce:Two distinct entities, one charger. We cannot see through a template sensor to its source, and there is no invariant to check the sum against —
car_energy_reported_loadexists precisely because the charger may sit outside the house CT clamp. A check that caught one shape and stayed silent on the other would be worse than none: the silence would read as "validated".So ask instead
num_chargersnames the concept Predbat was missing entirely — it models cars and has no notion of chargers at all, which is the root of the confusion. The existingentriesvalidation then compares it against the length ofcar_charging_power, catching both shapes because it stops trying to infer something invisible.apps.yamlchanges behaviour.num_chargers: 1— the common case, and the one that gets this wrong.arg_error, so a legitimate setup is never left showing "apps.yaml has N errors".Two supporting changes worth a reviewer's eye
1.
entriesvalidation only ever rejected too FEW entries. Extra entries have always been tolerated, and newly rejecting them everywhere would fail working installs. But for a list that is summed rather than indexed, an extra entry inflates the total instead of being ignored. Hence a newentries_exactflag, set only on this key, so every existing key keeps its current behaviour.2. A latent hole in the item type check. It trims a list to
required_entriesbefore validating the items:With a count key that is simply unset,
required_entriesis0, so the list was trimmed to nothing and no items were type-checked at all. That silently disabled the existing "a nonsense charger sensor is rejected" behaviour the moment this key gained anentriesrule — caught by an existing test, which is how I found it. Guarded withif required_entriesinstead. The same hole applied to everyentrieskey whenever its count was 0.Docs
docs/car-charging.mdsaid only the positive case ("one per line, if you have more than one charger"). It now says the negative out loud: this list describes chargers not cars,num_carshas no bearing on it, list each charger once however many cars you have, and two cars sharing a charger is a single entry. Plus the self-check — comparepredbat.car_charging_poweragainst the charger's own app; if it reads double, an entry is repeated.Testing
Four new cases in
test_web_power_flow: a duplicated charger againstnum_chargers: 1is an error; the same list validates atnum_chargers: 2; fewer entries than declared warns without recording anarg_error; and a nonsense sensor is still type-checked withnum_chargersunset (the truncation regression above)../run_all --quickand./run_all --test debug_casesboth pass.