Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRSF baud rate is incorrect. #12398

Closed
hydra opened this issue Feb 21, 2023 · 38 comments · Fixed by #13614
Closed

CRSF baud rate is incorrect. #12398

hydra opened this issue Feb 21, 2023 · 38 comments · Fixed by #13614
Labels
BUG Bugs are excluded from automatically being marked as stale

Comments

@hydra
Copy link
Contributor

hydra commented Feb 21, 2023

Describe the bug

The baudrate for CRSF is defined as 420,000baud. This is incorrect. Trappy from TBS confirmed to me today that CRSF uses 416,666 and always has. I've not verified this statement however.

When the wrong baud rates are uses (e.g. RX/FC mismatch) then FCs will get framing errors, data corruption, CRC mismatches and packet loss for RC channel data. (Verified by inspecting UART ISR register and looking at the FE flag).

Related information:

  • Old official TBS CRSF documentation (at least up to version 7) specified a 400,000baud ("400kbaud") which was corrected in revision 10 to be 416,666baud.
  • ELRS has a default baud rate of 420,000baud.

Code:
BF - https://github.com/betaflight/betaflight/blob/master/src/main/rx/crsf_protocol.h#L30
ELRS - https://github.com/ExpressLRS/ExpressLRS/blob/c5f67bd28a9f559a0bba3a4bcf509a4340445df5/src/lib/OPTIONS/options.cpp#L84

Rev10 CRSF docs:
image

Rev7 CRSF docs:
image

To Reproduce

Use mismatched baud rates on RX and FC.

Expected behavior

No data corruption or packet loss for users of both TBS and ELRS receivers.

  • Agreement with BF, ELRS and TBS on the correct baud rate to use.
  • Notification to users that it's changed, if/when it changes.
  • Added to the list of known-issues until corrected.

Support ID

N/A

Flight controller

N/A

Other components

No response

How are the different components wired up (including port information)

No response

Add any other context about the problem that you think might be relevant here

No response

@hydra hydra added the Template: Bug Set by auto_close_issue. label Feb 21, 2023
@hydra
Copy link
Contributor Author

hydra commented Feb 21, 2023

Note that users of ELRS using SPI-based RX will be unaffected by any baud rate changes as there's no CRSF UART involved.

@hydra
Copy link
Contributor Author

hydra commented Feb 21, 2023

After a bit more digging, CRSF V3 users may be less affected because the baud rate is auto-negotiated, and the Rx tells the FC what baud rate to use.

However, for CRSF V3, if there are 3 CRC errors in a row after baud-rate negotiation BF will drop-back to the pre-negotiated baud rate without the receiver knowing and the receiver somehow has to handle it, but from the little information I have (in the official specs) it's unclear how the receiver is supposed to handle a baud-rate-change on the FC, if at all.

It can also do this during flight, after arming, as I couldn't find any arming status check before dropping back to the pre-negotiated baud rate.
See also:

@klutvott123
Copy link
Member

Yes, it's true that the correct baud rate is 416666.
For CRSF V3, the receiver uses telemetry from the FC to detect a mismatch. If telemetry is disabled, the FC will send heartbeat frames instead, and if the telemetry wire isn't connected it wouldn't be able to negotiate the baud rate anyway. Both incomplete frames and crc errors are counted as errors.

@hydra
Copy link
Contributor Author

hydra commented Feb 21, 2023

Looks like Kiss V1 was also affected by this, at least according to this comment in the ELRS codebase (link below), perhaps @fedorcomander can confirm what baud rate new KISS firmware is using for CRSF these days?

https://github.com/ExpressLRS/ExpressLRS/blob/c5f67bd28a9f559a0bba3a4bcf509a4340445df5/src/user_defines.txt#L48-L50

@fedorcomander
Copy link
Contributor

fedorcomander commented Feb 21, 2023 via email

@fedorcomander
Copy link
Contributor

fedorcomander commented Feb 21, 2023 via email

@howels
Copy link
Contributor

howels commented Feb 22, 2023

Do you need to customise ELRS to use the 400K baud rate on RX or is that automatic too?

@hydra
Copy link
Contributor Author

hydra commented Feb 22, 2023

follow TBS protocol specs. With auto negotiation.

@fedorcomander Which version of the spec document did you follow? As the baud rate changed between Rev 7 and Rev 10, see OP above.

@fedorcomander
Copy link
Contributor

fedorcomander commented Feb 22, 2023 via email

@fedorcomander
Copy link
Contributor

fedorcomander commented Feb 22, 2023 via email

@SteveCEvans
Copy link
Member

@hydra please post links to the TBS CRSF specifications.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

@github-actions github-actions bot added the Inactive Automatically detected and labeled, will be closed after another week of inactivity. label Mar 25, 2023
@github-actions
Copy link

github-actions bot commented Apr 2, 2023

Issue closed automatically as inactive.

@github-actions github-actions bot closed this as completed Apr 2, 2023
@hydra
Copy link
Contributor Author

hydra commented Sep 22, 2023

@haslinghuis Please can you re-open this, official confirmation from TBS is here:

tbs-fpv/freedomtx#26 (comment)

Edit: and ideally pin this, until it's resolved somehow, rather than automatically closed by a bot.

@haslinghuis haslinghuis reopened this Sep 22, 2023
@KissUltra
Copy link

My doc from 14.08.2017 Rev 07 doc says 416kbaud :) Lol...

