Skip to content

fix(modbus): give external clients exclusive write ownership - #1539

Merged
frahlg merged 2 commits into
masterfrom
codex/modbus-proxy-ownership
Oct 8, 2026
Merged

frahlg merged 2 commits into
masterfrom
codex/modbus-proxy-ownership

Conversation

@frahlg

@frahlg frahlg commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

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:

  • Read only, the default: external clients read; FTW keeps control.
  • External control (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.
  • GitHub CI on ddb6efd1: all applicable checks passed, including Core tests/vet, full stack, web, registry, solver and Linux amd64/arm64 builds.
  • Browser check in a running local Core with no equipment: checked the rendered Integrations tab, Home Assistant section and proxy controls; an untouched Save showed no restart prompt and left modbus_proxy absent from saved config.
  • Regression tests cover writer exclusion, continued reads, listener failure, restart ownership, Lua lifecycle writes, API control, planner capacity, allowed units, malformed requests, driver priority, timeout, reconnect and shutdown.

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

  • The change follows VISION.md and one selected scope.
  • I checked overlapping PRs and coordinated shared files/contracts.
  • Relevant checks cover the changed behaviour and failure paths.
  • A human reviewed changed UI in a browser, or no UI changed.
  • A Changeset is included, or the change is exempt.
  • Every commit has a DCO sign-off.

frahlg added 2 commits October 8, 2026 09:08
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
@frahlg
frahlg marked this pull request as ready for review October 8, 2026 14:38
@frahlg
frahlg merged commit 6526638 into master Oct 8, 2026
15 checks passed
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-08T14:42:20.265924Z ddb6efd Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines 25 to +27
ctrl.SupportsPVCurtail = supportsPVCurtailFrom(cfg.Drivers)
ctrl.SolarFeedDrivers = solarFeedDriversFrom(cfg.Drivers)
ctrl.DriverLimits = driverLimitsFrom(cfg.Drivers, cfg.Batteries)
ctrl.DriverLimits = driverLimitsFrom(cfg.ModbusControlDrivers(), cfg.Batteries)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant