ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
10.87k stars 17.33k forks source link

CRSF broken #26302

Closed olliw42 closed 7 months ago

olliw42 commented 7 months ago

EDIT: wasn't a bug in ArduPilot but elsewhere

Bug report

I've updated yesterday my fork of master from a version of 25th Jan, and now, when using a mLRS rc telemetry system with the receiver configured to output CRSF, the rc signal is not detected. When the receiver is configured output to SBus it works. For versions before 25th Jan it all was always working fine since two years or so.

From the fact that sbus works, it can be concluded that the issue is not due to the recent detect_async_protocol() addition, but likely due to the recent CRSF changes.

I have no idea what the issue is, couldn't yet analyse and the code is also a bit not so easy to follow, but as a wild guess it might have to do with that the mLRS rc telemetry system outputs the rc frames at a relatively low rate (the report is for 31 Hz, couldn't yet tested other rates). I could imagine that the recent changes to make "high speed" CRSF better work may have resulted in breaking "low speed" CRSF.

Version master 26d4e0d285141130469c306f22a38e32400ec9d8

Platform [ ] All [ ] AntennaTracker [x] Copter [x] Plane [ ] Rover [ ] Submarine

Airframe type

Hardware type mateh H743

Logs

andyp1per commented 7 months ago

Hey @olliw42 the recent changes actually make the protocol more tolerant of larger frame gaps, so I think the speed is unlikely to be the problem. Do you use the same frame marker in mLRS (0xC8) as CRSF?

andyp1per commented 7 months ago

Also the change only went in yesterday, so maybe its something else if 25th Jan?

olliw42 commented 7 months ago

Hey @olliw42 the recent changes actually make the protocol more tolerant of larger frame gaps, so I think the speed is unlikely to be the problem. Do you use the same frame marker in mLRS (0xC8) as CRSF?

many thx for your response. You save me a ton of trying to trace the issue down. mLRS is indeed not using 0xC8. Haven't noted this change in behavior of AP. (https://github.com/olliw42/mLRS/blob/main/mLRS/CommonRx/out.cpp#L288)

The AP code before accepted both 0xC8 and broadcast. For my curiosity, why was it necessary or what was the reason for this change of behavior?

this very likely explains it. Will close when I have explicitely tested.

andyp1per commented 7 months ago

@olliw42 the reason for the change was two-fold

I could obviously add broadcast as another frame marker, but that's a little bit problematic as there are always a lot of zeros in frames. Is it possible for mLRS to use 0xC8? In my testing this is pretty much exclusively used by TBS kit for use cases that matter to us

olliw42 commented 7 months ago

I sure will change mLRS to 0xC8 it was actually just a random choice, I saw that AP supported both and saw some other repos being random on it, so a throw of dice ... bad luck

THX for the explanaitions

andyp1per commented 7 months ago

Thanks @olliw42

olliw42 commented 7 months ago

tested it now on two vehicles and also flew one both sbus and crsf work perfectly fine

it's exactly as you said. The change was in https://github.com/ArduPilot/ardupilot/commit/d5ba0b6302050dee76f7d15c71f358c7f7b7fc66, which I missed to realize. THX again for your hint!

all good, entirely my fault, so closing :)