fix(edge-client): serialize Modbus TCP read/write over one shared connection - #1
Merged
Merged
Conversation
…nection Small PLCs / Modbus-TCP gateways (e.g. vakum2t) accept only one concurrent TCP socket. The old 2-client pattern opened a separate reader (telemetry) and writer (control) socket to the same host:port, so the writer's socket never became usable and every control write failed with EINPROGRESS (os error 115), even when the control gate granted access. Route every TCP transport through the existing actor pattern (previously only RTU): one connection per endpoint, read+write serialized over an mpsc channel. Actors are shared via ActorCache keyed by connection identity (tcp:host:port / rtu-tcp:host:port / serial:path): - Same host:port (vakum2t :502 read+write) -> one shared connection, fixed. - Split-port (vakum750 :502 read / :503 write) -> two independent actors, unchanged behavior. Remove the now-orphaned direct-socket path (ModbusReader/ModbusWriter, Backend::Tcp, for_tcp, with_unit_id).
There was a problem hiding this comment.
Pull request overview
This PR updates edge-client’s Modbus transport layer so Modbus TCP read + write are serialized over a single shared connection per endpoint via the existing actor pattern, fixing control writes failing on single-socket PLCs/gateways.
Changes:
- Routes Modbus TCP (in addition to RTU-over-TCP and RTU-serial) through
modbus_actor, so all requests share one actor-owned connection. - Introduces a generalized
ActorCachekeyed by endpoint identity (tcp:host:port,rtu-tcp:host:port,serial:path) and threads it through telemetry/control config builders. - Removes the old direct TCP reader/writer factory backend and updates tests to cover shared vs split-port TCP behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modbus_client.rs | Replaces per-connect TCP sockets with actor-handle factories; adds ActorCache and endpoint keying + tests. |
| src/modbus_actor.rs | Extends the actor to support plain Modbus TCP and updates spawn/connect behavior and tests. |
| src/telemetry.rs | Updates telemetry config/build flow to use the shared ActorCache. |
| src/main.rs | Wires up a single ActorCache instance and passes it into telemetry/control configs. |
| src/control_subscriber.rs | Updates control subscriber config to build per-device writer factories via ActorCache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
186
to
+190
| ); | ||
| let handle = crate::modbus_actor::spawn(transport, slave_id, timings) | ||
| .await | ||
| .context("spawn RTU serial actor (via SerialActorCache)")?; | ||
| lock.insert(path, handle.clone()); | ||
| .context("spawn modbus actor (via ActorCache)")?; | ||
| lock.insert(key, handle.clone()); |
Comment on lines
+531
to
+535
| Transport::Tcp { host, port } => { | ||
| let addr = format!("{host}:{port}"); | ||
| let stream = tokio::net::TcpStream::connect(&addr) | ||
| .await | ||
| .with_context(|| format!("tcp connect {addr}"))?; |
Comment on lines
+139
to
+141
| /// Returns `Result` for call-site ergonomics, but currently never fails: every | ||
| /// transport is supported and network errors don't surface here — the actor | ||
| /// handles connect/reconnect internally. |
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.
Problem
Small PLCs / Modbus-TCP gateways (e.g. vakum2t) accept only one concurrent TCP socket. The old 2-client pattern opened a separate reader (telemetry) and writer (control) socket to the same
host:port, so the writer's socket never became usable and every control write failed withEINPROGRESS(os error 115) — even when the control gate granted access.Fix
Route every TCP transport through the existing actor pattern (previously RTU-only): one connection per endpoint, read+write serialized over an mpsc channel. Actors are shared via
ActorCachekeyed by connection identity (tcp:host:port/rtu-tcp:host:port/serial:path)::502read+write) → one shared connection — fixes os error 115.:502read /:503write) → two independent actors — behavior unchanged.Removes the now-orphaned direct-socket path (
ModbusReader/ModbusWriter,Backend::Tcp,for_tcp,with_unit_id).Test
cargo build,cargo clippy --all-targets,cargo test— all clean. Added coverage: TCP reader+writer share one actor perhost:port; split-port keeps two;spawnaccepts TCP.