DroneCAN: Add support for few more statistics to DroneCAN RC

[olliw42]

This PR adds support for few more link statistics to the DroneCAN RC.

Specifically, it adds the support of three status flags which are propsoed to be added to the DroneCAN RCInput message (dronecan/DSDL#56), which allows to also send LQ, RSSI_DBM, and SNR, in addition to RSSI, via that message. These data are then made available to the OSD.

This PR is in principle dependent on dronecan/DSDL#56, but it doesn't need it, i.e. compiles and works fine without this being merged, since the code uses bit fields, and none of the flags defined in dronecan/DSDL#56.

I could test this only very lightly myself on my bench setup, but @jlpoltrack did test this carefully on a matekh743 with plane firmware. He found the CRSF to still work and also the new DroneCAN support to work.

The PR has a slight change in behavior as compared to before, namely that then no STATUS flags is set it now reports -1 to the add_input() rssi field, whereas before it was reporting 0. (old behavior: https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_RCProtocol/AP_RCProtocol_DroneCAN.cpp#L97).

The OSD was using so far only the crsf() singleton, so it needed now some abstraction so either the DroneCAN or the crsf class is used by the OSD, dependent on which RC protocol is configured by the user. I found that all a bit strange code, and as result of a struct being used the abstraction doesn't look like it is for other frontedn/backend examples. Bit it's aligned with how the crsf() singleton works.

As regards the receiver side, an example implementation of the proposed support is in the main branch of mLRS: https://github.com/olliw42/mLRS/blob/main/mLRS/CommonRx/dronecan_interface_rx.h#L339-L394

[jlpoltrack]

Related Issue: #28295

Tested with both CRSF and DroneCAN RC - OSD elements behaved as expected when using RC_PROTOCOLS bitmask to switch between the two protocols.

[peterbarker]

Sorry, we must not have communicated our thoughts well on how we'd like this shaped.

We wanted three bits set aside which would specify a value out of an enumeration, not three bits, one for each of the types (and re-use of valid as RSSI, it seems?)

[peterbarker]

Suggested change

	volatile struct RcLinkStatus _rc_link_status;
	volatile struct RCLinkStatus _rc_link_status;

Why volatile?

[olliw42]

this I didn't understand too ... so I was just closily following what is done for the CRSF class
https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_RCProtocol/AP_RCProtocol_CRSF.h#L310-L314
https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_RCProtocol/AP_RCProtocol_CRSF.h#L361
assuming that someone had a good reason which is above my paygrade

pl say clear "yes volatile" or "no voloatile" and I will do

[olliw42]

has the "volatile" thing been sorted out? It should stay?

[olliw42]

We wanted three bits set aside which would specify a value out of an enumeration, not three bits, one for each of the types (and re-use of valid as RSSI, it seems?)

so as @tpwrules said in dronecan/DSDL#56 :)
I'll do

Question: Would it be acceptable to send two data types in one transmission, i.e. to bitmask the quality field?

Specifically: The LQ data only needs 7 bits of the unit8 quality, so the 8th bit could be used e.g. for reproting the active antenna. Since the data has to be send round robin, this would be highly benefitial, i.e., adding an extra type just for that one bit would be sad as it would need an extra slot, The type could be named TYPE_LQ_ANT.

[olliw42]

made the changes as suggested - I hope at least.

also added the option for tx_power (mLRS doesn't use it but orther systems may)

changes were tested by @jlpoltrack and found to work for both normal CRSF and DroneCAN rc.

I'd like to reiterate the question if it would be acceptable to pack both LQ and active_antenna into the quality field

[peterbarker]

Please do add the active antenna bit.

We need to know what older versions of ArduPilot (eg. 4.5) will shows when a RC receiver sending this new style of packet is used.

[olliw42]

Please do add the active antenna bit.

great, will do.

We need to know what older versions of ArduPilot (eg. 4.5) will shows when a RC receiver sending this new style of packet is used.

in the OSD it will show nothing since there is no code path for how the data received by DroneCAN can go to the OSD. The OSD uses the crsf() singleton. E.g. https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_RCProtocol/AP_RCProtocol_DroneCAN.cpp#L97-L98, and similarly for the other ext stats fields.

as regards AP_RCpororocol_DroneCAN it will set rssi to 0 if the QUALITY bit is not set: https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_RCProtocol/AP_RCProtocol_DroneCAN.cpp#L97-L98. So, if in the new message other bits are set rssi will be set to zero - like before.

IMHO I did consider backwards compatibilioty properly when I made that suggestion

1. new message + old AP -> should work just fine (see what just was said)
2. old message + new AP -> should work just fine (the code will handle QUALITY or !QUALITY as before)
3. old message + old AP -> should work just fine (obvious LOL)
4. new message + new AP -> should work just fine (will be even so when the code in the DroneCAN node is not changed, but just recompiled with new message in DSDL)

[olliw42]

active_antenna bit added

also noted that tx_power is only uint8_t so made it to be in units of 5 mW, to cover the range of 0 ... 1000 mW

as before, tested by @jlpoltrack and found to work fine, both with CRSF and with DroneCAN rcinput

[timtuxworth]

active_antenna bit added

This only allows for 2 antennas. Note that the RadioMaster DBR4 has 4 antennas! Just an example - but assuming 2 antennas will be a problem soon, if not right away. (You always need more).

https://www.radiomasterrc.com/products/dbr4-dual-band-xross-gemini-expresslrs-receiver

[olliw42]

it needs to be understood that the mechanism used here isn't and can't be a full fledged link statistics
if that is wanted a link stats message needs to come

if 4 antenna is really intended one could add an EXTRA_ANTENNA flag (in addition, in case it is needed)

[olliw42]

We need to know what older versions of ArduPilot (eg. 4.5) will shows when a RC receiver sending this new style of packet is used.

while the explanation given in the above based on the code might be compelling enough, @jlpoltrack has explicitely tested this with result "AP 4.5.7 + mLRS Latest w/ if(1) works fine too. RC Works and OSD Elements don't show (as expected)"