-
Notifications
You must be signed in to change notification settings - Fork 287
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
base: master
Are you sure you want to change the base?
Conversation
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. |
@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 ... |
could this be added to the devcall for consideration ? |
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. |
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? |
There was a problem hiding this 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
many thx for looking at it
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.
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. |
My 2 bits
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. |
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
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.
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. |
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 ... |
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. |
I understand that :). |
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:
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 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 :)