Only do SW flow control for 3DR radio

[nicovanduijn]

Since the removal of the type field in #10395, the type of a radio was always assumed to be 3DR. Removal made sense, because the corresponding mavlink message does not contain a type field.

Consequently, when other components report a radio_status, the mavlink instance would be automatically using software flow control. This isn't always desired.

This fixes #14027

[dagar]

The idea of calling this a "3DR radio" can definitely go away, but I don't think this is quite right. As far as I can tell it's always been the receipt of RADIO_STATUS that made it a 3Dr radio. The only difference is where it was set (directly in mavlink receiver thread or not).

Let's see if we can drop the "3DR radio" type entirely and treat it generically. Can you verify what RADIO_STATUS looks like for this radio or configuration? https://mavlink.io/en/messages/common.html#RADIO_STATUS

Is txbuf set to something we can interpret as unused? What about any other fields we could use as identification?

[dagar]

As I mentioned in #14027, the flow control from RADIO_STATUS never seemed to work that well, although I suspect it could be made to work well if someone had time to analyze a few real world scenarios. With that in mind I wouldn't be opposed to adding a mavlink parameter to disable flow control via RADIO_STATUS if it's not possible to make this decision based on the contents of RADIO_STATUS.

This would be in addition to dropping LINK_TYPE_3DR_RADIO where we can simply call it generic, but store a bit indicating if RADIO_STATUS is available or not.

[nicovanduijn]

So the reason we stumbled on this in the first place is because our radio sent 0 in the txbuf field - which then led to continuous cycles of "starvation" -> "timeout" -> "reset" of the mavlink stream. Of course it can (and will) be fixed on the other side too. But I still think this part needs re-work.

