feat(modbus): share one TCP session per device and proxy it - #1003
Conversation
Drivers, probes and an optional LAN listener now go through one engine keyed by host:port, so two Lua drivers can poll the same inverter (or different unit IDs behind one gateway) without each taking a socket the device will not accept. modbus_proxy is off by default. When on, other integrations talk to FTW instead of the device. Writes stay blocked unless allow_write is set — they would bypass the control loop. Enabling it needs a restart. Signed-off-by: Cursor Agent <cursoragent@cursor.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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3890d96698
ℹ️ 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".
| first = err | ||
| } | ||
| } | ||
| p.wg.Wait() |
There was a problem hiding this comment.
When a proxy client keeps a persistent connection idle, as Home Assistant commonly does, Close closes only the listeners and then waits while the accepted connection remains blocked in readMBAP until its 90-second read deadline. Because main defers this call on SIGTERM and API-triggered restarts, shutdown can stall for up to 90 seconds; track and close accepted connections or otherwise cancel their reads before waiting.
Useful? React with 👍 / 👎.
| render: function (ctx) { | ||
| var field = ctx.field, config = ctx.config; | ||
| if (!config.homeassistant) config.homeassistant = {}; | ||
| if (!config.modbus_proxy) config.modbus_proxy = {}; |
There was a problem hiding this comment.
On an upgraded installation with no modbus_proxy section, merely opening this tab and saving any Home Assistant edit creates a disabled proxy object because the render initializes it and captureCurrentTab serializes all of the new controls. modbusProxyRestartReasons then compares the old nil pointer with this object and reports that the TCP listener requires a restart, so an otherwise hot-reloadable HA edit produces a spurious restart prompt and persists unrelated configuration; preserve the absent disabled state until the operator actually changes a proxy setting.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3890d96. Configure here.
| return | ||
| } | ||
| slog.Warn("modbus proxy accept", "listen", ln.Addr().String(), "err", err) | ||
| return |
There was a problem hiding this comment.
Accept errors stop the proxy
Medium Severity
A non-shutdown Accept error returns from serve and never loops again. The listen socket stays bound, so LAN clients cannot reconnect until the process restarts, even though startup logged the proxy as listening.
Reviewed by Cursor Bugbot for commit 3890d96. Configure here.
| '<label><input type="checkbox" data-checkbox-path="modbus_proxy.enabled"' + (config.modbus_proxy && config.modbus_proxy.enabled ? ' checked' : '') + '> Enabled</label>' + | ||
| field("Listen", "modbus_proxy.listen", "text", ":1502", | ||
| "Local bind. Default :1502 when the site has one Modbus device. Several inverters need capabilities.modbus.proxy_listen on each driver.") + | ||
| '<label><input type="checkbox" data-checkbox-path="modbus_proxy.allow_write"' + (config.modbus_proxy && config.modbus_proxy.allow_write ? ' checked' : '') + '> Allow writes from the LAN (bypasses FTW control)</label>' + |
There was a problem hiding this comment.
Settings save injects proxy section
Medium Severity
Rendering the Home Assistant tab creates an empty modbus_proxy object and its form fields. A save then persists a disabled proxy (listen defaulted to :1502), and RestartRequiredFor treats nil versus that struct as a startup bind change.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 3890d96. Configure here.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Reviewed the Modbus proxy + shared-session engine. Two lenses:
Correctness — session pooling in engine.go is ref-counted and mutex-guarded correctly; Capability.mu serializes every Read/Write*/executePDU call including applyUnit, so concurrent drivers/proxy clients sharing one socket can't interleave requests or race on the unit-ID switch. readMBAP/writeMBAP bound the ADU length (≤254 bytes) before allocating, so a malicious length field can't trigger a large alloc. Connection count is capped (proxyMaxClients=16) and idle-timed out (90s), so a LAN client can't hold the proxy open indefinitely or exhaust listener slots.
Security — the write gate (forward(), proxy.go) runs before cap.executePDU, so a denied write never reaches the driver/hardware; unknown function codes default-deny to illegal-function. allow_write is one flag applied to all binds from a single proxy config, matching the docs. Each listener is pinned to exactly one backend host:port at creation, no way for a LAN client to redirect to an arbitrary backend. Off by default.
No blocking findings. Good test coverage on the shared-socket + unit-id-multiplex paths (engine_test.go, proxy_test.go, slave_test.go). Safe to merge from my read — the PR body already flags overlap with #999/#957/#826 on config.go/main.go, worth a rebase-check before merging whichever lands second.
3 participants
Accepted text proposal
Fredrik asked for a Modbus proxy in FTW so other integrations can talk to devices through the box, and for every driver to go through the same engine so several drivers can poll one device while it still holds a single socket.
What changed
host:port. Drivers, the debug probe, fingerprinting and the proxy allOpen()that pool. Unit ID is per request, so two drivers on one RS-485 gateway share the socket and still address different slaves.Dial()stays a private connection for tests and one-shot probes.modbus_proxylistens on the LAN (default:1502when the site has one Modbus endpoint) and multiplexes client PDUs onto that session. Writes are denied unlessallow_writeis set. Unknown function codes are illegal-function, not forwarded.host:portbackends each needcapabilities.modbus.proxy_listen. Two drivers on the same endpoint share one listen address.config.example.yamlis the operator surface.Why
Many inverters accept one Modbus TCP client. FTW holding that socket is why Home Assistant’s native Modbus / Sungrow / SolarEdge integrations cannot share the device. A proxy on the box is the usual fix (port 1502 is already treated as a proxy port in discovery). Sharing among FTW’s own drivers is the same problem internally.
Boundaries and safety
slog.Errorand continue).0x0B, dispatch still uses the driver path and its own backoff.modbus_proxy.*and, while the proxy is on, driver Modbus host/port/proxy_listenrequire a restart. Driver add/remove still hot-reloads; they join or leave the shared session.Out of scope: exposing FTW telemetry as a virtual Modbus map, TLS, unit-id remapping, hot-reloading the listen port.
Verification
go test ./internal/modbus/— shared socket (one Accept, two unit IDs),Dialstill private, proxy read multiplex, write denied with exception0x01, write forwarded when allowed, unknown FC illegal.go test ./internal/config/— bind map, multi-endpointproxy_listenrequired, YAML parse, restart-required.make verify— Go + Python suites, compose/container/release-workflow checks,go vet ./...,go build ./...(exit 0).node --test web/settings/tabs/ha.test.mjs— proxy fieldset on the HA tab.sim-sungrowon127.0.0.1:15502, FTW proxy on127.0.0.1:11502. Driver poll + a second Modbus TCP client through the proxy both saw Sungrow input 4999 = 3598 (device type). Backend stayed at one ESTABLISHED socket (127.0.0.1:47182 ↔ :15502). FC 06 write returned illegal-function0x01.127.0.0.1:11502, Allow writes unchecked. Dashboard stayed live after closing the modal.Live dashboard with Sungrow telemetry while the proxy is up
Settings Home Assistant tab showing the Modbus TCP proxy fieldset
settings_ha_modbus_proxy_walkthrough.mp4
Open PRs that also touch
config.go/main.go/config.example.yaml(notably #999 OCPP, also draft #957 onmain.goand #826 onindex.html) have right of way on overlap; this adds a newmodbus_proxysection and a factory swap.Checklist
To show artifacts inline, enable in settings.