SPI ExpressLRS 4.0 support - #14932
Conversation
|
Do you want to test this code? You can flash it directly from the Betaflight App:
WARNING: It may be unstable. Use only for testing! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConditional ELRS V3/V4 support added across the RX SPI driver: version-aware branches for RC channel mapping, telemetry state and payload sizing, CRC/nonce initialization derived from UID, binding/init flows, FHSS sequence logic, and packet/type definitions. Changes
Sequence Diagram(s)mermaid (Note: diagram shows high-level interactions for binding/init, CRC init, telemetry payload retrieval and SPI transmission.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/drivers/rx/rx_sx1280.c`:
- Around line 847-849: When expressLrsIsFhssReq() is false we must still call
sx1280SetFreqComplete(0) but preserve the telemetry vs non-telemetry branching:
after calling sx1280SetFreqComplete(0) check expressLrsTelemRespReq() and if it
returns true call sx1280MarkBusy() and
sx1280SetBusyFn(sx1280SendTelemetryBuffer) to protect the telemetry SPI path;
otherwise call sx1280EnableExti() to re-enable the EXTI line. This keeps
sx1280Processing/sx1280EnableBusy() protection for the telemetry path while
ensuring the non-telemetry branch restores interrupts (avoid leaving EXTI
disabled).
In `@src/main/rx/expresslrs_telemetry.c`:
- Around line 262-269: In receiveMspData(), the else-if branch that handles
packageIndex == 1 mistakenly resets sender state variables
currentPackage/currentOffset; change those assignments to update the MSP
receiver state instead: set mspCurrentPackage to 2 (because we accept package 1
and the next expected package is 2), set mspCurrentOffset to 0, and leave
acceptData = true; ensure you do not modify currentPackage or currentOffset in
this branch so the telemetry sender state is not corrupted.
In `@src/test/unit/rx_spi_expresslrs_unittest.cc`:
- Line 493: The test stub for getCurrentTelemetryPayload has the wrong type for
its second parameter (uses size_t) causing a linkage/signature mismatch with the
header; update the stub function signature in rx_spi_expresslrs_unittest.cc to
use uint8_t for the maxLen parameter so it exactly matches the declaration of
uint8_t getCurrentTelemetryPayload(uint8_t *outData, uint8_t maxLen) (preserving
extern "C" semantics and the same parameter names if present).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/unit/rx_spi_expresslrs_telemetry_unittest.cc`:
- Around line 100-101: The computation of maxLength can become 0 when
payloadSize is an exact multiple of ELRS_TELEMETRY_BYTES_PER_CALL which breaks
end-of-data logic; update the logic around maxLength (used before calling
getCurrentTelemetryPayload) to compute the remainder first and only use the
remainder when it is non-zero on the last package (otherwise use
ELRS_TELEMETRY_BYTES_PER_CALL), so bytesLastPayload and downstream checks
((currentOffset + bytesLastPayload) >= length) behave correctly for frames like
Attitude (10 bytes).
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/test/unit/rx_spi_expresslrs_unittest.cc`:
- Line 493: The test stub getCurrentTelemetryPayload in
rx_spi_expresslrs_unittest.cc should match the updated function signature;
confirm the stub definition uint8_t getCurrentTelemetryPayload(uint8_t *,
uint8_t) { return 0; } is present and keep it aligned with the production
declaration so the unit tests link correctly to getCurrentTelemetryPayload.
|
The tests are failing because they are tests that expect ELRSv3 values, I will resolve those /rolleyes I've also found that there is an issue with TX to RX data upload that is not behaving properly due to the RX presenting the wrong ack value some of the time. This will take me a bit to track down. |
|
Resolved the issue with the uplink data being unreliable and updated the tests, but 50Hz now really struggles to connect, if at all. There's something in the timing changes that broke it because it was working prior to that. Looks like I gotta spend more time on it. Sorry about the premature PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/rx/expresslrs.c (1)
494-501:⚠️ Potential issue | 🟡 MinorFrequency-offset adjustment now fires at nonce 0 (immediately after connection).
Changing from
nonceRX % 8 == 1to== 0means the adjustment fires on the very first tick (nonce 0) rather than the second. At low packet rates (50Hz), this earlier adjustment could interact with the phase-lock filter before it has meaningful data. Given the PR-noted 50Hz connection difficulty, this timing change may be a contributing factor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/rx/expresslrs.c` around lines 494 - 501, The frequency-offset adjustment is currently triggered on the very first tick because the condition uses receiver.nonceRX % 8 == 0, causing adjustments at nonce 0; revert or guard this so it does not run immediately after connection. Update the check around receiver.nonceRX (used with pl.offsetUs and the calls expressLrsTimerIncreaseFrequencyOffset()/expressLrsTimerDecreaseFrequencyOffset()) to skip nonce 0 — for example require receiver.nonceRX > 0 and (receiver.nonceRX % 8 == 1) or otherwise ensure the first adjustment only occurs after at least one valid packet/period has elapsed.src/test/unit/rx_spi_expresslrs_unittest.cc (1)
190-206:⚠️ Potential issue | 🟠 MajorLoop bound mismatch: validation should iterate up to
seqCount, notELRS_NR_SEQUENCE_ENTRIES.
fhssGenSequence()populatesfhssSequenceonly for indices 0 toseqCount-1(lines 206 and 216 in expresslrs_common.c). For ISM2400 and FCC915,seqCountis 240, which is less thanELRS_NR_SEQUENCE_ENTRIES. The test assertions at lines 196–197 and 204–205 validate allELRS_NR_SEQUENCE_ENTRIESentries, comparing uninitialized/unchanged entries beyondseqCount. Align loop bounds withseqCountto match the actual sequence generation.Also,
UNUSED(expectedSequence)at line 190 contradicts the variable's immediate use in the validation loops below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/unit/rx_spi_expresslrs_unittest.cc` around lines 190 - 206, The test currently validates all ELRS_NR_SEQUENCE_ENTRIES entries even though fhssGenSequence(seed, ISM2400/FCC915) only fills fhssSequence up to seqCount; change the validation loops in rx_spi_expresslrs_unittest.cc to iterate from i = 0 to i < seqCount (use the seqCount returned or available from fhssGenSequence context) instead of ELRS_NR_SEQUENCE_ENTRIES, and remove or update the UNUSED(expectedSequence) macro since expectedSequence is actually used in the EXPECT_EQ comparisons; reference fhssGenSequence, fhssSequence, expectedSequence, seqCount and ELRS_NR_SEQUENCE_ENTRIES to locate the affected code.
🧹 Nitpick comments (4)
src/main/rx/expresslrs_impl.h (1)
22-30:ELRS_TLM_PACKETcollides withELRS_RC_DATA_PACKET(both0x00) in V4 mode.In the
#else(V4) branch,ELRS_TLM_PACKET = 0x00is the same value asELRS_RC_DATA_PACKET = 0x00. This is valid C but worth verifying it's intentional — anyswitchon packet type that includes both cases would be unreachable for one of them. From the code inexpresslrs.c,ELRS_TLM_PACKETis only used to set outgoing telemetry packet types (downlink), and theswitchinprocessRFPacketonly handles uplink types (ELRS_RC_DATA_PACKET,ELRS_MSP_DATA_PACKET,ELRS_SYNC_PACKET), so this appears safe.A brief comment documenting the intentional overlap would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/rx/expresslrs_impl.h` around lines 22 - 30, ELRS_TLM_PACKET currently shares the 0x00 value with ELRS_RC_DATA_PACKET in the V4 (`#else`) branch which is intentional; add a concise comment next to the enum entries (around ELRS_TLM_PACKET in expresslrs_impl.h) stating that ELRS_TLM_PACKET intentionally overlaps ELRS_RC_DATA_PACKET in V4 because ELRS_TLM_PACKET is only used for setting outgoing telemetry types (downlink) and the packet dispatch in processRFPacket (in expresslrs.c) only switches on uplink types (ELRS_RC_DATA_PACKET, ELRS_MSP_DATA_PACKET, ELRS_SYNC_PACKET), so the overlap is safe and by design.src/main/rx/expresslrs.c (3)
284-295: Preprocessor-wrappedif/elseis functional but fragile.The pattern where
#ifdefwraps only theifbranch and theelsekeyword (lines 284–291), leaving the else-body as a standalone block for V4, is correct but easily misread. A stray edit could break the flow.Consider restructuring for clarity:
♻️ Suggested restructuring
- `#ifdef` USE_ELRSV3 - // ELRS V3 switched between 6 and 7 bit mode depending on TLM ratio - // ELRS V4 always uses 6 bit mode - if (currTlmDenom < 2 || currTlmDenom > 4) { + `#ifdef` USE_ELRSV3 + if (currTlmDenom < 2 || currTlmDenom > 4) { + // ELRS V3: 7-bit mode when TLM ratio outside 2..4 bins = 127; switchValue = switchByte & 0x7F; // 7-bit - } else - `#endif` - { - bins = 63; - switchValue = switchByte & 0x3F; // 6-bit + } else { + bins = 63; + switchValue = switchByte & 0x3F; // 6-bit } + `#else` + // ELRS V4: always 6-bit mode + bins = 63; + switchValue = switchByte & 0x3F; + `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/rx/expresslrs.c` around lines 284 - 295, The preprocessor wraps only the if branch (USE_ELRSV3) making the if/else fragile; refactor so the control flow is explicit and safe: set bins and switchValue to the ELRS V4 (6-bit) defaults first (use symbols bins, switchValue, switchByte), then under `#ifdef` USE_ELRSV3 add a full if that checks currTlmDenom and overrides bins and switchValue (using switchByte & 0x7F) when the V3 condition applies; this removes the cross-boundary else and keeps the conditional logic clear and resilient to edits.
739-745: V4 sync validation uses fewer UID bytes — comment may be stale.V4 only checks
UID[4]+ maskedUID[5](14 bits of connection matching) while V3 also checksUID[3](22 bits). The comment on line 748 ("first 18 bits") doesn't match either version. Consider updating the comment to reflect the actual bit counts for each protocol version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/rx/expresslrs.c` around lines 739 - 745, Update the stale comment that currently says "first 18 bits" to accurately describe the sync/UID matching for both protocol versions: state that under USE_ELRSV3 the code compares otaPktPtr->sync.UID3 and UID4 (total 22 bits of UID matching) while in V4 it only compares otaPktPtr->sync.UID4 plus the masked UID5 bits (14 bits of connection matching); reference the symbols otaPktPtr->sync.UID3, otaPktPtr->sync.UID4, receiver.UID[3], receiver.UID[4], receiver.UID[5] and the USE_ELRSV3 macro so the comment clearly documents which bits are checked per protocol.
469-478: V4nonceValidatorisuint8_twhile V3 usesuint16_t— works but inconsistent.In the V4 branch (line 476),
nonceValidatorisuint8_tsince it holdsnonceRX + (0 or 1). At line 478 it's XORed with theuint16_tcrcInitializer. This is fine due to implicit promotion, and aligns with the V4 CRC design (version in high byte, nonce in low byte per lines 668–670). No bug, but the type inconsistency between branches is worth noting for readability.The F500 timing hack (lines 473–476) is well-documented. Given the PR notes about 50Hz connection struggles, this area may warrant further investigation if the issue persists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/rx/expresslrs.c` around lines 469 - 478, Declare nonceValidator with the same type in both branches (use uint16_t instead of uint8_t) to remove the inconsistency; in the non-USE_ELRSV3 branch change the declaration of nonceValidator to uint16_t and keep the assignment using receiver.nonceRX + (isF500 ? 0 : 1) (or cast the result to uint16_t) so the subsequent calcCrc14 call (crcInitializer ^ nonceValidator) operates on consistent 16-bit values; reference symbols: nonceValidator, receiver.nonceRX, receiver.rateIndex, domainIsTeam24, crcInitializer, calcCrc14.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/rx/expresslrs.c`:
- Around line 494-501: The frequency-offset adjustment is currently triggered on
the very first tick because the condition uses receiver.nonceRX % 8 == 0,
causing adjustments at nonce 0; revert or guard this so it does not run
immediately after connection. Update the check around receiver.nonceRX (used
with pl.offsetUs and the calls
expressLrsTimerIncreaseFrequencyOffset()/expressLrsTimerDecreaseFrequencyOffset())
to skip nonce 0 — for example require receiver.nonceRX > 0 and (receiver.nonceRX
% 8 == 1) or otherwise ensure the first adjustment only occurs after at least
one valid packet/period has elapsed.
In `@src/test/unit/rx_spi_expresslrs_unittest.cc`:
- Around line 190-206: The test currently validates all ELRS_NR_SEQUENCE_ENTRIES
entries even though fhssGenSequence(seed, ISM2400/FCC915) only fills
fhssSequence up to seqCount; change the validation loops in
rx_spi_expresslrs_unittest.cc to iterate from i = 0 to i < seqCount (use the
seqCount returned or available from fhssGenSequence context) instead of
ELRS_NR_SEQUENCE_ENTRIES, and remove or update the UNUSED(expectedSequence)
macro since expectedSequence is actually used in the EXPECT_EQ comparisons;
reference fhssGenSequence, fhssSequence, expectedSequence, seqCount and
ELRS_NR_SEQUENCE_ENTRIES to locate the affected code.
---
Nitpick comments:
In `@src/main/rx/expresslrs_impl.h`:
- Around line 22-30: ELRS_TLM_PACKET currently shares the 0x00 value with
ELRS_RC_DATA_PACKET in the V4 (`#else`) branch which is intentional; add a concise
comment next to the enum entries (around ELRS_TLM_PACKET in expresslrs_impl.h)
stating that ELRS_TLM_PACKET intentionally overlaps ELRS_RC_DATA_PACKET in V4
because ELRS_TLM_PACKET is only used for setting outgoing telemetry types
(downlink) and the packet dispatch in processRFPacket (in expresslrs.c) only
switches on uplink types (ELRS_RC_DATA_PACKET, ELRS_MSP_DATA_PACKET,
ELRS_SYNC_PACKET), so the overlap is safe and by design.
In `@src/main/rx/expresslrs.c`:
- Around line 284-295: The preprocessor wraps only the if branch (USE_ELRSV3)
making the if/else fragile; refactor so the control flow is explicit and safe:
set bins and switchValue to the ELRS V4 (6-bit) defaults first (use symbols
bins, switchValue, switchByte), then under `#ifdef` USE_ELRSV3 add a full if that
checks currTlmDenom and overrides bins and switchValue (using switchByte & 0x7F)
when the V3 condition applies; this removes the cross-boundary else and keeps
the conditional logic clear and resilient to edits.
- Around line 739-745: Update the stale comment that currently says "first 18
bits" to accurately describe the sync/UID matching for both protocol versions:
state that under USE_ELRSV3 the code compares otaPktPtr->sync.UID3 and UID4
(total 22 bits of UID matching) while in V4 it only compares
otaPktPtr->sync.UID4 plus the masked UID5 bits (14 bits of connection matching);
reference the symbols otaPktPtr->sync.UID3, otaPktPtr->sync.UID4,
receiver.UID[3], receiver.UID[4], receiver.UID[5] and the USE_ELRSV3 macro so
the comment clearly documents which bits are checked per protocol.
- Around line 469-478: Declare nonceValidator with the same type in both
branches (use uint16_t instead of uint8_t) to remove the inconsistency; in the
non-USE_ELRSV3 branch change the declaration of nonceValidator to uint16_t and
keep the assignment using receiver.nonceRX + (isF500 ? 0 : 1) (or cast the
result to uint16_t) so the subsequent calcCrc14 call (crcInitializer ^
nonceValidator) operates on consistent 16-bit values; reference symbols:
nonceValidator, receiver.nonceRX, receiver.rateIndex, domainIsTeam24,
crcInitializer, calcCrc14.
|
thanks @CapnBry lets me put it on a build and flight test |
sugaarK
left a comment
There was a problem hiding this comment.
flight tested!! flys good.. first link lockup is a bit messy at 500hz but other wise runs good !!!
Yeah most the first link connects are pretty sketch due to the code trying to save the rateIndex to flash so it can connect quickly the next time. That compounds the "getting started" issues where the phase/interval of the packet period is still settling. It connects wicked fast for me on subsequent boots, before the VTX even seems to initialize haha. I pushed a change because I realized that the RF mode displayed in the OSD was the old V3 value and not our globally unified RF mode that we'd use if we had a real receiver connected (Reference RF Mode Indexes). The extra field in the table comes for free due to field alignment leaving a gap, and having it meant I could use it for the F500 detection, thus resulting in a smaller firmware overall! |
retested.. looks good all round just need some code review now |
|
Thank you so much for continuing to maintain SPI ELRS; it is a huge help to us! 🙌 |
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
|
It works fine. Upgraded to 2026.6.0 today and it works fine. Thanks!!! |
|
I flashed 2026.0 today and it works, bind and arm ok, movement on all 3-axis works as usual. Thanks a lot. |
* Working FHSS, SYNC packet and RC_DATA packet * Working downlink telemetry and linkstats * Working binding * Working data uplink and updated for fast resync * Document OtaUpdateCrcInitFromUid * Remove stubborn receiver reboot detection fast resync * Rejigger SX1280 to correct timing of telemetry, late FHSS * Missed unit test fixup * More test fixing * Wrong variables used * Minor differences in v3/v4 data ul/dl semantics, resolve unreilable ul * v4 compatible hop tables and sync channels * Revert "Rejigger SX1280 to correct timing of telemetry, late FHSS" This reverts commit b937ce3. * Fake telemetry nonceRx for F500, use standard tock slack * Team900 cycling through too many rates * Remove write-only variable * 8bit nonceValidator for V3 as well * Proper RFMD display for v4 * Use proper value to detect F500 instead of table index * My terrible whitespace Co-authored-by: Mark Haslinghuis <mark@numloq.nl> --------- Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
|
Thanks for working on this!! |
Backport of betaflight#14932, betaflight#15232, betaflight#15694 and betaflight#15204 to 2025.12-maintenance. Co-authored-by: Bryan Mayland <bmayland@capnbry.net> Co-authored-by: Mark Haslinghuis <mark@numloq.nl> Co-authored-by: Houston Sasseen <126936838+xhlsa@users.noreply.github.com> Co-authored-by: ChrisRosser <41840611+ChrisRosser@users.noreply.github.com>
Backport of #14932, #15232, #15694 and #15204 to 2025.12-maintenance. Co-authored-by: Bryan Mayland <bmayland@capnbry.net> Co-authored-by: Mark Haslinghuis <mark@numloq.nl> Co-authored-by: Houston Sasseen <126936838+xhlsa@users.noreply.github.com> Co-authored-by: ChrisRosser <41840611+ChrisRosser@users.noreply.github.com>
Updates the SPI ExpressLRS implementation to be compatible with ExpressLRS OTA version 4. The decision to accept this code or reject it and close it lies fully in the hands of the incredible Betaflight developers-- I will not harbor any resentment either way.
Issue #14919
PR STATUS: Ready to rock! 🤘
This ends ExpressLRS SPI support
I will never touch this code again after this PR is closed. On 2022 Jun 13 the first ExpressLRS SPI support released. Two days later, the official ExpressLRS message was to not buy these devices, and just 2 weeks later, SPI support was broken with the release of the first ExpressLRS 3.0 RC. Manufacturers believe if it is easier for them, they can just force the problems with this out-of-sync system onto the users, and more often than not the anger comes at the developers. It has been a problem for nearly 4 years and it needs to stop.
ExpressLRS OTA 3 Support
This code also supports ExpressLRS 3.x.x, by performing a cloud build with the ELRSV3 define (
USE_ELRSV3). It isn't truly the same as ELRS 3.x, but rather doing thing the ELRS 4.x way but with the 3.x OTA structures and validation. It just so happens that an ELRS 3.x transmitter is a lot looser with what it will accept so it works.That's right, I made even more of a support nightmare than SPI already is by adding one more variable to the system-- no longer will just the Betaflight version be enough to tell what version the user expects to connect with!
I'm not salty, you're salty!
Summary by CodeRabbit