We could define 255 to be "unknown" in txbuf and disable flow control - but note that the effect is the same as if we were to just send 50% - with fewer lines of code. (or 100% - since it's clamped eventually anyway)

I find the notions of "telemetry status" and "radio status" a bit confusing. I have to look at the code a bit more to complete my understanding.

Right now I'm thinking of getting rid of the radio_status message entirely and introducing the missing fields in telemetry_status instead. And since we're breaking the auto-detection of the 3DR radio, we might need more parameters such as MAV_${i}_TYPE and MAV_${i}_SW_FLOWCTL.

Should I go down that road or is it a dead end anyway?

Edit: I don't think getting rid of radio_status completely is the way to go. Also, getting rid of LINK_TYPE_3DR_RADIO isn't straightforward, since we are doing some radio configuration via AT commands from here. Wondering if that shouldn't just be in a a driver. But then again, mavlink owns the serial port, so I'm not sure what the proper thing to do is

[dagar]

I find the notions of "telemetry status" and "radio status" a bit confusing. I have to look at the code a bit more to complete my understanding.

It's a bit muddled now, but telemetry_status should probably be called mavlink_status, providing the status to the rest of the system of each mavlink instance. Some of those mavlink instances also have a radio_status, but often they don't.

[dagar]

Also, getting rid of LINK_TYPE_3DR_RADIO isn't straightforward, since we are doing some radio configuration via AT commands from here.

Yes, that's where this really falls apart. In the past I had some hacks started to expose more SiK radio configuration via mavlink parameters, but I didn't think we'd still be using them for this long. If we really wanted to figure out if it's a SiK radio or not we could try to check the version through the AT interface, but might not be worth it.

Anyway, how about a simple parameter to control the use of RADIO_STATUS or not?

MAV_${i}_SW_FLOWCTL sounds a bit too generic and possibly suggesting something else (https://en.wikipedia.org/wiki/Software_flow_control). Maybe something specific to Mavlink RADIO_STATUS?

Yet another option I'm wondering about is configuration that could force the type at start of the mavlink instance.

Multiple mediocre options...

[dagar]

Ok, here's what I'd propose for the least bad overall option, although at this point if you want to just move forward with the param to disable RADIO_STATUS flow control (perhaps with a different name) that's understandable and fine for now.

1. we divorce the notion that 3DR radio === RADIO_STATUS from mavlink entirely
2. RADIO_STATUS is valid for any link type
3. if possible, update your usage of RADIO_STATUS to set a valid TXBUF (or always 100%)
4. Mavlink::update_radio_status() add a special case to automatically disable usage if txbuf only ever reports 0 from the beginning
5. any remaining uses of LINK_TYPE_3DR_RADIO that actually matters I'll update to check the radio version via AT

[codecov]

Codecov Report

Merging #14032 into master will decrease coverage by 2.08%.
The diff coverage is 18.18%.

@@            Coverage Diff             @@
##           master   #14032      +/-   ##
==========================================
- Coverage   54.48%    52.4%   -2.09%     
==========================================
  Files         605      605              
  Lines       51256    51305      +49     
==========================================
- Hits        27929    26886    -1043     
- Misses      23327    24419    +1092

Flag	Coverage Δ
#mavsdk	?
#python	3.15% <0%> (-0.01%)	⬇️
#sitl_mission_FW	29.28% <18.18%> (+0.03%)	⬆️
#sitl_mission_MC_box	29.7% <18.18%> (-0.13%)	⬇️
#sitl_mission_MC_offboard_att	28.19% <18.18%> (-71.81%)	⬇️
#sitl_mission_MC_offboard_pos	28.39% <18.18%> (-0.18%)	⬇️
#sitl_mission_Rover	?
#sitl_mission_VTOL_standard	33.84% <18.18%> (+0.03%)	⬆️
#sitl_mission_VTOL_tailsitter	33.87% <18.18%> (+0.05%)	⬆️
#sitl_python_FW	29.36% <18.18%> (-70.64%)	⬇️
#sitl_python_MC_box	29.77% <18.18%> (-0.13%)	⬇️
#sitl_python_MC_offboard_att	28.27% <18.18%> (-0.17%)	⬇️
#sitl_python_MC_offboard_pos	28.46% <18.18%> (-71.54%)	⬇️
#sitl_python_Rover	74.68% <ø> (+48.29%)	⬆️
#sitl_python_VTOL_standard	33.91% <18.18%> (+0.03%)	⬆️
#sitl_python_VTOL_tailsitter	33.94% <18.18%> (+0.05%)	⬆️
#unittest	34.81% <18.18%> (+0.05%)	⬆️

Impacted Files	Coverage Δ
src/modules/mavlink/mavlink_main.h	52.43% <ø> (ø)	⬆️
src/modules/mavlink/mavlink_main.cpp	38.25% <18.18%> (-0.04%)	⬇️
...modules/rover_pos_control/RoverPositionControl.hpp	0% <0%> (-100%)	⬇️
src/modules/land_detector/RoverLandDetector.cpp	0% <0%> (-100%)	⬇️
...b/flight_tasks/tasks/Descend/FlightTaskDescend.cpp	0% <0%> (-83.34%)	⬇️
src/modules/navigator/takeoff.cpp	3.92% <0%> (-76.48%)	⬇️
...flight_tasks/tasks/Failsafe/FlightTaskFailsafe.cpp	0% <0%> (-73.92%)	⬇️
src/lib/pid/pid.cpp	0% <0%> (-72.59%)	⬇️
...modules/rover_pos_control/RoverPositionControl.cpp	0% <0%> (-63.87%)	⬇️
src/modules/land_detector/RoverLandDetector.h	0% <0%> (-50%)	⬇️
... and 65 more

[dagar]

It's probably time to replace all mention of "3DR radio" with "SiK radio". https://docs.px4.io/v1.9.0/en/telemetry/sik_radio.html

[nicovanduijn]

@dagar Rebased and default param value set to reflect the old behavior.

[dagar]

We need to re-evaluate the behavior on a SiK radio without hardware flow control (CTS/RTS pins) to either update that RADIO_STATUS throttling logic or perhaps even remove it. For now the safe thing to do is maintain the existing behavior, but now at least it can be optionally disabled for these newer systems.