inverter: add fronius modbus backup safety#350
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in safety guard to the fronius-modbus inverter integration to avoid sending “restrictive” Modbus writes while the system is off-grid/unknown, by inferring grid status from SunSpec line-frequency reads (inverter + meter) and failing open to allow-discharge.
Changes:
- Introduces a SunSpec-based grid-status reader/inference and wires it into
FroniusModbusControlto gate restrictive writes. - Extends the fronius-modbus inverter + factory wiring to optionally create/close a second (meter) Modbus transport for backup safety.
- Adds targeted unit tests for grid-status inference/reads, factory wiring/cleanup, and control-layer fail-open behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/batcontrol/inverter/inverter.py |
Wires optional backup_mode_safety_enabled behavior: creates meter transport + FroniusModbusGridStatusReader, ensures cleanup on failure. |
src/batcontrol/inverter/fronius_modbus/control.py |
Adds grid-status gating for restrictive mode writes, with fail-open to allow-discharge. |
src/batcontrol/inverter/fronius_modbus/grid_status.py |
New grid-status inference + reader based on SunSpec “common model” frequency registers. |
src/batcontrol/inverter/fronius_modbus/inverter.py |
Accepts grid_status_reader and extra_transports; closes extra transports on shutdown. |
src/batcontrol/inverter/fronius_modbus/__init__.py |
Exposes FroniusModbusGridStatusReader from the package. |
tests/batcontrol/inverter/test_fronius_modbus_control.py |
Adds tests for restrictive-mode gating and fail-open behavior. |
tests/batcontrol/inverter/test_fronius_modbus_factory.py |
Adds tests for factory wiring, defaults, and cleanup when meter transport creation fails. |
tests/batcontrol/inverter/test_fronius_modbus_inverter.py |
Adds tests verifying reader wiring and shutdown closes extra transports. |
tests/batcontrol/inverter/test_fronius_modbus_grid_status.py |
New tests for grid-status inference and SunSpec frequency reads. |
| if config.get('backup_mode_safety_enabled', False): | ||
| meter_transport = FroniusModbusTcpTransport( | ||
| config['address'], | ||
| port=config.get('port', 502), | ||
| unit_id=config.get('meter_unit_id', 200), | ||
| ) |
There was a problem hiding this comment.
The new fronius-modbus config options backup_mode_safety_enabled and meter_unit_id are read here, but they do not appear to be documented in the repo's example config (e.g. config/batcontrol_config_dummy.yaml) or anywhere else in-repo. Please add these keys (at least as commented-out options) to the dummy/example config and/or add a short doc entry so users can discover and correctly configure the safety feature.
| except Exception: | ||
| for opened_transport in [transport, *extra_transports]: | ||
| close = getattr(opened_transport, 'close', None) | ||
| if close is not None: | ||
| with suppress(Exception): | ||
| close() | ||
| raise |
There was a problem hiding this comment.
This except Exception: cleanup block is very broad and will likely trigger pylint's broad-exception-caught warning (and currently doesn't bind the exception). Consider narrowing the exception types you expect here, or explicitly annotating the intent with except Exception as exc plus a local # pylint: disable=broad-exception-caught (and optionally logging at debug) before re-raising.
| try: | ||
| status_read = self.grid_status_reader.read_grid_status() | ||
| except Exception as exc: | ||
| logger.warning( | ||
| "Skipping Fronius Modbus %s because grid status could not be read: %s", | ||
| mode_name, | ||
| exc, | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Catching Exception here is intentional for a fail-open safety guard, but it will likely raise pylint's broad-exception-caught warning. Consider catching the specific exception types your transport/reader raises, or add a local # pylint: disable=broad-exception-caught to document the intent and keep lint noise down.
| return lower_bound < frequency_hz < upper_bound | ||
|
|
||
|
|
||
| def _is_inverter_operating_frequency(frequency_hz: float) -> bool: | ||
| lower_bound = GRID_FREQUENCY_HZ - INVERTER_OPERATING_FREQUENCY_TOLERANCE_HZ | ||
| upper_bound = GRID_FREQUENCY_HZ + INVERTER_OPERATING_FREQUENCY_TOLERANCE_HZ | ||
| return lower_bound < frequency_hz < upper_bound |
There was a problem hiding this comment.
The tolerance checks use strict inequalities (<), so a frequency exactly on the boundary (e.g. 49.8 Hz or 50.2 Hz with a 0.2 Hz tolerance) will be treated as not near grid. If the tolerance is meant to be inclusive, switch these comparisons to <=/>= to avoid boundary misclassification.
| return lower_bound < frequency_hz < upper_bound | |
| def _is_inverter_operating_frequency(frequency_hz: float) -> bool: | |
| lower_bound = GRID_FREQUENCY_HZ - INVERTER_OPERATING_FREQUENCY_TOLERANCE_HZ | |
| upper_bound = GRID_FREQUENCY_HZ + INVERTER_OPERATING_FREQUENCY_TOLERANCE_HZ | |
| return lower_bound < frequency_hz < upper_bound | |
| return lower_bound <= frequency_hz <= upper_bound | |
| def _is_inverter_operating_frequency(frequency_hz: float) -> bool: | |
| lower_bound = GRID_FREQUENCY_HZ - INVERTER_OPERATING_FREQUENCY_TOLERANCE_HZ | |
| upper_bound = GRID_FREQUENCY_HZ + INVERTER_OPERATING_FREQUENCY_TOLERANCE_HZ | |
| return lower_bound <= frequency_hz <= upper_bound |
| def _unsigned_to_signed_16(value: int) -> int: | ||
| return value - 65536 if value >= 32768 else value |
There was a problem hiding this comment.
_unsigned_to_signed_16() duplicates unsigned_to_signed_16() from fronius_modbus/reads.py. To reduce duplication and risk of the helpers diverging, consider reusing the existing conversion helper (or moving the shared conversion into a small common module).
filiplajszczak
force-pushed
the
fronius-modbus-grid-safety
branch
from
539f6c7 to
28164a5
Compare
|
Addressed the Copilot follow-ups: reused the existing signed 16-bit helper, made frequency tolerance boundaries inclusive with tests, and annotated the intentional broad exception handlers. |
Adds an opt-in safety guard for
fronius-modbus.When
backup_mode_safety_enabledis true, restrictive Modbus writes are only sent while grid status is confidently on-grid. If status is off-grid, unknown, or unreadable, batcontrol fails open by restoring allow-discharge mode.Addresses #286.