@haslinghuis haslinghuis added BUG Bugs are excluded from automatically being marked as stale and removed Inactive Automatically detected and labeled, will be closed after another week of inactivity. Template: Bug Set by auto_close_issue. labels Sep 22, 2023
@klutvott123
Copy link
Member

we should just change it

@hydra
Copy link
Contributor Author

hydra commented Sep 26, 2023

we should just change it

this is my feeling too, it's currently incorrect and should be corrected.

I'm not sure how happy other projects will be about it though, as they will have to communicate this to their users.

Suggest co-ordinating the change with the devs of at least the ELRS project.

@fedorcomander
Copy link
Contributor

fedorcomander commented Sep 26, 2023 via email

@hydra
Copy link
Contributor Author

hydra commented Sep 26, 2023

i plan to make it configurable on ultra. ;) just in case ;) may be you should do the same.

Not sure if that's a good idea, traditionally all RX protocols, that I can remember, have used a fixed baud rate associated with the protocol.

Every additional option only serves to complicate things, confuse the user and generate more support issues.

@KissUltra
Copy link

KissUltra commented Sep 26, 2023 via email

@ctzsnooze
Copy link
Member

Closing because the difference is small. ELRS deals running their baud rate report no errors. A project to refresh CRSF is underway. This specific issue can be laid to rest for now.

@hydra
Copy link
Contributor Author

hydra commented Dec 29, 2023

why not just correct it in BF if the difference is so small that the error difference will not be noticable? it's kinda silly to drift off-spec like this and currently just serves to confuse people.

@ledvinap ledvinap reopened this Dec 29, 2023
@ledvinap
Copy link
Contributor

@ctzsnooze : There may be other error sources (internal RC and possibly others). Wasting baudrate margin is not a good idea

@howels
Copy link
Contributor

howels commented Dec 29, 2023

@ctzsnooze : There may be other error sources (internal RC and possibly others). Wasting baudrate margin is not a good idea

The current situation provably works and eLRS and others have standardised on 420K as this was the observable implementation of CRSF at the RX. So please can we close this and move on? This is pedantry and not related to observable issues. 420K is an easy to use number already implemented without issue.

@faryna1993
Copy link

I have a request, add the following command.
set serialrx_baund 115200

@KissUltra
Copy link

eLRS and others have standardised on 420K

Unfortunately only @hydra makes sense here. Pedantry to keep to the standard instead of reinventing your own square wheel and call it OK :) That is not pedantry. Thats laziness and ignorance. God luck. :)

@faryna1993
Copy link

It seems that this is too difficult a task for you. Therefore, it is easy to attribute it to pedantry. Good luck. :)

@KissUltra
Copy link

i can say exactly the same about your task. ;)

@hydra
Copy link
Contributor Author

hydra commented May 3, 2024

The current situation provably works and eLRS and others have standardised on 420K as this was the observable implementation of CRSF at the RX. So please can we close this and move on? This is pedantry and not related to observable issues. 420K is an easy to use number already implemented without issue.

In my recent testing here, as the hardware designer of at least 4 UART based ELRS RX's now, using both the ESP32 Pico D4 and the ESP32S3 I can 100% say that you DO notice observable errors and corruption of the data when using the wrong baud rate.

@haslinghuis
Copy link
Member

@hydra did you try #13614

@hydra
Copy link
Contributor Author

hydra commented May 3, 2024

@hydra did you try #13614

no, but happy to see the PR!

@KissUltra
Copy link

Thank you @hydra and @haslinghuis

@SunjunKim
Copy link

The current situation provably works and eLRS and others have standardised on 420K as this was the observable implementation of CRSF at the RX. So please can we close this and move on? This is pedantry and not related to observable issues. 420K is an easy to use number already implemented without issue.

In my recent testing here, as the hardware designer of at least 4 UART based ELRS RX's now, using both the ESP32 Pico D4 and the ESP32S3 I can 100% say that you DO notice observable errors and corruption of the data when using the wrong baud rate.

Do you have any data showing how serious the problem is?

@fedorcomander
Copy link
Contributor

fedorcomander commented May 8, 2024 via email

@CapnBry
Copy link
Contributor

CapnBry commented May 8, 2024

i have arc error counter on the osd

What is the "arc error counter" that can be placed on the OSD?

EDIT: Or wait, are you referring to while running at 400k baud? We need to separate out "getting errors at 400k baud" vs "getting errors at 420k baud".

@KissUltra
Copy link

KissUltra commented May 8, 2024 via email

@SunjunKim
Copy link

SunjunKim commented May 9, 2024

crc---Cheers,AlexOn 8 May 2024, at 16:42, Bryan Mayland @.> wrote: i have arc error counter on the osd What is the "arc error counter" that can be placed on the OSD? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.>

Sorry this is not much informative.. the conversation is like it’s A. -> what is it? -> it’s A.

Providing your gear setup comprehensively will be better helpful. Also please explain what that “arc error counter” means and how it’s measured will be even better.

Ah you mean CRC!!! sorry that part was hard to spot.

@hydra
Copy link
Contributor Author

hydra commented May 14, 2024

Just commenting that #13614 does not fix/complete this issue and that BF is not following the CRSF specification outlined in the OP here: #12398 (comment)

Users wanting a build of BF that follows the CRSF spec have to add #define USE_CRSF_OFFICIAL_SPEC as it is not, currently, the default.

Additionally the authors (@tbs-trappy) of CRSF and the spec commented here https://github.com/betaflight/betaflight/pull/13614/files#r1596251113

| tl;dr: suggest to fix problem at the source. 416.6 is correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Bugs are excluded from automatically being marked as stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.