Add MAVLink receiver support, RADIO_RC_CHANNELS, no III

[olliw42]

This PR substitutes #25838, and also #24577

It does three things

• updates to the latest design of the RADIO_RCCHANNELS message, meregd here development.xml: RADIO_RC_CHANNELS message mavlink#343
• accounts for the introduction of the function detect_async_protocol(protocol()
• add RC_MAVLINK_RADIO option to the build options (I believe such a comment was droped in a half sentence at the dev call)

The "original" code is in my betapilot fork, which I have test flown on two vehicles (a copter and a plane). The exact code in this PR here I have tested on a bench test setup.

[olliw42]

ups, seems I have screwed up the spelling of the file names ... strange it compiles fine for me. Will correct this evening.

[peterbarker]

Looking good.

I think the filenames need to be AP_RCProtocol_MAVLinkRadio.h etc, too.

[olliw42]

@peterbarker I think I incorporated all suggestions. I added also few more #if protections which were needed to compile on 1024 size boards,

yet there are some checks failing, related to the inclusion of mavlink. I have unfortunately no idea what's up with them. Any suggestion would be welcome.
PS: the issue seems to be that the step "Processing modules/mavlink/message_definitions/v1.0/all.xml" is not done for the failing boards
ok, had to take into account HAL_GCS_ENABLED

still one check failing

[peterbarker]

still one check failing

That Sub test is flapping, not your fault :-)

[olliw42]

still one check failing

That Sub test is flapping, not your fault :-)

great, many thx for the response, I hear it with some relieve (given the other earlier check fails did had been my fault :))

any other thing required from my side for merging?

[olliw42]

@peterbarker @tridge
while testing out @IamPete1's suggestion I found that the current code using detect_async_protocol() implies a change in behavior as compared to what I had originally suggested, which I'm not sure is what is wanted:

as is, the RC_PROTOCOLS bit mask is not respected for neither DroneCAN nor MAVLinkRadio protocol. That is, if in that bit mask one would set to allow only CRSF, the MAVLinkRadio would be choosen nevertheless.

For DroneCAN this appears to have been the intention, at least it is how it was before, and in the bit mask documentation in RC_Channels_VarInfo.h there indeed is no bit for DroneCAN. My original intention however was that MAVLinkRadio can be masked out from detection.

I do see 3 courses of action now:

1. it's good as it is, both DroneCAN and MAVRadio shall be choosen whatever RC_PROTOCOLS (implying no bits for both in RC_Channels_VarInfo.h)
2. DroneCAN should be handled as currently but MAVRadio should be maskable by RC_PROTOCOLS (implying no bit for DroneCAN but a bit for MAVRadio in RC_Channels_VarInfo.h)
3. both DroneCAN and MAVRadio should be maskable by RC_PROTOCOLS (implying bits for both in RC_Channels_VarInfo.h)

What is your choice?
Personally I would go with 2 or 3, preferably 3, but I sure follow your suggestion.

[peterbarker]

as is, the RC_PROTOCOLS bit mask is not respected for neither DroneCAN nor MAVLinkRadio protocol. That is, if in that bit mask one would set to allow only CRSF, the MAVLinkRadio would be choosen nevertheless.

That would be a bug, then :-(

For DroneCAN this appears to have been the intention, at least it is how it was before, and in the bit mask documentation in RC_Channels_VarInfo.h there indeed is no bit for DroneCAN. My original intention however was that MAVLinkRadio can be masked out from detection.

That was not my intention. There does seem to be a DroneCAN bit in the documentation for RC_PROTOCOLS`:

I do see 3 courses of action now:

3. both DroneCAN and MAVRadio should be maskable by RC_PROTOCOLS (implying bits for both in RC_Channels_VarInfo.h)

What is your choice? Personally I would go with 2 or 3, preferably 3, but I sure follow your suggestion.

Definitely this one. Do you think #26411 would be sufficient to fix the problem? We need something easily back-portable. I'll be testing that later today.

[olliw42]

THX. So option 3. 👍

Do you think #26411 would be sufficient to fix the problem?

yes. Essentially exactly what I tried. I however would (and am going to) move that part into detect_async_protocol(). Makes it also somewhat more similar to the proecess bytes and pulse functions.

I will do this as part of this PR here, i.e., #26411 should be changed accordingly.

[olliw42]

@peterbarker next round to have a look at it :)

[peterbarker]

Do you think #26411 would be sufficient to fix the problem?

yes. Essentially exactly what I tried. I however would (and am going to) move that part into detect_async_protocol(). Makes it also somewhat more similar to the proecess bytes and pulse functions.

That's

I will do this as part of this PR here, i.e., #26411 should be changed accordingly.

I've fixed that PR up and marked you as co-author on it. #26411 is a good one to merge because it will be an easy backport.

[olliw42]

ok, so I removed that part from detect_async_protocol(). So you could first merge this one here and then #26411. Hope I got your plan :)

[peterbarker]

ok, so I removed that part from detect_async_protocol(). So you could first merge this one here and then #26411. Hope I got your plan :)

Tested that PR.

Turns out we were also missing a call to populate the rc_protocols_mask member variable, so I've added that. I think we might be going a little overboard with our hestitance to populate that variable every time update is called. It does save work in the general case that you've got an RC receiver you're happy with, but now we have three calls to populate it....

I've marked that other PR for MergeOnCIPass - you might want to rebase on top of it?

Please let me know once you've retested and I can merge this (tridge is happy).

[olliw42]

ups ... in the older PRs (#25838) I had that ...

I've marked that other PR for MergeOnCIPass - you might want to rebase on top of it?
Please let me know once you've retested and I can merge this (tridge is happy).

sounds like a plan :) THX!!

[olliw42]

@peterbarker
rebased and tested on my bench setup, and my copter and plane (not flight, only on ground)
=> looks all good from my side and ready to merge

given that I have not spotted in my testing routine effects of the missing rc_protocols_mask = rc().enabled_protocols(); I'm not sure however what my testing is worth it. Test routine on the test bench setup:

• with only MAVRadio unmasked in RC_OPTIONS: power up fc, connect to MP to see channels, power up mLRS: see if channels work
• change mask to unmask all except CRSF: see if channels stop
• change hw setup, wire CRSF from receiver and fc, power up fc, connect to MP, power up mLRS: see if channels work
• change mask to unmask all except MAVRadio: see if channels stop
• while still runing change hw, remove CRSF line, keep MAVLink line, change mask to unmask all except MAVRadio: see if channels come alive
• power up fc, connect to MP to see channels, power up mLRS: see if channels work

On the copter and plane I just flashed ArduPilot and checked if the channels still work.
These tests all were passed.

[olliw42]

@peterbarker
maybe my last post was confusing, so to be unambigious:

Please let me know once you've retested and I can merge this (tridge is happy).

tested and IMHO you can merge :)

also just did another rebase

[peterbarker]

LGTM

[peterbarker]

Merged, thanks!

[olliw42]

Yiiiippppeeeehhhhhhh

[Hwurzburg]

@olliw42 , @peterbarker was looking at what wiki is needed(if any) for this and saw that there is no extract_features.py update to accompany this...can one of you do this so some user can know if the feature is included or not in the firmware for a board type?

[olliw42]

birefly looked at it
can't claim to really understand it but to me it seems this is also so for some other rc protocols, like DRONE_CAN, UDP, etc.
I can give it a try for for MAVLINK_RADIO

[Hwurzburg]

any entry in build_options.py should have a corresponding entry in extract_features.py....if you find any that dont, create an issue...thanks

[olliw42]

ah, ok, not all rc protocol options are in build_options.py. thx.