crsf-wg / crsf

MIT License
114 stars 7 forks source link

CRSF compatibility strategy #5

Open mmosca opened 8 months ago

mmosca commented 8 months ago

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 commented 8 months ago

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

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 commented 8 months ago

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 commented 8 months ago

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 SYNCBYTE 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 SYNCBYTE 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.