Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

development.xml: RADIO_LINK_STATS message (revised) #367

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olliw42
Copy link

@olliw42 olliw42 commented Aug 15, 2024

follow up to #354

this here is my proposal for a RADIO_LINK_STATS message, to be used by MAVLink RC receivers to indicate statistics to e.g. a flight controller. It is somewhat revised as compared to what has been presented for discussion in #354

As outlined elsewhere, the link statistics should be distinguished into "fast changing" and "slowly changing" metrics. The "fast changin" metrics is the content of the RADIO_LINK_STATS message proposed here. The "slowly changing" metrics is suggested to be put into a RADIO_LINK_INFORMATION message, which has yet to be defined.

The discussion in #354 on the layout of RADIO_LINK_STATS can be described as pretty diverse, and hardly standardizable. The proposal here is driven by two contradicting observations:

  • In principle such a message is not needed since ArduPilot's lua scripting and MAVLink's existing message and dialect system would allow writing a lua script which could do exactly what each link systems finds is the best thing to do. Maybe few little and simple additions to the lua scripting would have to be done, which doesn't invalidate the statement however.
  • On the other hand, the existing MAVLink messages for conveying link statics are obviously outdated, and thus an improved standard message which is more adapted to today's link systems appears highly needed.

The proposal tries to strike a balance between providing some standard yet widely desireable metrics and keeping the more specialized metrics to the scripting.

In comparion to the previous proposal in #354 two main changes were made:

  • the info on the usage of the antennas is condensed into one flag, instead of individual fields. This reduces the byte count, but also makes them less umbigious, and moreover is easy to extend.
  • two freq float fields are added as extensions, to put them at the end of the message and thus subject to zero-byte-truncation. That is, when not used, they don't come at a byte count cost. The frequency fields can be extremely useful to investigate the main interferences in a certain area. It follows a suggestion in development.xml: RADIO_LINK statistics messages #354

The message as proposed is easily extended to e.g. provide the stats for more antenna, if there is need for. This can be relevant for more advanced systems, such as e.g. FrSky's "3 antenna" devices or the 4 antenna situation mentioned in #354.

This effort for this proposal is the response to the recent trigger by @hamishwillee in #354. Let's see how it goes :)

@olliw42
Copy link
Author

olliw42 commented Aug 31, 2024

now that I've made it would be nice if it could get some review :)
@hamishwillee @auturgy @peterbarker

@hamishwillee
Copy link

hamishwillee commented Sep 4, 2024

now that I've made it would be nice if it could get some review :)

@olliw42 Thanks very much, a very reasonable request. I'm deferring first round on that to @auturgy because he is opinionated, and I am ignorant.

When/if I do comment, it will be to work out if the documentation is sufficient to implement/consume this stuff without access to all the discussion in the many threads that have lead to it - i.e. answering the question Can I as an [implementer | consumer] work out what needs to be done to [populate | consume] this field, based on the description, invalid values, minValue, maxValue.

You're usually good at that, so I am sure it won't be far off. If you'd rather plan to include that in a "Radio protocol document" then that's fine too.

@olliw42
Copy link
Author

olliw42 commented Sep 4, 2024

@hamishwillee MANY thx for your response. I'm happy enough to see it gets some attention. So I'll wait for @auturgy to share his opinionated opinion. :) Looking forward to hear your suggestions for improving the descriptions.

I just saw that ArduPilot plans to start beta testing of v4.6-beta1 in few weeks, and I'd really love if this would get into 4.6 ... also because since MatekSys's mLRS devices entering the market we (mLRS) do have now already cases where one just would love to get the receiver side links stats for diagnosis ...

@olliw42
Copy link
Author

olliw42 commented Sep 10, 2024

could this be added to the devcall for consideration ?

@auturgy
Copy link

auturgy commented Sep 11, 2024

it would be good to get this in: note it is a PR against development.xml so iteration can continue until it is pulled across into common (upstream common preferably) or ardupilotmega.

