Repository navigation
Conversation
springfall2008
left a comment
There was a problem hiding this comment.
Automated comment from the triage bot.
Clean review of PR #5808 at 3c2dac7 - 0 findings inline, 0 in the body.
What I checked: the planned-sensor rule in charger_control_dispatch_active() / charger_control_planned_sensor() side by side against Fetch.car_planned_slot_entity() and its caller (they match, including the completed-from-both reads, the single-value-is-car-0's list handling, the re: and self-reference guards, and the octopus_intelligent_slot_action_config carve-out); the new filtered arm in charger_control_drive_one() against every pre-existing leave-alone rule (the Ohme claim for car 0, never driving a sensor-said charge point, the discovery waits - all preserved, verified against main's copy of the function); charger_control_declared_device()'s arg handling against get_arg() / resolve_arg() (list wrapping, optional_entries, templating); the APPS_SCHEMA entry's allowed validation (it fires for the string type; validate_config() only enforces allowed there and in the integer branch); the new tests' claims (both fail on main - there charger_control_dispatch_active() reads the raw slot sensor and charger_control_declared_device() does not exist); the three docs files (anchors valid, the setting documented, the Hypervolt recipe covers the new combination); and each deliberate behaviour in the PR body against the code (no slot-state fallback with a planned sensor set, snake_case attributes only, the duplicated planned-sensor rule kept).
The one design change worth naming, since it is broader than the issue title: octopus_intelligent_charger_follows_car expert mode now also follows the planned sensor where one is set, rather than the raw slot sensor. That is documented in the car-charging and Hypervolt docs and reads as the coherent rule - the charger follows the same plan the car is planned on - so I see no problem with it, just flagging it in case anyone reading the issue expected the raw follow to survive there.
|
Not sure I want to make this mandatory to set, probably need it automatic |
3c2dac7 to
65894a4
Compare
|
Made it automatic, and |
|
Posted by Claude on behalf of @chalfontchubby. Short version: the planned-slot fix (part 1) is a regression fix and I think it should go in. Our Hypervolt port (#5791) introduced the regression. For the new Part 1: agreed. Before #5791 the Hypervolt followed only Predbat's car plan, which honours Part 2: the automatic route. On "probably need it automatic": Predbat's own Octopus and Kraken components already publish Detection doesn't decide the behaviour, though. Once Predbat can tell that Octopus drives this Hypervolt, today's rule leaves that charger entirely to Octopus. The filter here would then stop working. tico24's setup works today only because the integration can't say. So the real decision is whether a user-filtered |
|
Posted by Claude on behalf of @chalfontchubby. A follow-up on the automatic route. Predbat can already tell which device Octopus drives for Octopus Energy integration users, without waiting for #1917. The integration's dispatching sensor fills in
Shared with The caveat from my last comment still applies: once Predbat can tell that Octopus drives tico24's Hypervolt, today's rule leaves it alone and the planned-slot filter stops working. So the veto question needs settling before or alongside that change. |
The shared charger loop also runs a charger in any dispatch on octopus_intelligent_slot, so a dispatch the planned sensor leaves out still started it, although the car plan ignores those. Planned dispatches now come from octopus_intelligent_planned_slot where it is set, and completed ones from both sensors, as fetch.py reads them. With a planned sensor set there is no fallback to the slot sensor on/off state, which would put back the dispatches the planned sensor filtered out.
65894a4 to
44242cb
Compare
|
Split as you suggested. This PR is now just the planned-slot fix, and the device detection is out, so that stays with your #3034 work. I'll put the veto rule up as its own small PR on top of this one, so it can be settled before your detection lands, with what my system has shown about stopping the charger inside dispatches. |
Fixes #5807
What and why
When the shared charger control drives a charger, it also runs it in any dispatch on
octopus_intelligent_slot. Withoctopus_intelligent_planned_slotset, the docs say planned slots onoctopus_intelligent_slotare ignored for that car, and the car plan does ignore them, but the charger didn't. It now takes planned dispatches from the planned sensor and completed ones from both, asfetch.pydoes. With a planned sensor set it no longer falls back to the slot sensor's on/off state. Before #5791 the Hypervolt followed only the car plan, so for Hypervolt users this is a regression from the move to the shared loop.Who this changes things for
Users with
octopus_intelligent_planned_slotset whose charger Predbat drives: Octopus drives the car andoctopus_intelligent_charger_follows_caris on, or Predbat can't tell and the control setting istrue. Their charger now follows the planned sensor rather than the slot sensor. Nobody without a planned sensor is affected.Deliberately not changed
fetch.pybuilds the car plan from it too, and falling back would run the charger in exactly the dispatches the user filtered out. Predbat's own plan windows still apply.fetch.pyalso reads the camelCase ones.Fetch.car_planned_slot_entity()plus itsoctopus_intelligent_slot_action_configcheck, in the same waycharger_control_dispatch_sensor()already readsoctopus_intelligent_slotitself.is_chargerwork. Whether Predbat may stop a charger Octopus drives is a separate question, discussed in the comments.Testing
test_planned_slot_sensor_replaces_the_slot_sensor_planintest_car_charger_control.pycovers the planned sensor replacing the slot sensor's plan, completed dispatches from it, regex and same-sensor values, and the action config. It fails onmain.car_charger_control,hypervolt,ohme,myenergiandge_cloudpass. pre-commit passes on the changed files.octopus_intelligent_planned_slot.