Active bat control: update power_limit_mode#3169
Active bat control: update power_limit_mode#3169ndrsnhs wants to merge 4 commits intoopenWB:masterfrom
Conversation
|
@ndrsnhs Bitte ein Rebase machen. |
benderl
left a comment
benderl
left a comment
There was a problem hiding this comment.
Bitte auch die anderen alten Bezeichnungen migrieren. "no_limit" ist nur einer von dreien.
ndrsnhs
force-pushed
the
remove-power_limit_mode
branch
from
cbd2b6d to
ce0aaa4
Compare
…core into remove-power_limit_mode
There was a problem hiding this comment.
Pull request overview
This PR bumps the datastore version and adds a new datastore migration to normalize legacy openWB/bat/config/power_limit_mode values that may still be present (e.g. no_limit).
Changes:
- Increase
UpdateConfig.DATASTORE_VERSIONfrom 119 to 120. - Add
upgrade_datastore_120to migrateopenWB/bat/config/power_limit_modeto current mode values. - Append datastore version
120after running the new migration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def upgrade(topic: str, payload) -> None: | ||
| if re.search("openWB/bat/config/power_limit_mode", topic) is not None: | ||
| mode = decode_payload(payload) | ||
| if mode == "no_limit" or mode == "limit_stop": | ||
| mode = "mode_no_discharge" | ||
| return {topic: mode} | ||
| else: | ||
| mode = "mode_discharge_home_consumption" | ||
| return {topic: mode} |
There was a problem hiding this comment.
In upgrade_datastore_120, the else branch rewrites any existing openWB/bat/config/power_limit_mode value to mode_discharge_home_consumption. This will unintentionally override already-valid values like mode_no_discharge or mode_charge_pv_production and change user configuration during migration. Only map the legacy values you want to replace (e.g. no_limit, limit_stop) and return None for already-valid/unknown values so they remain unchanged.
| def upgrade(topic: str, payload) -> None: | |
| if re.search("openWB/bat/config/power_limit_mode", topic) is not None: | |
| mode = decode_payload(payload) | |
| if mode == "no_limit" or mode == "limit_stop": | |
| mode = "mode_no_discharge" | |
| return {topic: mode} | |
| else: | |
| mode = "mode_discharge_home_consumption" | |
| return {topic: mode} | |
| def upgrade(topic: str, payload) -> Optional[dict]: | |
| if re.search("openWB/bat/config/power_limit_mode", topic) is not None: | |
| mode = decode_payload(payload) | |
| if mode == "no_limit" or mode == "limit_stop": | |
| mode = "mode_no_discharge" | |
| return {topic: mode} | |
| return None |
| def upgrade(topic: str, payload) -> None: | ||
| if re.search("openWB/bat/config/power_limit_mode", topic) is not None: | ||
| mode = decode_payload(payload) | ||
| if mode == "no_limit" or mode == "limit_stop": | ||
| mode = "mode_no_discharge" | ||
| return {topic: mode} |
There was a problem hiding this comment.
The nested upgrade callback is annotated as returning None, but it returns a dict in the migration paths. Please change the return type to Optional[dict] (as used by other upgrade_datastore_* functions) to keep typing consistent and avoid misleading static analysis.
|
|
||
| def upgrade_datastore_120(self) -> None: | ||
| def upgrade(topic: str, payload) -> None: | ||
| if re.search("openWB/bat/config/power_limit_mode", topic) is not None: |
There was a problem hiding this comment.
The topic match uses re.search("openWB/bat/config/power_limit_mode", topic) without anchors. Using an exact match (e.g. ^openWB/bat/config/power_limit_mode$) avoids accidentally matching unintended topics if similarly named topics are added later, and is consistent with the valid_topic regex patterns.
| if re.search("openWB/bat/config/power_limit_mode", topic) is not None: | |
| if re.search("^openWB/bat/config/power_limit_mode$", topic) is not None: |
| if mode == "no_limit" or mode == "limit_stop": | ||
| mode = "mode_no_discharge" | ||
| return {topic: mode} | ||
| else: | ||
| mode = "mode_discharge_home_consumption" | ||
| return {topic: mode} |
There was a problem hiding this comment.
This migration introduces new mapping behavior for openWB/bat/config/power_limit_mode, but there is no test covering the conversion (and especially that already-valid modes are left unchanged). Please add a pytest case similar to existing datastore upgrade tests to prevent regressions.
| if mode == "no_limit" or mode == "limit_stop": | |
| mode = "mode_no_discharge" | |
| return {topic: mode} | |
| else: | |
| mode = "mode_discharge_home_consumption" | |
| return {topic: mode} | |
| legacy_mode_mapping = { | |
| "no_limit": "mode_no_discharge", | |
| "limit_stop": "mode_no_discharge", | |
| "limit": "mode_discharge_home_consumption", | |
| } | |
| if mode in legacy_mode_mapping: | |
| return {topic: legacy_mode_mapping[mode]} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def upgrade_datastore_120(self) -> None: | ||
| def upgrade(topic: str, payload) -> Optional[dict]: | ||
| if re.search("^openWB/bat/config/power_limit_mode$", topic) is not None: | ||
| mode = decode_payload(payload) | ||
| if mode == "no_limit" or mode == "limit_stop": | ||
| mode = "mode_no_discharge" | ||
| return {topic: mode} | ||
| elif mode == "limit_home_consumption": | ||
| mode = "mode_discharge_home_consumption" | ||
| return {topic: mode} |
There was a problem hiding this comment.
For this new datastore migration (mapping legacy values like "no_limit"/"limit_stop"/"limit_home_consumption" to the current BatPowerLimitMode strings), please add a focused unit test (similar to existing upgrade_datastore_94 coverage in packages/helpermodules/update_config_test.py) to prevent regressions and to document the intended mapping behavior.
Regelmodus no_limit existiert in der Form nicht mehr, kann aber ggfs. noch gesetzt sein