@tridge
Copy link
Collaborator

tridge commented Sep 11, 2024

who is the consumer of this message? is this aiming to go into the ArduPilot onboard log? or for display on the GCS? or used for flow control like RADIO_STATUS?

Copy link

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have concerns about the amount of data (and potential system load!) for consuming this message at your typical 50Hz RC input rate.

What's this data intended for? Simply logging?

@@ -96,5 +128,33 @@
Channel values are in centered 13 bit format. Range is -4096 to 4096, center is 0. Conversion to PWM is x * 5/32 + 1500.
Channels with indexes equal or above count should be set to 0, to benefit from MAVLink's trailing-zero trimming.</field>
</message>
<message id="421" name="RADIO_LINK_STATS">
<description>Radio link statistics for a MAVLink RC receiver or transmitter. Tx: ground-side device, Rx: vehicle-side device.
The message is normally emitted in regular time intervals upon each actual or expected receiption of an over-the-air data packet on the link.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The message is normally emitted in regular time intervals upon each actual or expected receiption of an over-the-air data packet on the link.
The message is normally emitted in regular time intervals upon each actual or expected reception of an over-the-air data packet on the link.

similarly elsewhere

@olliw42
Copy link
Author

olliw42 commented Sep 11, 2024

many thx for looking at it

who is the consumer of this message? is this aiming to go into the ArduPilot onboard log? or for display on the GCS? or used for flow control like RADIO_STATUS?

What's this data intended for? Simply logging?

the intended consumer is the autopilot, and a primary purpose is IMHO indeed for onboard logging in ArduPilot. In addition, I believe that someone very quickly will also want to have (some of) the data on the OSD. These would be the two main usages I would see at this point.

it is EXPLICETLY NOT for flow control purposes like parts of RADIO_STATUS are. I had outlined the concept in a longer text in the mavlink repo, but to spare you having to read it: A key idea of the "new" set of messages is to logically disentangle the contents. That means, eventually there should be also a new message for the purposes of flow control. This message here is NOT for that purpose.

Since that new flow control message isn't here yet and may not come soon the implication is that currently RADIO_STATUS needs to be also send by a receiver, It is sufficiently small so this isn't an issue.

If eventually that message should also be send from the tx (ground) side to a GCS, like the RADIO_STATUS is send from the vehicle side to the autopilot and on the ground side to the GCS, might be an extension which can be discussed in future, but this side of things, what to do on the ground side, IMHO would clearly need more discussion and thought, and in my view is explicitely not a purpose.

We have concerns about the amount of data (and potential system load!) for consuming this message at your typical 50Hz RC input rate.

That's indeed a valid important point to discuss.

First off, I think that's a general issue with using MAVLink for rc receiver purposes. The major issue however DOES COME from the RC data message RADIO_RC_CHANNELS itself, as it is the largest message of em all (16 channels make already 32 bytes payload!). The suggested RADIO_LINK_STATS message is comparatively small to it. The load for digesting this additional mavlink message is not really dramatic, in my judgement.

The bottom line is certainly that all this is NOT for high rate rc systems. In mLRS e.g. the output of these messages is limited to 50 Hz even if a faster mode is used. A 1000 Hz mode like in ELRS must certainly NOT translate in an emission rate of RADIO_RC_CHANNELS (and RADIO_LINK_STATS) of 1000 Hz ... so, a limit of 50 Hz max seems reasonable to me.

