Added Transport Socket, Transport Factory and Timer traits#78
Added Transport Socket, Transport Factory and Timer traits#78JustinKovacich wants to merge 7 commits into
Conversation
JustinKovacich
added
documentation
There was a problem hiding this comment.
Pull request overview
Introduces an executor-agnostic transport abstraction layer intended to decouple the client/server networking code from tokio::net::UdpSocket, enabling future alternate backends (e.g., embedded stacks).
Changes:
- Added
TransportSocket,TransportFactory, andTimertraits plus supporting types (TransportError,IoErrorKind,SocketOptions,ReceivedDatagram). - Added compile-check style unit tests for trait implementability and default behaviors.
- Exposed the new module and re-exported its public API from
src/lib.rs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/transport.rs |
New transport-layer traits and supporting types, with module-level documentation and tests. |
src/lib.rs |
Adds pub mod transport; and re-exports transport API types at the crate root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three docs/fmt fixes on PR #78: - src/transport.rs IPv4-only rationale: replaced the `239.0.0.0/8` claim with a reference to `crate::protocol::sd::MULTICAST_IP` (239.255.0.255) — that's the actual multicast address this crate uses, not the class-D block. Also clarified that only the transport layer is IPv4-today; the protocol layer does parse IPv6 SD option endpoints. - src/lib.rs transport-module doc: the "used by client and server modules" claim was aspirational. Reworded to "intended to be consumed by... in a future refactor" with a one-line note that client/server still use tokio/socket2 directly today. - src/transport.rs:356 `fn join_multicast_v4` signature: the return `->` was split onto its own line unnecessarily; rustfmt puts it on one line since it fits. Collapsed to match. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JustinKovacich
force-pushed
the
feature/stack_buffer_encoding_bufs
branch
from
e68f013 to
f5bf200
Compare
The module-level "# `Send` and multithreaded executors" section showed a HRTB bound on `<T as TransportSocketSendFut<'a>>::Fut: Send` as the way consumers should bind `Send`. No such trait exists in this crate — with RPITIT the returned future type is anonymous and cannot be named, and introducing a GAT-style escape hatch would pollute the trait for the common single-threaded case. Replaced with the reviewer-preferred pattern: wrap the call in an `async move` block and require `T: Send + 'static` on the captured state. A tokio-backed implementation whose underlying `UdpSocket` is already `Send + Sync` produces `Send` futures automatically via async-block capture inference, so no trait-level bound is required. Implementations holding `!Send` state fail the `T: Send` bound at the `tokio::spawn` call site, which is the actionable location. Docs-only change; `cargo test --doc` passes on the new ignore-fenced example. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three docs/fmt fixes on PR #78: - src/transport.rs IPv4-only rationale: replaced the `239.0.0.0/8` claim with a reference to `crate::protocol::sd::MULTICAST_IP` (239.255.0.255) — that's the actual multicast address this crate uses, not the class-D block. Also clarified that only the transport layer is IPv4-today; the protocol layer does parse IPv6 SD option endpoints. - src/lib.rs transport-module doc: the "used by client and server modules" claim was aspirational. Reworded to "intended to be consumed by... in a future refactor" with a one-line note that client/server still use tokio/socket2 directly today. - src/transport.rs:356 `fn join_multicast_v4` signature: the return `->` was split onto its own line unnecessarily; rustfmt puts it on one line since it fits. Collapsed to match. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to 1235f59 — the lib.rs re-export doc for `pub mod transport` still claimed "used by the client and server modules" which is aspirational. Aligns the re-export doc with the matching rewording on transport.rs itself: "Intended to be consumed by... in a future refactor; currently those paths still use tokio/socket2 directly." Should have landed in 1235f59; my earlier edit didn't get saved before the commit closed. Additive commit per stacked-PR discipline. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Visibility qualifiers aren't permitted on items declared inside a function
body. With the `client` feature enabled, the transport module's "Minimal
adapter sketch" doctest failed to compile because it wrapped `pub struct`
declarations in `fn wrapper() { ... }`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The prior trait shape made send_to / recv_from / join_multicast_v4 / leave_multicast_v4 take &mut self. A pending recv_from future therefore holds an exclusive borrow of the socket, which prevents a single-task socket loop from calling send_to in a concurrent select! branch — the exact pattern used by the client and server socket loops. That would have forced either an awkward pin-and-drop dance per iteration or a later breaking trait change once the socket loops were rewired onto this trait. Switch all I/O methods to &self. Rationale for the specific backends the crate targets: - tokio::net::UdpSocket already exposes send_to / recv_from on &self, so the Tokio adapter (and the illustrative doctest) becomes a 1:1 mirror of the underlying API with no shadowing adapter state. - embassy_net::udp::UdpSocket is likewise &self; the bare-metal spike adapter was only taking &mut self because the trait forced it (the spike's own comment called this out as a forced mismatch). That downgrade disappears. - Raw smoltcp users must wrap the socket in RefCell<_> (single-threaded no_std) or critical_section::Mutex<RefCell<_>>, which is the standard interior-mutability shape for that crate. Update the in-file NullSocket test impl and the "Minimal adapter sketch" doctest to match the new signatures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Module-level doc claimed the client/server modules "bind tokio::net::UdpSocket directly", but they actually configure sockets via socket2 (SO_REUSEADDR, multicast interface, multicast loop) and then convert to tokio::net::UdpSocket. Rewrite the paragraph to describe the real backend so consumers aren't misled about what's being abstracted away. - Add explicit # Errors sections to TransportSocket::send_to and TransportSocket::recv_from describing the TransportError variants and kinds backends are expected to produce, matching the style of other fallible APIs in this crate. Also note that recv_from does not treat oversize datagrams as an error — truncation is surfaced via ReceivedDatagram::truncated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JustinKovacich
force-pushed
the
feature/transport_socket_trait
branch
from
62bce46 to
756f4b2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// On backends that size `buf` at least as large as the link MTU (the | ||
| /// expected configuration — see [`crate::UDP_BUFFER_SIZE`]), truncation | ||
| /// should not occur in practice; the field exists so backends that cannot | ||
| /// guarantee this can surface it explicitly instead of silently dropping. |
There was a problem hiding this comment.
Docs for ReceivedDatagram::truncated imply that sizing buf to crate::UDP_BUFFER_SIZE means it’s “at least as large as the link MTU” and that truncation should not occur. In this crate, UDP_BUFFER_SIZE = 1500 is an application-level cap and is explicitly not Ethernet-MTU-safe for UDP payloads (see UDP_BUFFER_SIZE docs in src/lib.rs), so this rationale is misleading. Please reword to avoid equating UDP_BUFFER_SIZE with link MTU / non-fragmenting payload size, and describe truncation expectations in terms of the configured application cap vs backend datagram sizes.
| /// On backends that size `buf` at least as large as the link MTU (the | |
| /// expected configuration — see [`crate::UDP_BUFFER_SIZE`]), truncation | |
| /// should not occur in practice; the field exists so backends that cannot | |
| /// guarantee this can surface it explicitly instead of silently dropping. | |
| /// If callers use a buffer sized to [`crate::UDP_BUFFER_SIZE`], truncation | |
| /// is generally not expected on backends whose delivered datagrams are | |
| /// bounded by that configured application-level cap. Backends that may | |
| /// deliver larger datagrams should surface this explicitly instead of | |
| /// silently dropping the fact that data was discarded. |
|
|
||
| /// Upper bound, in bytes, on datagrams this socket will successfully | ||
| /// accept in `send_to` or return via `recv_from`. The default returns | ||
| /// [`crate::UDP_BUFFER_SIZE`] (1500), matching standard Ethernet MTU. |
There was a problem hiding this comment.
max_datagram_size docs say the default returns crate::UDP_BUFFER_SIZE (1500) “matching standard Ethernet MTU”. A 1500-byte UDP payload does not fit within a 1500-byte L2 MTU once IP/UDP headers are included (and UDP_BUFFER_SIZE’s own docs call this out), so this statement is incorrect. Please rephrase to describe it as the crate’s default application-level UDP payload cap (currently 1500), without implying it’s MTU-safe.
| /// [`crate::UDP_BUFFER_SIZE`] (1500), matching standard Ethernet MTU. | |
| /// [`crate::UDP_BUFFER_SIZE`], the crate's default application-level | |
| /// UDP payload cap (currently 1500 bytes). |
|
Closing without merge to declutter the stack: this phase's changes are carried in full by the consolidated lineage under PR #114 (phase 21), which the next development stack builds on. Branch is retained. |
Define the transport layer trait to later implement for both x86 and embedded systems.
This is a chained PR. Prev: #77. Next: #79