Repository navigation
fix(modbus): give external clients exclusive write ownership - #1539
Conversation
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddb6efd195
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ctrl.SupportsPVCurtail = supportsPVCurtailFrom(cfg.Drivers) | ||
| ctrl.SolarFeedDrivers = solarFeedDriversFrom(cfg.Drivers) | ||
| ctrl.DriverLimits = driverLimitsFrom(cfg.Drivers, cfg.Batteries) | ||
| ctrl.DriverLimits = driverLimitsFrom(cfg.ModbusControlDrivers(), cfg.Batteries) |
There was a problem hiding this comment.
Exclude external owners from PV curtail capability
When an externally owned Modbus inverter also has supports_pv_curtail, only DriverLimits uses the ownership-filtered drivers; SupportsPVCurtail still advertises that inverter as controllable, and the hot-reload path repeats the same pattern. During an export-ceiling breach, computePVExportGuard therefore counts the proposed cap as a real reduction and can report the residual as cleared, while the registry rejects the command as observe-only and nothing reaches the device. Exclude externally owned drivers when building the PV-curtail map so the guard reports the export as uncontrollable rather than hiding the unmet clamp.
AGENTS.md reference: AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
Problem and result
The Modbus proxy can accept register writes while FTW still writes through its drivers. Two controllers can then send commands to the same device.
Make the existing proxy choice select one writer:
allow_write: true): external clients write; FTW keeps reading and sends no device commands.The selected owner stays in force until the setting changes and Core restarts. Saving a new mode, losing a client or failing to bind the listener does not return control to FTW.
Scope and safety
Apply ownership to every configured unit on each exposed endpoint. Block FTW writes at both the shared Modbus connection and Lua host, including init, poll, command, recovery and cleanup. Exclude those devices from FTW control capacity while keeping telemetry.
Allow only configured unit IDs and valid standard request shapes. Reject broadcast and unknown units. Give driver requests priority; proxy requests get one attempt with a timeout of at most one second and leave reconnect to drivers. Close idle clients on shutdown and retry listener accept errors.
Rename the settings tab to Integrations. Home Assistant MQTT and the Modbus proxy each keep their own section. Use the existing controls to explain both proxy modes. Opening or saving an untouched settings view no longer creates a disabled proxy section or asks for a restart.
Raw external register writes bypass Core command validation. In external mode, the external controller must handle device safety. There is no automatic fallback to FTW control.
The proxy from #1003 already exists on master through #1225. This PR fixes write ownership on that merged base and addresses the proxy review findings. Other open changes to config, startup and settings remain outside this scope; shared protocol names and driver snapshots do not change.
Verification
make verify: passed tests, migration/release checks, bundled worker checks, vet and build.cd go && go test -race -count=1 ./internal/modbus ./internal/drivers -run 'Proxy|ExternalModbus|Engine|DialShares': passed.npm test: all 701 tests passed.ddb6efd1: all applicable checks passed, including Core tests/vet, full stack, web, registry, solver and Linux amd64/arm64 builds.modbus_proxyabsent from saved config.Hardware validation and an explicit human review of the rendered settings remain open. The owner requested merge on green CI; those checks remain part of beta validation.
Checklist