Another way to look at it is in terms of the baudrate of the serial connection between rc receiver and autopilot. For the typical use case 57600 is way sufficient (the only reason to go to higher baudrates is to reduce latency, but it's not for reasons of the required data rate). With that low baudrate being sufficient, it appears implausible that it should be this what kills the cpu, in comparison to the data rate produced by other MAVLink components or other serial connections or serial transports.

So, if one can accept 50 Hz as recommended upper limit then I don't see really a concern as regards the load for the pure MAVLink handling. That's of course not a "hard scientific fact", but my common sense evaluation.

As a side note, that's also one important reason for logically disentangling the content into different messages. A flow control message for instance does typically NOT have to be send at 50 Hz but at much low rates.

The more tricky part is here IMHO related to the use of that data, the on board logging. It's indeed a bunch of data, which e.g. required me to split it into two datalogging fields (it seems each can carry only 7 data fields or so). I really don't have the competence to judge and determine if that produces too much load, all I can say with confidence is that on H7 autopilots I could not see issues ( I use this for at least 1/2 year).

What I also just can tell is that the data appears to be extremely valuable. The discussion on range and performance of links or in case of issues usually very quick moves towards looking at logs. The only really reasonable log possible today (for folks like me) is a log of the CRSF elements on the EdgeTx/OpenTx radio, which (1) only shows the transmitter/ground side of things, (2) ionly shows what the transmitter receives, and (3) doesn't carry some data, which in the discussions are then darely missed. I think that especially the analysis in terms of also the frequency will become pretty routine for certain cases (once tools for analysis logs make that easier than today).

If the logging part of the data is a system load issue, maybe it can be alleviated by a means to enable/disable it.

@hamishwillee
Copy link

My 2 bits

  • if this is for logging it probably isn't needed all the time. Could this be configured to be low rate by default and something where GCS/Autopilot could set the rate if it is interested?
  • The message needs for logging vs need for an OSD are likely to be quite different. That also implies perhaps rates should be configurable, and we should consider what the OSD needs as well and see if that should be different.

I'm not saying any of this because the message is big: it is not. But it is desirable not to keep adding things that are sent by default even when consumers aren't all that interested. A model where the systems on the network request the minimum messages they need is probably more scalable.

@olliw42
Copy link
Author

olliw42 commented Sep 12, 2024

if this is for logging it probably isn't needed all the time.

I think if one wants that data to be logged, then one wants it all, since otherwise any analysis always will have to make assumptions. The alternative would be to also record the min and max values in the time interval, as it was proposed in the other thread, which bloats up the size of the message ...

having said this, any rx mavlink receiver system can choose to send at lower rate ... but I would not engrain that as requirement

The message needs for logging vs need for an OSD are likely to be quite different.

I myself can't see that. As I have outline in the above, I blieve ANY special needs can be well handled by ArduPilots lua system and MAVLink's available opaque messages. So this message should be about a minimal consens so to say.

But it is desirable not to keep adding things that are sent by default even when consumers aren't all that interested. A model where the systems on the network request the minimum messages they need is probably more scalable.

I argue a little bit differently. This message is not going over the air, it's just on the system MAVLink netwrok, on which there are good resources. Wanting to optimize for all sorts of situations feels a bit like starting to over engineer things a bit, at least to me.

@olliw42
Copy link
Author

olliw42 commented Sep 12, 2024

I really start to wonder why this discussion becomes so signifcant for this message here, and wasn't and isn't elsewhere, For instance, it appeared to be no concern to broadcast the AUTOPILOT_STATE_FOR_GIMBAL_DEVICE message at 100 Hz and it was only for a buffer thing in the greemsy that it was tamed down to 50 Hz ... and other examples exist where ArduPilot deals with high rate MAVLink message streams.

Sure, if it is used it puts burden on the system ... but this is so with (m)any other things too, it's not exclusive to this one here ...

@hamishwillee
Copy link

I think we've all been running into bandwidth issues lately, so this is "on our mind". You're quite right that this is no worse than many other cases. Also that if this is on an internal network that it isn't particularly significant. I will keep quite now.
P.S. The ability for Lua to do anything is irrelevant since Lua support is not an expectation on MAVLink systems.

@olliw42
Copy link
Author

olliw42 commented Sep 12, 2024

P.S. The ability for Lua to do anything is irrelevant since Lua support is not an expectation on MAVLink systems.

I understand that :).
What I'm saying is that not any task a particular autopilot system may want to offer to their user needs a dedicated MAVLink solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants