Conversation
ndrsnhs
marked this pull request as ready for review
August 25, 2026 07:54
ndrsnhs
force-pushed
the
victron-bat-control-mode
branch
from
September 10, 2026 13:40
ddf11b9 to
292abae
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The dispatch can pass SetPoint objects to unupdated battery implementations, and Victron’s unconditional register read can interrupt control writes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends battery control with EVU power data and Victron ESS Mode 1 grid-setpoint handling.
Changes:
- Adds and propagates the
SetPointdataclass. - Migrates battery adapters and MQTT/configuration handling.
- Updates Victron charge/discharge control.
File summaries
| File | Summary |
|---|---|
packages/modules/devices/victron/victron/bat.py |
Implements Victron Mode 1 grid-setpoint control. |
packages/modules/devices/varta/varta/bat_modbus.py |
Migrates battery control input. |
packages/modules/devices/sungrow/sungrow_sh/bat.py |
Migrates battery control input. |
packages/modules/devices/sungrow/sungrow_ihm/bat.py |
Migrates battery control input. |
packages/modules/devices/sonnen/sonnenbatterie/bat.py |
Migrates battery control input. |
packages/modules/devices/solis/solis/bat.py |
Migrates battery control input. |
packages/modules/devices/solax/solax/bat.py |
Migrates battery control input. |
packages/modules/devices/solaredge/solaredge/bat.py |
Migrates battery control input. |
packages/modules/devices/sma/sma_sunny_island/bat.py |
Migrates battery control input. |
packages/modules/devices/sma/sma_sunny_boy/bat.py |
Migrates battery control input. |
packages/modules/devices/sigenergy/sigenergy/bat.py |
Migrates battery control input. |
packages/modules/devices/qcells/qcells/bat.py |
Migrates battery control input. |
packages/modules/devices/marstek/venus_c_e/bat.py |
Migrates battery control input. |
packages/modules/devices/kostal/kostal_plenticore/bat.py |
Migrates battery control input. |
packages/modules/devices/good_we/good_we/bat.py |
Migrates battery control input. |
packages/modules/devices/generic/mqtt/bat.py |
Migrates MQTT battery control input. |
packages/modules/devices/batterx/batterx/bat.py |
Migrates battery control input. |
packages/modules/devices/anker/solarbank/bat.py |
Migrates battery control input. |
packages/modules/devices/alpha_ess/alpha_ess/bat.py |
Migrates battery control input. |
packages/modules/common/configurable_device.py |
Passes setpoint data through the fault wrapper. |
packages/helpermodules/update_config.py |
Registers the EVU power topic. |
packages/helpermodules/setdata.py |
Validates EVU power input. |
packages/helpermodules/mosquitto_dynsec/missing_role_topics_test.py |
Updates internal topic permissions tests. |
packages/control/process.py |
Passes complete battery setpoints to devices. |
packages/control/bat.py |
Adds the EVU power setpoint field. |
packages/control/bat_all.py |
Populates EVU power for battery control. |
Review details
Suppressed comments (1)
packages/modules/devices/victron/victron/bat.py:64
- This read is performed even for
Noneand0setpoints, althoughbat_poweris only used in the charge/discharge branches. Besides adding a Modbus request every control cycle, a read failure can prevent the following self-regulation/stop writes and leave the previous Victron mode active. Read register 842 only when calculating an active grid setpoint (or use the cached value for that calculation).
bat_power = self.__tcp_client.read_holding_registers(842, ModbusDataType.INT_16, unit=modbus_id)
- Files reviewed: 26/26 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| target=set_power_limit_wrapper, | ||
| args=(bat_component, | ||
| data.data.bat_data[f"bat{bat_component.component_config.id}"].data.set.power_limit), | ||
| data.data.bat_data[f"bat{bat_component.component_config.id}"].data.set), |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new SetPoint contract is not reflected in the AbstractBat interface and remaining sample implementations.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Comment on lines
+134
to
+139
| def set_power_limit_wrapper(bat_component: AbstractBat, setpoint: Optional[SetPoint]): | ||
| """set_power_limit innerhalb des SingleComponentUpdateContext aufrufen, | ||
| damit Fehler im fault_state-Handler behandelt werden | ||
| """ | ||
| with SingleComponentUpdateContext(bat_component.fault_state, update_always=False): | ||
| bat_component.set_power_limit(power_limit) | ||
| bat_component.set_power_limit(setpoint) |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The AbstractBat contract migration leaves the repository’s documented sample implementations on the old scalar API.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Comment on lines
34
to
+35
| @abstractmethod | ||
| def set_power_limit(self, power_limit: Optional[int]) -> None: | ||
| def set_power_limit(self, setpoint: SetPoint) -> None: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Victron erwartet in allen Modi die Übergabe des Grid Setpoints.
In dieser Implementierung wird dafür Mode 1 genutzt.
Mode 2 deaktiviert die Phasensaldierung und ist daher nicht nutzbar.
Mode 3 setzt die Register entgegen der Dokumentation nach 10-14 Sekunden zurück
Die für die Berechnung nötigen zusätzlichen Daten werden als dataclass übergeben. Die Entscheidung welche Werte und wie diese genutzt werden/ die Berechnung verbleibt damit in der Komponente.
Für die Entladesperre wird weiterhin Mode 2 genutzt. Bei kompletter Sperre des Speichers spielt die fehlende Saldierung keine Rolle. Die Register haben eine längere Lebensdauer als in Mode 3