Repository navigation
fix command timing problem - #3271
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the command handling to avoid timing/staleness issues by reading and mutating the live SubData structures (instead of control.data.data) when creating/removing hierarchy items and when copying templates/plans.
Changes:
- Switch multiple command paths from
data.data.*toSubData.*(hierarchy, chargepoint templates, EV templates, charge templates). - Adjust chargepoint-template copy behavior to reassign autolock plan IDs and publish an updated autolock max-id.
- Update charge-template source resolution when changes come from the theme (chargepoint-local template vs persistent vehicle template).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SubData.cp_template_data[f'cpt{payload["data"]["template"]}'].data.autolock.plans.remove(plan) | ||
| break | ||
| Pub().pub( | ||
| f'openWB/chargepoint/template/{payload["data"]["template"]}', |
There was a problem hiding this comment.
This publishes the updated template to openWB/chargepoint/template/<id> (without the set/ prefix), while other mutations (e.g. addAutolockPlan, addChargepointTemplate) publish to openWB/set/chargepoint/template/<id>. If openWB/set/... is the intended write-path, this will bypass it and may not persist/update consistently; publish to the same openWB/set/chargepoint/template/<id> topic here as well.
| f'openWB/chargepoint/template/{payload["data"]["template"]}', | |
| f'openWB/set/chargepoint/template/{payload["data"]["template"]}', |
| self.max_id_charge_template = new_id | ||
| # check if "payload" contains "data.copy" | ||
| if "data" in payload and "copy" in payload["data"]: | ||
| new_charge_template = copy.deepcopy(data.data.ev_charge_template_data[f'ct{payload["data"]["copy"]}'].data) | ||
| new_charge_template = copy.deepcopy(SubData.ev_charge_template_data[f'ct{payload["data"]["copy"]}'].data) | ||
| new_charge_template.name = f'Kopie von {new_charge_template.name}' | ||
| for plan in new_charge_template.chargemode.scheduled_charging.plans: | ||
| plan.id = self.max_id_charge_template_scheduled_plan + 1 |
There was a problem hiding this comment.
When copying a charge template, the code updates plan.id values by incrementing self.max_id_charge_template_scheduled_plan / self.max_id_charge_template_time_charging_plan, but later publishes the broker topics openWB/set/command/max_id/charge_template_scheduled_plan and .../charge_template_time_charging_plan with new_id (the charge template id). This desynchronizes the max-id topics from the actual plan ids and can lead to id collisions when creating the next plan; publish the updated plan max-ids instead.
| ProcessBrokerBranch(f'chargepoint/{cp_id}/').remove_topics() | ||
| data.data.counter_all_data.hierarchy_remove_item(cp_id, ComponentType.CHARGEPOINT) | ||
| Pub().pub("openWB/set/counter/get/hierarchy", data.data.counter_all_data.data.get.hierarchy) | ||
| SubData.counter_all_data.hierarchy_remove_item(cp_id, ComponentType.CHARGEPOINT) |
There was a problem hiding this comment.
hierarchy_remove_item is defined as (id_to_find: int, keep_children: bool = True), but this call passes ComponentType.CHARGEPOINT as the second argument. That works only incidentally (truthy enum) and is misleading; pass an explicit boolean (or omit the second argument to use the default).
| SubData.counter_all_data.hierarchy_remove_item(cp_id, ComponentType.CHARGEPOINT) | |
| SubData.counter_all_data.hierarchy_remove_item(cp_id, True) |
| # if copying a template, increase id of autolock plans | ||
| if "data" in payload and "copy" in payload["data"]: | ||
| for plan in new_chargepoint_template["autolock"]["plans"]: | ||
| plan["id"] = self.max_id_autolock_plan + 1 | ||
| self.max_id_autolock_plan += 1 | ||
| Pub().pub("openWB/set/command/max_id/autolock_plan", self.max_id_autolock_plan) |
There was a problem hiding this comment.
The new copy-path mutates autolock plan IDs and publishes an updated max_id/autolock_plan. There’s existing pytest coverage for Command (see packages/helpermodules/command_test.py), but no test currently exercises this copy behavior; adding a unit test for addChargepointTemplate that asserts (1) copied plans get new unique IDs and (2) the published max-id matches the highest assigned plan id would help prevent regressions.
* fix timing problem * review * flake8
Uh oh!
There was an error while loading. Please reload this page.