diff --git a/Cargo.lock b/Cargo.lock index 388d26d..78e3743 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1214,6 +1214,7 @@ dependencies = [ "rp2040-hal", "usb-device", "usbd-serial", + "wiredsensor-core", ] [[package]] diff --git a/README.md b/README.md index e6a1a8f..383930f 100644 --- a/README.md +++ b/README.md @@ -20,12 +20,15 @@ Written in Rust on **RTIC 2** + `rp2040-hal`. | Path | Contents | |------------|--------------------------------------------------------------------------| -| `core/` | `wiredsensor-core` — framing, CRCs, dispatch, SHT3x math. Pure, no I/O. | +| `core/` | `wiredsensor-core` — framing, CRCs, dispatch, SHT3x math, line rate. Pure, no I/O. | | `firmware/`| `wiredsensor-fw` — the RTIC application, drivers and register access. | +| `bridge/` | `wiredsensor-bridge` — a spare RP2040 as a USB-to-RS485 bridge, for testing. | +| `tools/` | `wiredsensor.py` — PC-side master and end-to-end test suite. | -The split exists so the entire protocol can be exercised by ordinary host unit -tests (`cargo test`) with no hardware and no emulator. `core` performs no I/O and -touches no peripherals; everything hardware-shaped lives in `firmware`. +The `core`/`firmware` split exists so the entire protocol can be exercised by +ordinary host unit tests (`cargo test`) with no hardware and no emulator. `core` +performs no I/O and touches no peripherals; everything hardware-shaped lives in +`firmware`. ## Pinout @@ -199,18 +202,30 @@ unknown cmd 0x7E → 01 7E 00 01 A0 `PING` is the intended first bring-up step: it exercises framing, CRC and the RS485 driver-enable turnaround without involving the sensor at all. -## Per-unit configuration +## Configuration -Everything a unit needs is in `firmware/src/board.rs`: +Per-unit settings live in `firmware/src/board.rs`: ```rust pub const UNIT_ADDRESS: u8 = 0x01; // this node's bus address pub const DEVICE_SERIAL: u32 = 0x0000_0001; -pub const BAUD_RATE: u32 = 19_200; pub const SENSOR_I2C_ADDR: u8 = 0x44; // match the ADDR strap pub const SENSOR_PERIOD_MS: u32 = 1_000; ``` +The **line rate is not** among them. It belongs to the segment rather than to any +one board, so it is defined once in `core/src/timing.rs` and read by both the +node and the bridge: + +```rust +pub const BAUD_RATE: u32 = 19_200; // wiredsensor_core::timing +``` + +A baud mismatch between two ends of a bus is silent and presents as random CRC +failures, which is a poor thing to debug — hence one definition rather than a +copy per firmware. `INTER_FRAME_GAP_US` and `CHAR_TIME_US` derive from it, and a +unit test asserts they cannot go stale if it changes. + The address is compile-time by design: no flash wear, no commissioning protocol, and no way for a bus glitch to renumber a live node. The cost is one image per unit, so keep `UNIT_ADDRESS` and `DEVICE_SERIAL` the only things that differ. @@ -223,7 +238,7 @@ is already reserved for it. ```bash cargo build --release -p wiredsensor-fw # firmware, thumbv6m-none-eabi -cargo test -p wiredsensor-core --target x86_64-unknown-linux-gnu # 42 host tests +cargo test -p wiredsensor-core --target x86_64-unknown-linux-gnu # 43 host tests ``` The host-target flag is needed because `.cargo/config.toml` defaults the whole @@ -283,6 +298,82 @@ if you want to see it. Note `16c0:27dd` is the pid.codes generic CDC-ACM pair, intended for development. Replace it before shipping. +## End-to-end bus testing + +`bridge/` turns a spare RP2040 into a USB-to-RS485 bridge, and +`tools/wiredsensor.py` drives the protocol from the PC through it. Together they +test the bus itself rather than just the node's internals. + +Two design choices make this a usable test instrument rather than a mirror of +the firmware's own assumptions: + +- **The bridge is protocol-agnostic.** Bytes from USB go out on the pair, bytes + from the pair come back up USB. It knows nothing of frames, addresses or CRCs, + so it cannot have a bug that happens to agree with the node's. +- **The PC side reimplements the wire format** from the specification above + rather than sharing `wiredsensor-core`. Shared code would let a framing or CRC + bug cancel out and every test pass regardless. + +### Wiring + +You need two transceiver modules. The bridge mirrors the node's pin assignment, +so one diagram covers both boards: `GPIO0 → DI`, `GPIO1 ← RO`, `GPIO2 → DE+RE`. + +``` +bridge A ────────── A node + B ────────── B + GND ────────── GND + [120Ω] [120Ω] ← across A-B at each end; both are segment ends +``` + +Ground is common if both boards are USB-powered from the same host. If only one +is, run **VSYS → VSYS** plus **GND → GND** between them — not `3V3(OUT)`, which +back-feeds the regulator on the unpowered board. + +### Running + +```bash +cargo build --release -p wiredsensor-bridge +cp target/thumbv6m-none-eabi/release/wiredsensor-bridge /tmp/bridge.elf +picotool load -x /tmp/bridge.elf # hold BOOTSEL while plugging in +``` + +The bridge is `16c0:27de`, the node `16c0:27dd`. Find it with: + +```bash +for d in /dev/ttyACM*; do udevadm info -q property -n $d | grep -q 27de && echo $d; done +``` + +```bash +./tools/wiredsensor.py --port /dev/ttyACM2 measure +./tools/wiredsensor.py --port /dev/ttyACM2 monitor +./tools/wiredsensor.py --port /dev/ttyACM2 -v test # -v dumps bus traffic +``` + +### What the suite covers + +15 checks. Framing and CRC arithmetic are already covered by the host tests; +what only a real bus can verify is everything timing-dependent: + +- **Silence where required** — frames for another unit, broadcasts, bad CRCs, + truncated frames and frames whose `LEN` lies must all produce *no reply*. A + node that wrongly answered would collide with whoever was actually addressed. +- **Driver-enable turnaround** — every intact reply is evidence that `DE` was + held past the final stop bit. Release it early and the last byte truncates. +- **No state leakage** between back-to-back requests. +- **Binary transparency** across the full legal frame-size range. +- **Counter integrity** — inject one corrupt frame, assert `crc_errors` rises by + exactly one. + +This earned its keep immediately: it caught a frame-truncation bug that both the +host tests and the sensor verification were structurally incapable of reaching, +because it needed real bus timing *and* a frame longer than the RX FIFO +watermark. See the first item under *Details that are easy to get wrong*. + +Frames must fit one 64-byte USB packet, since each host write becomes exactly one +DE-bracketed transmission. Every real command is at most 21 bytes; only an +oversized `PING` can approach the limit. + ## Design notes ### Why RTIC rather than Embassy @@ -324,15 +415,31 @@ The ISR deliberately does no parsing. At 19200 baud a character arrives every ~520 µs, roughly 65,000 core cycles, so there is ample slack; keeping the handler to a FIFO drain is what bounds the jitter everything else sees. -### Three details that are easy to get wrong +### Details that are easy to get wrong -**`RTIM` alone is not a sufficient frame delimiter.** It only fires while the RX -FIFO is non-empty, so a frame whose length lands exactly on the FIFO watermark -can be drained by the watermark interrupt and never produce a timeout. The -authoritative delimiter is therefore a monotonic timer measured from the last -received byte; `RTIM` merely makes the common case prompt. `frame_gap` re-reads -the arrival timestamp and sleeps again rather than cancelling and respawning a -timer per byte. +**`RTIM` is not a sufficient frame delimiter, and an interrupt timestamp is not +a trustworthy end-of-frame.** Two separate traps here, and the second one bit us +for real. + +`RTIM` only fires while the RX FIFO is non-empty, so a frame drained exactly +empty by the watermark interrupt never produces a timeout at all. + +Worse: once the watermark interrupt has fired mid-frame — at 16 bytes, with the +FIFO 32 deep — the *remaining* bytes sit below the watermark and raise no +interrupt whatsoever until the timeout eventually arrives. A gap measured from +the timestamp the ISR left behind therefore expires while the tail of the frame +is still arriving, and the node parses a prefix whose `LEN` disagrees with its +length. Every frame of 16 bytes or more was silently rejected this way; frames +under 16 never trip the watermark, so their only interrupt is the timeout, by +which point every byte is drained and the timestamp is honest. That is why all +five real commands worked and only an oversized `PING` exposed it. + +So `frame_gap` drains the FIFO *itself* before each decision rather than trusting +the ISR's timestamp, which lets a byte that has landed push the deadline back. +The gap is then measured from when the byte was observed rather than when it +arrived, making the reply up to one gap period later than strictly necessary — +1.8 ms is a fair price. It re-reads and sleeps again rather than cancelling and +respawning a timer per byte. **`DE` must be held until the last stop bit is gone.** Releasing it when the TX FIFO empties truncates the final character for every listener — a fault that diff --git a/bridge/Cargo.toml b/bridge/Cargo.toml index 2721c4c..4afb921 100644 --- a/bridge/Cargo.toml +++ b/bridge/Cargo.toml @@ -12,6 +12,10 @@ test = false bench = false [dependencies] +# Only for the shared line rate; the bridge deliberately knows nothing about +# the protocol itself. +wiredsensor-core = { workspace = true } + rp2040-hal = { version = "0.12", features = ["rt", "critical-section-impl"] } rp2040-boot2 = "0.3" diff --git a/bridge/src/main.rs b/bridge/src/main.rs index 85d9bea..9889358 100644 --- a/bridge/src/main.rs +++ b/bridge/src/main.rs @@ -42,9 +42,9 @@ use usb_device::{ device::{StringDescriptors, UsbDeviceBuilder, UsbVidPid}, }; use usbd_serial::SerialPort; - -/// Line rate. Must match the sensor node's `board::BAUD_RATE`. -const BAUD_RATE: u32 = 19_200; +// The one thing shared with the node: both ends of the bus must agree on the +// line rate, so it is defined once in the core crate rather than copied here. +use wiredsensor_core::timing::BAUD_RATE; /// Crystal fitted to the board. const XTAL_FREQ_HZ: u32 = 12_000_000; diff --git a/core/src/timing.rs b/core/src/timing.rs index 18ef7f2..9567895 100644 --- a/core/src/timing.rs +++ b/core/src/timing.rs @@ -1,12 +1,29 @@ -//! Wire timing derived from the line rate. +//! The line rate, and the wire timing derived from it. //! -//! Kept here rather than in the firmware so the arithmetic — which decides -//! whether frames are delimited correctly and whether the transceiver is -//! released at the right moment — is covered by host tests. +//! Kept here rather than in the firmware for two reasons. The arithmetic — +//! which decides whether frames are delimited correctly and whether the +//! transceiver is released at the right moment — is covered by host tests. And +//! [`BAUD_RATE`] is a property of the *segment*, not of any one board: the node +//! and every master on the bus must agree on it, so it is defined once here +//! rather than copied into each firmware and kept in sync by hand. + +/// Line rate for the whole segment. +/// +/// 19200 8N1 is the Modbus default and a safe starting point for long runs. +/// Changing it here changes it for every device built from this workspace, +/// which is the point — a mismatch between two ends is silent and looks like +/// random CRC failures. +pub const BAUD_RATE: u32 = 19_200; /// Bits on the wire per character: 1 start + 8 data + 1 stop, i.e. 8N1. pub const BITS_PER_CHAR: u32 = 10; +/// Inter-frame idle gap at [`BAUD_RATE`], in microseconds. +pub const INTER_FRAME_GAP_US: u32 = inter_frame_gap_us(BAUD_RATE); + +/// Time to clock out one character at [`BAUD_RATE`], in microseconds. +pub const CHAR_TIME_US: u32 = char_time_us(BAUD_RATE); + /// Idle time that delimits one frame from the next, in microseconds. /// /// Modbus specifies 3.5 character times, but pins the value at a fixed 1750 µs @@ -79,4 +96,13 @@ mod tests { assert_eq!(tx_time_us(1, 19_200), char_time_us(19_200)); assert_eq!(tx_time_us(21, 19_200), 21 * 520); } + + /// The derived constants must track whatever [`BAUD_RATE`] is set to, so + /// changing the line rate cannot leave a stale gap or character time behind. + #[test] + fn derived_constants_follow_the_configured_rate() { + assert_eq!(INTER_FRAME_GAP_US, inter_frame_gap_us(BAUD_RATE)); + assert_eq!(CHAR_TIME_US, char_time_us(BAUD_RATE)); + assert!(INTER_FRAME_GAP_US > CHAR_TIME_US); + } } diff --git a/firmware/src/board.rs b/firmware/src/board.rs index 47f9207..e57f4eb 100644 --- a/firmware/src/board.rs +++ b/firmware/src/board.rs @@ -86,15 +86,11 @@ pub const XTAL_FREQ_HZ: u32 = 12_000_000; // -------------------------------------------------------------- bus timing --- -/// RS485 line rate. 19200 8N1 is the Modbus default and a safe starting point -/// for long segments; every timing constant below derives from it. -pub const BAUD_RATE: u32 = 19_200; - -/// Idle time that delimits one frame from the next, in microseconds. -pub const INTER_FRAME_GAP_US: u32 = timing::inter_frame_gap_us(BAUD_RATE); - -/// Time to clock out one character at [`BAUD_RATE`], in microseconds. -pub const CHAR_TIME_US: u32 = timing::char_time_us(BAUD_RATE); +// Re-exported rather than redefined. The line rate belongs to the segment, not +// to this board, so it lives in `wiredsensor_core::timing` where the bridge +// firmware reads the same value — a mismatch between two ends of the bus is +// silent and presents as random CRC failures. Change it there. +pub use timing::{BAUD_RATE, CHAR_TIME_US, INTER_FRAME_GAP_US}; // ------------------------------------------------------------ sensor timing ---