Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the battery control logic to use Enum members directly (instead of their .value strings) for configuration and state fields in bat_all, and updates related unit tests accordingly.
Changes:
- Change
BatAllData.ConfigandBatAllData.Setfields fromstrto Enum-typed fields and update defaults. - Replace multiple comparisons against
*.valuewith comparisons against Enum members. - Update
bat_all_test.pyparameter defaults and test cases to pass Enum members instead of strings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/control/bat_all.py | Switches configuration/state fields and comparisons from string values to Enum members throughout battery control logic. |
| packages/control/bat_all_test.py | Updates test parameter defaults/cases to use Enum members (but still has some mismatched type hints). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| power_limit_mode: BatPowerLimitMode = field(default=BatPowerLimitMode.MODE_NO_DISCHARGE, | ||
| metadata={"topic": "config/power_limit_mode"}) | ||
| power_limit_condition: BatPowerLimitCondition = field(default=BatPowerLimitCondition.VEHICLE_CHARGING, | ||
| metadata={"topic": "config/power_limit_condition"}) | ||
| manual_mode: ManualMode = field(default=ManualMode.MANUAL_DISABLE, | ||
| metadata={"topic": "config/manual_mode"}) |
There was a problem hiding this comment.
The config fields are now typed/stored as Enum instances, but MQTT subscription writes per-field config values via SubData.set_json_payload_class without Enum coercion (it assigns decoded payloads directly). That means these attributes will typically become plain strings at runtime (e.g., "mode_no_discharge"), and later Enum comparisons will fail. Either keep these config fields as str (and compare to .value), or update the subscription/deserialization path to convert strings to the declared Enum type for dataclass fields.
| @@ -349,7 +349,7 @@ def _get_charging_power_left(self): | |||
| charging_power_left = 0 | |||
| self.data.set.regulate_up = True if self.data.get.soc < 100 else False | |||
| # ev wird nach Speicher geladen | |||
| elif config.bat_mode == BatConsiderationMode.EV_MODE.value: | |||
| elif config.bat_mode == BatConsiderationMode.EV_MODE: | |||
There was a problem hiding this comment.
pv_charging.bat_mode is populated as a string in practice (see PvCharging.bat_mode default in packages/control/general.py and SubData.set_json_payload_class assignment), but this code now compares it against BatConsiderationMode Enum members. This will make both branches unreachable unless the value is explicitly converted to the Enum elsewhere. Compare against .value or ensure bat_mode is stored as BatConsiderationMode (with conversion on MQTT receive).
| # Speicher entlädt oder Speicher lädt bei gewollter PV-Ladung | ||
| bat_power_valid = (self.data.get.power <= 0 or | ||
| (self.data.get.power > 0 and | ||
| self.data.config.power_limit_mode == BatPowerLimitMode.MODE_CHARGE_PV_PRODUCTION.value)) | ||
| self.data.config.power_limit_mode == BatPowerLimitMode.MODE_CHARGE_PV_PRODUCTION)) | ||
| # EVU Bezug vorhanden oder gewollte PV-Ladung aktiv | ||
| evu_power_valid = (data.data.counter_all_data.get_evu_counter().data.get.power >= -100 or | ||
| self.data.config.power_limit_mode == BatPowerLimitMode.MODE_CHARGE_PV_PRODUCTION.value) | ||
| self.data.config.power_limit_mode == BatPowerLimitMode.MODE_CHARGE_PV_PRODUCTION) |
There was a problem hiding this comment.
These comparisons assume power_limit_mode is a BatPowerLimitMode Enum, but when the value comes from MQTT it will be a string unless it’s converted on receipt. With the current subscription logic this will cause bat_power_valid / evu_power_valid to evaluate incorrectly. Consider comparing to BatPowerLimitMode.*.value or adding Enum coercion when setting config values from topics.
| if ((self.data.config.bat_control_permitted is False or | ||
| self.data.config.bat_control_activated is False) | ||
| and self.data.set.current_state == CurrentState.STARTUP.value): | ||
| and self.data.set.current_state == CurrentState.STARTUP): | ||
| self.data.set.set_limit = False | ||
| elif (self.data.set.current_state == CurrentState.IDLE.value and | ||
| elif (self.data.set.current_state == CurrentState.IDLE and | ||
| charge_mode == BatChargeMode.BAT_SELF_REGULATION): | ||
| self.data.set.set_limit = False | ||
| else: | ||
| self.data.set.set_limit = True | ||
|
|
||
| if charge_mode == BatChargeMode.BAT_SELF_REGULATION: | ||
| self.data.set.current_state = CurrentState.IDLE.value | ||
| self.data.set.current_state = CurrentState.IDLE | ||
| else: | ||
| self.data.set.current_state = CurrentState.ACTIVE.value | ||
| self.data.set.current_state = CurrentState.ACTIVE |
There was a problem hiding this comment.
current_state is now modeled as CurrentState, but the MQTT subscription path sets set/current_state directly from the decoded payload (string). This can lead to mixed types (str vs Enum) and make these state checks/assignments behave incorrectly after retained topics are received. Either keep current_state as str and use .value, or ensure Enum conversion when consuming the topic payload.
| power_limit_mode: str = BatPowerLimitMode.MODE_NO_DISCHARGE | ||
| power_limit_condition: str = BatPowerLimitCondition.VEHICLE_CHARGING | ||
| bat_manual_mode: str = ManualMode.MANUAL_DISABLE |
There was a problem hiding this comment.
These fields are still annotated as str but now default to Enum members. Update the type hints to the corresponding Enum types (BatPowerLimitMode, BatPowerLimitCondition, ManualMode) to match how the params are used and avoid misleading typing in tests.
| power_limit_mode: str = BatPowerLimitMode.MODE_NO_DISCHARGE | |
| power_limit_condition: str = BatPowerLimitCondition.VEHICLE_CHARGING | |
| bat_manual_mode: str = ManualMode.MANUAL_DISABLE | |
| power_limit_mode: BatPowerLimitMode = BatPowerLimitMode.MODE_NO_DISCHARGE | |
| power_limit_condition: BatPowerLimitCondition = BatPowerLimitCondition.VEHICLE_CHARGING | |
| bat_manual_mode: ManualMode = ManualMode.MANUAL_DISABLE |
No description provided.