From 23ddc15557879b6cc526eda276f323e77970a5d5 Mon Sep 17 00:00:00 2001 From: Stephen Waits Date: Fri, 24 Apr 2026 01:44:47 -0600 Subject: [PATCH] fix(radio): consume upstream lora-phy pre-command BUSY check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pick up the proper fix from our fork at swaits/lora-rs@fix/sx126x-busy-race: move the ready check from *after* to *before* each SPI op per the Semtech manuals (SX1261/2 §8.3.1, LR1121 UM §3 / §3.5, LR1110 DS §1.2.5) and rename InterfaceVariant::wait_on_busy to wait_until_ready. Firmware side is surgical: since the upstream fix keeps the GenericSx126xInterfaceVariant::new signature at 5 args (no DelayNs threading), there is nothing to change in the four nRF / ESP / RP2040 board files beyond a one-line comment rename (wait_on_busy -> wait_until_ready) in the BUSY-pullup comment on the two nRF boards. Wire in the [patch.crates-io] override pointing at ../../lora-rs/lora-phy, bump the dep spec to the pre-release 3.0.2-alpha that matches the fork, and enable the defmt-03 feature that upstream moved behind a gate between 3.0.1 and the fix branch. TODO.md rewritten to track the new approach: PR body template with verbatim citations from all three Semtech chip manuals (SX1261/2, LR1121 UM, LR1110 DS) plus the SX1276 pin table confirming the family has no BUSY line. No change to firmware behavior beyond picking up the upstream fix, which takes first-TX-after-boot success from ~50% to 20/20 on the Seeed Wio Tracker L1 repro board. --- Cargo.lock | 224 +++++++++++++++++++++++++++++++++++++++++++++++++---- Cargo.toml | 10 ++- TODO.md | 59 ++++++++++++++ 3 files changed, 278 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dd233bc..eee5315 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,17 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "aes" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "66bd29a732b644c0431c6140f370d097879203d79b80c94a6747ba0872adaef8" +dependencies = [ + "cipher", + "cpubits", + "cpufeatures 0.3.0", +] + [[package]] name = "ahash" version = "0.8.12" @@ -127,6 +138,15 @@ dependencies = [ "generic-array", ] +[[package]] +name = "block-buffer" +version = "0.12.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdd35008169921d80bc60d3d0ab416eecb028c4cd653352907921d95084790be" +dependencies = [ + "hybrid-array", +] + [[package]] name = "byte-slice-cast" version = "1.2.3" @@ -172,6 +192,33 @@ dependencies = [ "pure-rust-locales", ] +[[package]] +name = "cipher" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e34d8227fe1ba289043aeb13792056ff80fd6de1a9f49137a5f499de8e8c78ea" +dependencies = [ + "crypto-common 0.2.1", + "inout", +] + +[[package]] +name = "cmac" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac78aa94ce13e432b332a4d1bf2eff167d3a2520188ee05b337180a42fd2e62e" +dependencies = [ + "cipher", + "dbl", + "digest 0.11.2", +] + +[[package]] +name = "cmov" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f88a43d011fc4a6876cb7344703e297c71dda42494fee094d5f7c76bf13f746" + [[package]] name = "codespan-reporting" version = "0.11.1" @@ -225,6 +272,12 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "cpubits" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5ef0c543070d296ea414df2dd7625d1b24866ce206709d8a4a424f28377f5861" + [[package]] name = "cpufeatures" version = "0.2.17" @@ -234,6 +287,15 @@ dependencies = [ "libc", ] +[[package]] +name = "cpufeatures" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b2a41393f66f16b0823bb79094d54ac5fbd34ab292ddafb9a0456ac9f87d201" +dependencies = [ + "libc", +] + [[package]] name = "crc-any" version = "2.5.0" @@ -265,6 +327,24 @@ dependencies = [ "typenum", ] +[[package]] +name = "crypto-common" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "77727bb15fa921304124b128af125e7e3b968275d1b108b379190264f4423710" +dependencies = [ + "hybrid-array", +] + +[[package]] +name = "ctutils" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d5515a3834141de9eafb9717ad39eea8247b5674e6066c404e8c4b365d2a29e" +dependencies = [ + "cmov", +] + [[package]] name = "darling" version = "0.20.11" @@ -368,6 +448,15 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "dbl" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0d7a944e61df464668c5f51f56cc667396a8821434273112948ea0b66e405d7" +dependencies = [ + "hybrid-array", +] + [[package]] name = "debug-helper" version = "0.3.13" @@ -442,8 +531,19 @@ version = "0.10.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" dependencies = [ - "block-buffer", - "crypto-common", + "block-buffer 0.10.4", + "crypto-common 0.1.7", +] + +[[package]] +name = "digest" +version = "0.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4850db49bf08e663084f7fb5c87d202ef91a3907271aff24a94eb97ff039153c" +dependencies = [ + "block-buffer 0.12.0", + "crypto-common 0.2.1", + "ctutils", ] [[package]] @@ -1022,7 +1122,7 @@ dependencies = [ "cfg-if", "critical-section", "delegate", - "digest", + "digest 0.10.7", "document-features", "embassy-embedded-hal", "embassy-futures", @@ -1235,6 +1335,12 @@ dependencies = [ "vcell", ] +[[package]] +name = "fastrand" +version = "2.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f1f227452a390804cdb637b74a86990f2a7d7ba4b7d5693aac9b4dd6defd8d6" + [[package]] name = "find-msvc-tools" version = "0.1.9" @@ -1283,12 +1389,42 @@ dependencies = [ "gcd", ] +[[package]] +name = "futures" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b147ee9d1f6d097cef9ce628cd2ee62288d963e16fb287bd9286455b241382d" +dependencies = [ + "futures-channel", + "futures-core", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + +[[package]] +name = "futures-channel" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "07bbe89c50d7a535e539b8c17bc0b49bdb77747034daa8087407d655f3f7cc1d" +dependencies = [ + "futures-core", + "futures-sink", +] + [[package]] name = "futures-core" version = "0.3.32" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7e3450815272ef58cec6d564423f6e755e25379b217b0bc688e295ba24df6b1d" +[[package]] +name = "futures-io" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cecba35d7ad927e23624b22ad55235f2239cfa44fd10428eecbeba6d6a717718" + [[package]] name = "futures-sink" version = "0.3.32" @@ -1308,6 +1444,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "389ca41296e6190b48053de0321d02a77f32f8a5d2461dd38762c0593805c6d6" dependencies = [ "futures-core", + "futures-sink", "futures-task", "pin-project-lite", ] @@ -1405,6 +1542,12 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hex" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" + [[package]] name = "hsmc" version = "0.2.0" @@ -1429,6 +1572,15 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "hybrid-array" +version = "0.4.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3944cf8cf766b40e2a1a333ee5e9b563f854d5fa49d6a8ca2764e97c6eddb214" +dependencies = [ + "typenum", +] + [[package]] name = "ident_case" version = "1.0.1" @@ -1454,6 +1606,15 @@ dependencies = [ "rustversion", ] +[[package]] +name = "inout" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4250ce6452e92010fdf7268ccc5d14faa80bb12fc741938534c58f16804e03c7" +dependencies = [ + "hybrid-array", +] + [[package]] name = "instability" version = "0.3.12" @@ -1488,7 +1649,7 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cb26cec98cce3a3d96cbb7bced3c4b16e3d13f27ec56dbd62cbc8f39cfb9d653" dependencies = [ - "cpufeatures", + "cpufeatures 0.2.17", ] [[package]] @@ -1572,22 +1733,57 @@ dependencies = [ [[package]] name = "lora-modulation" version = "0.1.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2c96c4a41a1f1ad04de765306e1829c7007a37898fe4033204fa82cf2e055624" +dependencies = [ + "defmt 0.3.100", +] [[package]] name = "lora-phy" -version = "3.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "61471c3b2909789e3332083577f6cf6c41a4fcf37674ef15156bcbb20504ac65" +version = "3.0.2-alpha" dependencies = [ "defmt 0.3.100", + "document-features", "embedded-hal 1.0.0", "embedded-hal-async", "lora-modulation", + "lorawan-device", "num-traits", ] +[[package]] +name = "lorawan" +version = "0.9.1" +dependencies = [ + "aes", + "cmac", + "defmt 0.3.100", + "hex", + "lorawan-macros", +] + +[[package]] +name = "lorawan-device" +version = "0.12.2" +dependencies = [ + "defmt 0.3.100", + "document-features", + "fastrand", + "futures", + "heapless 0.9.2", + "lora-modulation", + "lorawan", + "rand_core 0.6.4", +] + +[[package]] +name = "lorawan-macros" +version = "0.1.0" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", +] + [[package]] name = "matchers" version = "0.2.0" @@ -2214,8 +2410,8 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e3bf829a2d51ab4a5ddf1352d8470c140cadc8301b2ae1789db023f01cedd6ba" dependencies = [ "cfg-if", - "cpufeatures", - "digest", + "cpufeatures 0.2.17", + "digest 0.10.7", ] [[package]] @@ -2225,8 +2421,8 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" dependencies = [ "cfg-if", - "cpufeatures", - "digest", + "cpufeatures 0.2.17", + "digest 0.10.7", ] [[package]] @@ -2241,7 +2437,7 @@ version = "0.10.8" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "75872d278a8f37ef87fa0ddbda7802605cb18344497949862c0d4dcb291eba60" dependencies = [ - "digest", + "digest 0.10.7", "keccak", ] diff --git a/Cargo.toml b/Cargo.toml index aa44a2c..b64fd57 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ ucobs = { version = "0.3", features = ["legacy-msrv"] } heapless = "0.9" donglora-protocol = "1" hsmc = { version = "0.2", default-features = false, features = ["embassy"] } -lora-phy = "3.0" +lora-phy = { version = "3.0.2-alpha", features = ["defmt-03"] } # matches the in-tree [patch.crates-io] override; drop the pre-release selector once upstream releases the fix portable-atomic = { version = "1", optional = true, default-features = false, features = ["critical-section"] } static_cell = "2.1" @@ -212,6 +212,14 @@ esp-rtos = { git = "https://github.com/esp-rs/esp-hal.git", branch = "main" esp-backtrace = { git = "https://github.com/esp-rs/esp-hal.git", branch = "main" } esp-println = { git = "https://github.com/esp-rs/esp-hal.git", branch = "main" } +# Local fix for a BUSY-assertion race: lora-phy currently polls BUSY +# *after* each SPI op, but SX1261-2 §8.1.4 specifies a *pre*-command +# check (same pattern as RadioLib's SPIwriteStream). This override pulls +# in the corrected ordering and renames `wait_on_busy` to +# `wait_until_ready`. See TODO.md for the PR body. Remove this line once +# the fix is merged upstream + released on crates.io. +lora-phy = { path = "../../lora-rs/lora-phy" } + [lints.clippy] undocumented_unsafe_blocks = "deny" diff --git a/TODO.md b/TODO.md index d0c21f9..3d4d89b 100644 --- a/TODO.md +++ b/TODO.md @@ -1,5 +1,64 @@ # Firmware TODO List +## `lora-phy` fix shipped locally; upstream PR pending submission + +Branch: [`swaits/lora-rs @ fix/sx126x-busy-race`](https://github.com/swaits/lora-rs/tree/fix/sx126x-busy-race). +PR not yet opened. Firmware consumes the fix via `[patch.crates-io]` +pointing at `../../lora-rs/lora-phy`; drop the patch line + pre-release +selector once upstream releases. + +Hardware-verified on Wio Tracker L1: 20/20 clean-boot TX after the fix +(vs ~50% before). Additive fix: adds pre-command BUSY check, keeps +existing post-command wait in place. + +### PR body + +``` +Title: phy: pre-command BUSY check per Semtech datasheets + +## Problem + +While bringing up a downstream project (donglora), I hit a race where TX commands would occasionally fail. Tracked it down to lora-phy not following the BUSY-handshake algorithm Semtech recommends: the host should wait on BUSY *before* issuing each command, not after. + +Details: `SpiInterface::{write, write_with_payload, read, +read_with_status}` polls BUSY *after* each SPI op. + +On fast MCUs (nRF52840 at 64 MHz, release build) this is racy — `embedded-hal-async`'s `wait_for_low` returns immediately if the pin is already low, and the chip takes up to T_SW ns (SX1261/2 §8.3, Fig. 8-3) to assert BUSY after NSS rises. The host polls inside that pre-assertion window, sees a stale LOW, fires the next command while the chip is still busy, and the chip silently drops it. First `lora.tx()` after a clean boot on Seeed Wio Tracker L1 hangs ~50% of the time. + +## Authority + +- [SX1261/2 datasheet, DS.SX1261-2.W.APP Rev. 1.2](https://cdn.sparkfun.com/assets/6/b/5/1/4/SX1262_datasheet.pdf) §8.3.1 + "BUSY Control Line": *"it is essential to wait for the BUSY line to + go low before sending an SPI command."* +- [LR1121 User Manual, UM.LR1121.W.APP Rev. 1.2](https://files.waveshare.com/wiki/Core1121/UserManual_LR1121_v1_2.pdf) §3 + "Host-Controller Interface": *"it is necessary to check the status + of BUSY prior to sending a command."* (Applies to the LR11xx family + — see also [LR1110 DS Rev. 1.1](https://download.mikroe.com/documents/datasheets/LR1110_datasheet.pdf) §1.2.5.) +- [RadioLib](https://github.com/jgromes/RadioLib/blob/cffd5af/src/Module.cpp) + `Module::SPIwriteStream` / `SPIreadStream` call `waitForBusy()` at + the start of every transaction. +- The [SX1276/77/78/79 datasheet](https://cdn.sparkfun.com/assets/7/7/3/2/2/SX1276_Datasheet.pdf) + Table 2 exposes no BUSY pin, so + `GenericSx127xInterfaceVariant::wait_on_busy` stays a no-op and the + change is behaviorally identical for SX127x. + +## Fix + +Replace the post-command BUSY wait with a pre-command BUSY check at the start of each `SpiInterface` / `Lr1110SpiInterface` read/write method. Drop the redundant explicit `wait_on_busy()` calls in `sx126x` (after Calibrate, `ensure_ready` non-sleep arm) and `lr1110::mod` (4 regmem/bootloader sites) — all now covered by the interface pre-wait. + +The `is_sleep_command` flag keeps its name but now gates the pre-wait; set `true` only when BUSY is held HIGH by design (the wake-from-sleep GetStatus pulse in `sx126x::ensure_ready`). Two callsite flips as a result: `ensure_ready` Sleep arm `false`→`true`; `set_sleep` `true`→`false`. + +No public API signature changes. + +## Test + +- `cargo clippy -p lora-phy --features defmt-03 -- -D warnings` clean. +- Hardware: Seeed Wio Tracker L1 (nRF52840 + SX1262), 20 consecutive + clean-boot TX cycles → 20/20 (vs ~50% before). +``` + +--- + ## Drop `[patch.crates-io]` + upgrade esp-hal / esp-rtos + add `esp_app_desc!()` + bump espflash to v4 Single coordinated change that clears our last remaining dep-graph debt