Skip to content
New issue

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

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

Already on GitHub? # to your account

CRSF compatibility strategy #5

Open
mmosca opened this issue Dec 20, 2023 · 4 comments
Open

CRSF compatibility strategy #5

mmosca opened this issue Dec 20, 2023 · 4 comments

Comments

@mmosca
Copy link

mmosca commented Dec 20, 2023

While we are starting from the current state of the reverse engineered CRSF behavior across elrs, edgetx and the major fc software, TBS still owns the actual spec for CRSF and may make changes in the future that could conflict with this implementation.

My suggestion is to actively break backwards compatibility by changing the sync byte to give the project a cleaner slate and less concerns when new or previously unknown, crsf functionality comes up.

All involved open source parts in crsf processing already support multiple wire protocols. Adding a new one, with a new name per #2 should be trivial, specially if the initial changes are only the sync byte.

Yes, this would initially create some code duplication, but this would also prevent future checks for what flavor of crsf is being used and allow us to target pain points of CRSF, like the auto baud configuration being "backwards" from what we expect (controlled form the rx side, instead of downstream side).

@CapnBry
Copy link
Contributor

CapnBry commented Dec 27, 2023

I believe CRSF already has what you're asking for and all the rogue implementations we use are the flawed part. When writing the specs docs for the wiki, I went through the source to CRServoF, Betaflight, iNav, ExpressLRS, OpenTX, and EdgeTX all support proper CRSF (which start with a fixed SYNC_BYTE 0xc8).

Exceptions

  • EdgeTX's CHANNELS_PACKED is erroneously (I believe) sent with the wrong start byte (0xEE instead of 0xC8). ExpressLRS supports receiving this packet with the correct start byte, but I am waiting to hear back if TBS does so that this can be fixed to standard.
  • ExpressLRS's output has the wrong start byte in a few packets, but it is processed properly by everything because implementations tend to ignore the start byte (iNav / Betaflight, CRServoF) or have a loose restriction on the start byte (EdgeTX). ExpressLRS's implementation will shortly change to output the proper start byte for all packets.

This is why step 1 of CRSF-wg is to document the correct protocol. It already has what you're requesting we fork the protocol to add. 😆

To your point about eventually needing different implementations when the protocol becomes incompatible with CRSF, isn't that premature and not a function of this project? Each implementation would need to decide if the changes made warrant a whole new implementation. I would prefer to keep CRSF as backward compatible as possible so that everything that supports CRSF still supports CRSF without any changes needed. The baud rate negotiation is obviously a major potentially breaking change, but wouldn't it be a good idea to first know if it is? On account of no formal proposal for changing it has been published?

@mmosca
Copy link
Author

mmosca commented Dec 27, 2023

My main point on raising this issues it to clarify what the strategy should be, and that does not need to break from the current implementations. As you mentioned, step 1 is just formalizing what is out there, but without TBS buy in, any new message types we add in the future have the potential of conflicting with something coming from TBS.

My suggestion of changing the start byte was to intentionally break the compatibility with standard crsf to avoid future disagreements on what a given packet type should represent, caused by TBS adding features with conflicting frame types.

A possible compromise on keeping backward compatibility while giving the project room to innovate, would be to have different package sync bytes only for crsf-wg packages realated to extensions/new features.

@olliw42
Copy link

olliw42 commented Dec 30, 2023

When writing the specs docs for the wiki, I went through the source to CRServoF, Betaflight, iNav, ExpressLRS, OpenTX, and EdgeTX all support proper CRSF (which start with a fixed SYNC_BYTE 0xc8).

It is great that you guys are trying to continue with reverse engeneering and documenting the protocol, but - I'm very sorry - I really have to intervene here and tell you guys a bit about logical interference and not creating one's own dellusion ballon.

If I do produce some nonsense and then a million copies of that nonsense are created, does that nonsense then become true or fact?
I'm sure you all agree that it's then still nonsense.

But, heck, you should then here too!
The point is that these sources are not authorative and all are highly self-referencing and self-replicating. That is, none of these sources can claim for certainty to know what's proper CRSF, and these projects have a very strong tendency to copy forward.
How can you possibly claim "proper CRSF ... start with a fixed SYNC_BYTE 0xc8" or "All serial CRSF packets begin with the CRSF SYNC byte 0xC8, with ... exception" ???

The question arrises even more so as there is counter evidence. COUNTER EVIDENCE! I for sure don't know who TBS has cooperated with, but I believe to know for a fact that TBS and ArduPilot have cooperated closely. Surprise, surprise ... there is no such a thing like a SYNC_BYTE in their code ... go figure yourself ... surprise, suprise, EdgeTX's start byte which is believed to be erroneous suddenly makes sense ...
uuuuppppssss

Worth to be mentioned, this appears to not be the first and only case of a very likely totally wrong conclusion. It's for the same reasons that ELRS thought that the baudrate should be 420000.

To sum up, what I'm saying is that you guys should not fool yourself into wrong conclusions by not properly drawing your conclusions based on real facts and evidence. What you want to do here, do it seriously - or just don't.

Does it mean that I would know the truth? Hell, no, sure not. Does it mean that there might be not such a thing like a sync byte? Hell, no, sure not. I don't know. All it means is that as of today claiming "proper CRSF ... start with a fixed SYNC_BYTE 0xc8" or "All serial CRSF packets begin with the CRSF SYNC byte 0xC8, with ... exception" is exactly this, a claim, for which counter evidence appears to exist.

@OpenUAS
Copy link

OpenUAS commented Nov 21, 2024

@olliw42 Thanks for your write-up, however since https://github.com/tbs-fpv/tbs-crsf-spec/blob/main/crsf.md#frame-details maybe time to close this issue?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants