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

Proposal for fast yet simple and fully mavlink conform parameter upload #1918

Closed
olliw42 opened this issue Nov 15, 2022 · 17 comments
Closed

Comments

@olliw42
Copy link
Contributor

olliw42 commented Nov 15, 2022

Some time ago, ArduPilot has introduced upload of the parameters via MAVFtp, and the speed-up as compared to ArduPilot's normal upload via the PARAM messages is certainly stunning. However, this is largely a stunt, as will be demonstrated below. The same can in fact be achieved without all the burden of MAVFtp and in a clean "mavlink-ish" way, as will also be shown.

Furthermore, ArduPilot's MAVFtp parameter upload seriously conflicts with components in the system, like gimbals and cameras. The problem is that in order to get the parameters of the components, one would send a PARAM_REQUEST_LIST message as broadcast, which however can confuse ArduPilot as it would now get both the hint to downlaod via MAVFtp and via PARAM. A way out would be to send PARAM_REQUEST_LIST messages targeted for each component individually, which isn't been done however. As a matter of fact, both MissionPlanner and QGC today show worse behavior concerning components than years back.

The alternative scheme suggested here is equally good to MAVFtp in terms of download performance but totally avoids the said issue, and also provides further signifcant benefits and advantages, as will be discussed at the end.

In order to follow some calculus below, for reference some numbers:

  • in my ArduCopter system the typical number of parameters is 1200
  • we may determine a "flat size" of 23 bytes for a parameter: 2 bytes index, 16 bytes name, 1 byte type, 4 bytes value. The index can be disputed and one may argue for 21 bytes, but such details will not affect the arguments to be made below. So let's go with 23 bytes per parameter for the sake of this proposal.

Analysis of the situation, experimental data

Let's start with discussing why param upload via MAVFtp is a stunt. This is important to understand, to not be missled by whatever one may have heard or read or have been told or have seen oneselves.

The download speed is determined by these factors:

  • transmission rate of packages
  • size of a package
  • overhead
  • data compression

Obviously, a fair comparison between different schemes would require that the (average) data rate (bytes per sec) is equal.

If this is not respected, then the total download time and thus the subjective impression of a user may be quite different for two schemes, but for total trivial reasons.

For instance, if two schemes A and B send their packets at the same rate, but in scheme A the packets are 10 times larger than in scheme B, then it is obvious that scheme A will be 10 times faster than scheme B, and a user might be heavily impressed, even though all what has happened is that simply a 10 times higher bandwidth was allowed for scheme A!

Exactly this is what largely happens in case of ArduPilot's MAVFtp and PARAM schemes. For MAVFtp, the parameter data packets are send out as frequently as possible. The packets are in fact send out that densely and agressively that not even the 1 Hz HEARTBEAT would be send out inbetween. In contrast, for the PARAM scheme ArduPilot has a pretty weired streaming method, which results in that PARAM_VALUE messages are send out in typically 20 ms intervals. Ergo, the download time is ca 1200 * 0.02 sec = 24 sec, even on a high speed link.

A fair comparison would also allow the PARAM_VALUE messages to be send out as densely as possible.

This unfair comparison accounts for most of the subjective impression of the MAVFtp scheme.

Nevertheless, truth is, ArduPilot's MAVFtp scheme does introduce some nice ideas, and does objectively bring some speedup, for these reasons:

  • the overhead is significantly smaller:

Let's consider the size of a full mavlink frame of 267 bytes. This number of bytes is good for 6.9 PARAM_VALUE messages (size = 37 bytes), so the overhead is 61 % (37/23 - 1). In comparsion, a MAVFtp packet provides 240 bytes payload and thus can carry 10.4 parameters. The overhead is thus only 11% (267/240 - 1).

In other words: In the time it takes to transmit 267 bytes one can transmit 10.4 parameters with MAVFtp or 6.9 parameters with PARAM.

  • compression:

ArduPilot's MAVFtp implements a nice compression scheme (which can be further improved btw). On my system it yields a compression ratio of ca 2.2.

This estimate is obtained as follows: With MAVFtp I find that ca 52 messages are required to transfer all parameters, that is 10.4 bytes / parameter (52 * 240 / 1200 = 10.4). This can be compared to the flat size of 23 bytes per parameter. This yields a compression by 23/10.4 = 2.2.

Therefore, all together, MAVFtp provides a 3.3 (10.4/6.9 * 2.2) times faster download than PARAM.

3.3, that's the objective measure for the "very efficiently" and "a lot faster" parameter download of MAVFtp.

Proposal

However, this can equally be achieved also without all the burden and blunder of MAVFtp, in a total simple and easy to migrate fashion.

The idea is to introduce a new message PARAM_VALUE_ARRAY, which can host a number of parameters, which can be compressed in exactly the same way as in ArduPilot's MAVFtp scheme. The message I am using in my tests is:

<message id="60040" name="PARAM_VALUE_ARRAY">
  <description>Parameter multi param value container.</description>
  <field type="uint16_t" name="param_count">Total number of onboard parameters.</field>
  <field type="uint16_t" name="param_index_first">Index of the first onboard parameter in this array.</field>
  <field type="uint16_t" name="param_array_len">Index of the last onboard parameter in this array + 1.</field>
  <field type="uint16_t" name="flags">Flags.</field>
  <field type="uint8_t[247]" name="packet_buf">Param value packet buffer.</field>
</message>

The usage is simple: In response to a PARAM_REQUEST_LIST, the component sends out as many PARAM_VALUE_ARRAY messages as needed, instead of the stream of individual PARAM_VALUE messages. Everything else of the PARAM microservice remains as before. The parameters can be packed into PARAM_VALUE_ARRAY using the very same compression scheme as used by ArduPilot's MAVFtp. The flags may allow different schemes in future.

Obviously, this approach trivially resolves the issue with components described before. Also, obviously, it can be as efficient as ArduPilot's MAVFtp.

In theory, this message might offer even slightly higher efficiency than the MAVFtp message, given that the available payload size for the parameter data is slightly larger (247 vs 240) and the overhead hence slightly smaller. However, this isn't fully realizable, since e.g. the first param name in each PARAM_VALUE_ARRAY must be stored full size (common_len = 0). It should be darn obvious however that using PARAM_VALUE_ARRAY is on any practical level as efficient as using MAVFtp messages.

Advantages compared to MAVFtp param upload

  • A most significant advantage is that the burden of MAVFtp is not needed. All it needs is to send a PARAM_REQUEST_LIST, as before MAVFtp.
  • The proposed approach can in particular easily be implemented also by components, such as gimbals and cameras. This point should not be underestimated. For instance, the STorM32 gimbal controller has ca 250 parameters, which also have to be downloaded to the GCS. Some users have even two STorM32 gimbals installed, which brings a total of ca 500 additional parameters. Like-wise with cameras. It is pretty unrealistic that such components would support ArduPilot's MAVFtp parameter upload in some near future, and if param download would have to be scheduled component-wise anyways.
  • Another important advantage is that the discussed issues with getting the component parameters at connection time are trivially resolved, since in any case simply a PARAM_REQUEST_LIST would be broadcasted as it was done before ArduPilot's invention.
  • Lastly, it is, at least in my opinion, an important advantage that the proposed approach seamlessly fits into the existing parameter microservices and mavlink habits. It is conceptually clean. In fact, this can be seen as just another lesson that better approaches can result by trying to stay within the concepts of mavlink.

Next steps

I do have a test implementation using BetaPilot and mLRS. If the above proposal, after some discussion as needed, finds general agreement, I would finalize these test implementations based on the results of said discussion, could attempt an implementation in QGC, and make all that available to the public. This may serve as proof of principle, and maybe as example code.

Cheers,
Olli :)

@hamishwillee
Copy link
Collaborator

hamishwillee commented Nov 15, 2022

Thanks @olliw42 . I'll add it to the dev call.

My two bits:

  • Useful analysis/comparison. Even better if you didn't use emotive language like "stunt". It's an engineering solution that provides about 3x performance for the main use case of interest to the autopilot/GCS (according to your figures). The analysis and implications need stand on their own. Such as "what problems the mavftp based solution has for components".
  • The ArduPilot solution has not gained traction in PX4, and is hence not common, or likely to be common. PX4 does not historically have anywhere near as many parameters or as complicated changes in parameters, so there hasn't been such a need (may have changed with control allocation). Without a stakeholder willing to commit to implementation it can't go into to common. The most likely investor (Auterion) is not particularly interested in the approach.
  • One benefit and cost of the MAVFTP solution is ... MAVFTP. Reusing software is something that is already shared by the flight stacks means easier development work. The same approach can then be used as the basis for passing other data around. The cost is that if you're just wanting to pass parameters around, then adding FTP is overhead.
  • Will need to add mechanisms to allow backwards compatibility.
  • From a protocol description point of view it is very simple. However from an engineering flow point of view it might not be so easy to implement, particularly if it needs to work in parallel with the current parameter protocol.
  • This potentially provides a path to have a common parameter protocol and extended parameter protocol with ArduPilot, and do away with the current differences with ints being sent in floats.
  • I am keen that the solution works for all components.
  • PX4 should be interested because the speed implications are significant.
  • It is "fighting against" an entrenched approach in ArduPilot. Doing nothing is easier, in particular when this will not speed up the GCS/Autopilot interaction, which was the use case that drove the development in the first place.

I plan to advocate for this, because I think its a great compatibility solution; no guarantees though - we need at least two flight stacks to show interest.

You can help by providing more detail on where the existing solution and ardupilot solutions fail for components.

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 16, 2022

many thx for these quick thoughtfull thoughts

Will need to add mechanisms to allow backwards compatibility.

yes. I don't see however how this would be different with any change to a "new" protocol aspect, i.e., e.g. different to the situation with MAVFtp vs PARAM

from an engineering flow point of view it might not be so easy to implement, particularly if it needs to work in parallel with the current parameter protocol.

gladly, sampe implementations are available. We have ArduPilot & MissionPlanner, and it is even in GQC. I.e., it should be simple to implement it in QGC, since most of the "tricky" code (if one wants to call it so) is already there.

Doing nothing is easier.

yes, doing nothing is always easier. But often a significant blocker for any progress. See also response to next point.

You can help by providing more detail on where the existing solution and ardupilot solutions fail for components.

anyone can easily test this:

Use QGC with ArduPilot with MAVFtp enabled and some components with parameters attached to it. QGC will NOT show any components, e.g. in the Vehicle->Parameters sections.
I first thought this is so because QGC has stripped off component support, but no, it's because when it's ArduPilot it tries MAVFtp and in that case it does not send out also a PARAM_REQUEST_LIST, so the parameters for the components are simply never asked for!
https://github.com/mavlink/qgroundcontrol/blob/master/src/FactSystem/ParameterManager.cc#L530-L564
I then thought it's going to be simple to resolve, but no. In order to overcome this I ended up working along an approach where you first do MAVFtp, wait and see, and then send PARAM_REQUEST_LISTS individually to each components individually ... I find that difficult to do since it's a lot code change and I am not through it yet.
That's not the end of the story. I would want to support COMP_METADATA in STorM32, especially the parameter, events, and mission pieces. But hey, how much sense does it make to have that if one hasn't ever downloaded even the parameters? Long story short: Either one spends all the time and efforts to just get along with MAVFtp param upload or one gives up the idea of benefiting from COMP_METADATA.
THIS is what I call a progress blocker.

Use MissionPlanner with ArduPilot with MAVFtp enabled and some components with parameters attached to it. MP will NOT bother with uploading the components parameters. You will have to go to the component page, and press "refresh" yourself to get them. MP will also be struggling with showning the MAVFtp button. It will show a strange @sys. ... MP has just become a pain with reagrds to components. I assume that it does this because it uploads via MAVFtp at connection, for reasons layed out.

It is "fighting against" an entrenched approach in ArduPilot

yes, but be fair, it is going to be a fight only because it is not just about rational engineering but all the stuff beyond.
I am aware of this stumbling stone, and understand that it is where things are likely to fail, as it has been so in many other mavlink topics in the past.
But all this will NOT make me stop to at least propose what may be a much better solution. It's all I can do, but it's what I can do. :)

@hamishwillee
Copy link
Collaborator

  1. My first thought is that means MP is broken - it should test for support for the mavftp solution and fall back to the existing (slow) parameter protocol. That's not a justification for this proposal, it's a justification for a bug fix.
  2. Yes, support for a backwards compatibility is business as usual - I only mentioned it "for completeness".
  3. Great that you are happy to do some development work to maybe get this working. That's the sort of thing that might get this across the line.
  4. Very pleased you're still making these proposals. As I said, I am supportive.

W.r.t component metadata, if you want to use it then you're buying into the implementing MAVFTP. I have read your previous arguments about this actually restricting the use to GCS/Autopilot, and to a large extent I agree. What we are doing is trying to make sure that at least essential metadata is in messages - such as https://mavlink.io/en/messages/development.html#COMPONENT_INFORMATION_BASIC - and other cases we might take on a case-by-case basis.

NOTE, you raised a few other issues. I might not get to look at them until next week.

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 17, 2022

My first thought is that means MP is broken - it should test for support for the mavftp solution and fall back to the existing (slow) parameter protocol. That's not a justification for this proposal, it's a justification for a bug fix.

I then was not clear enough with my words. First off, the issue is in both MP and QGC. Second, the issue is that if you get the params from ArduPilot via MAVFtp you do NOT want to also send a PARAM_REQUEST_LIST as broadcast (since, this would make ArduPilot send its params again, and now even via slow PARAM ;)). Both MP and QGC simply do not send any PARAM_REQUEST_LIST to anyone upon connection (for QGC I know for sure since I looked also at the code, for MP it's what I deduce from its behavior). Getting around it is possible, but a "pain", at least with GQC it requires quite some rework (as much as I can see). Without proper handling of component params it doesn't make so much sense to do COMP_METADATA.

Great that you are happy to do some development work to maybe get this working. That's the sort of thing that might get this across the line.

I am ... but in this case I would go beyond what I have done so far only if there would be some general approval (so far I have done (only) as much as I needed to see it and convince me of the concept, but I didn't push my efforts that far to produce any code I would want to show)(I won't for the obvious reason that all the work is for the bin otherwise).

chicken egg situation

metadata ... you're buying into the implementing MAVFTP

STorM32 supports MAVFtp since more than 3 years ... since the early days of COMP_INFO ( :) ) ... yet it won't ever support ArduPilot's param upload (for various reason I don't want to defend here). This part of the game is all there :) COMP_INFO_BASIC + COMP_METADATA is essentially the old COMP_INFO, but split into two messages, which makes sense, I like it :)

@dakejahl
Copy link
Contributor

This really doesn't sound that bad for backwards compatibility. If PARAM_VALUE ARRAY is not implemented, the component will just respond to a PARAM_REQUEST_LIST using PARAM_VALUE. If the GCS does not implement PARAM_VALUE_ARRAY, then it will trigger the timeout to resend the PARAM_REQUEST_LIST message. A component can see that it has gotten two PARAM_REQUEST_LIST messages consecutively, infer that the GCS does not support PARAM_VALUE_ARRAY, and then fallback to just sending PARAM_VALUE. Am I missing something?

I'd be willing to help with the QGC implementation. I'm not as familiar with the PX4 parameter streaming, the only complicated piece of this is the fallback from PARAM_VALUE_ARRAY to PARAM_VALUE in the case of missing GCS support.

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 17, 2022

many thx for the thoughts. You pointed to the one situation were achieving backwards compatibility is not so clear. Two comments pl.

  1. Your idea may work, but I think there can be edge cases where it jumps to slow param even though fast param would be possible. We should not forget that a link can be lossy, so a gcs can't decide reliably if it didn't got a response because of loss or because the component wants to tell "I can't". Also, it is not so trivial on the component side to decide when it's a first or a second. A way out would be a new message PARAM_REQUEST_LIST_ARRAY, which a gcs tries first and then can fall back to PARAM_REQUEST_LIST.

  2. I think we really want to get away from all these fallbacks, they are annoying to implement and maintain, and add lots of time delay users may find annoying. Let's also not forget about PARAM_xxx_EXT, which is lingering around too. So, if you would ask me, I would suggest to just not be fully backwards compatible here, but to tell every user of a component who is upgrading to a version which supports PARAM_VALUE_ARRAY to also upgrade the gcs. I think one can safely assume that adoption by GCSes can be very fast, while components will follow more slowly. If this is a likely time scale of the events to occur, then there is probably not a significant issue at all. I know that breaking backwards compatibility isn't very popular, but that's what I would do. :)

@hamishwillee
Copy link
Collaborator

I'm on other work today, but IMO this would be easy to sort out with a protocol bit that indicates support for the new protocol.

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 17, 2022

IMO this would be easy to sort out with a protocol bit that indicates support for the new protocol.

unfortunately not. The situation considered here is that a OLD gcs is used, which does not know about PARAM_VALUE_ARRAY, but the component supports PARAM_VALUE_ARRAY, i.e. old gcs - new component ... but if the gcs is old, it then also does not know about any newly added protocol bits ... and moreover it would not have a means to tell the component that the component should send PARAM_VALUE and not PARAM_VALUE_ARRAY.

note that all other combinations, new gcs - new component, new gcs - old component, old gcs - old component are unproblematic, they just work.

@dakejahl
Copy link
Contributor

Old GCS - new component ... this is actually very common -- the HereLink. It runs a v4.0.8 fork of QGC but it can be updated. Although it's an older QGC version, it should be relatively easy for the project maintainer(s) to add support for the new message and push it in a new release. A ton of people use the HereLink so for that reason we need to make sure they are aligned with this proposal. @hamishwillee do you know the right person at cube pilot to bring into the discussion?

@peterbarker
Copy link
Contributor

It is "fighting against" an entrenched approach in ArduPilot. Doing nothing is easier, in particular when this will not speed up the GCS/Autopilot interaction, which was the use case that drove the development in the first place.

Pretty sure you're not under-estimating that. But don't underestimate it :-) I expect someone would point out that mavftp is a solid generic transport mechanism, and that adding support for PARAM_VALUE_ARRAY would cost us flash for a mechanism better from an abstraction perspective but has no massive advantage from a technical perspective.

Please note that we also transfer other "bulk" items like fence items and mission items using mavftp, so there's re-use there.

And yes, the opaque, not-yet-standardised formats for these transfers bothers me a bit.

@olliw42 's musing on PARAM_REQUEST_LIST no longer being issues is interesting. A GCS not doing that could be considered a good thing in that in some situations you may not want to download parameters from all vehicles/components you can see! The GCSs not fetching parameters from other components should be fixed - probably by separately requesting parameters based on heartbeats-received.

I think I'd agree with @olliw42 's suggestion about the need for PARAM_REQUEST_LIST_ARRAY - having a component have to record capabilities of each node requesting parameters isn't pleasant.

Q: PARAM_VALUEs are broadcast. How does a vehicle/component know that everyone receiving these values understands PARAM_VALUE_ARRAY? (note that ftp also breaks this use-case thoroughly ;-) )

My first thought is that means MP is broken - it should test for support for the mavftp solution and fall back to the existing (slow) parameter protocol. That's not a justification for this proposal, it's a justification for a bug fix.

It should fall back - I've seen it do so. You know I don't use it day-to-day, 'though :-)

In terms of QGC versions - there are a lot of people out there running things like the "H16" controllers with variable versions of QGC on them. These are likely to be entrenched for some time - long time constants on features coming in and out for actual users of our software :-)

@IamPete1
Copy link
Contributor

AP's FTP format now also reports the default value for every parameter, so the user can easily see what has been changed. This functionality should be maintained by any new protocol. There we have a flag bit saying if the param value is currently the default, if it is not we double the data length and send the current value and the default.

@hamishwillee
Copy link
Collaborator

IMO this would be easy to sort out with a protocol bit that indicates support for the new protocol.
....

...
@hamishwillee do you know the right person at cube pilot to bring into the discussion?

@olliw42 and @dakejahl Thanks. I see the problem. Yes a solution could be PARAM_REQUEST_LIST_ARRAY, or an extension/flag to PARAM_REQUEST_LIST to allow it to indicate it can handle the array response, if supported (0 if the GCS has not been updated, so updated component knows that in this case it must send the PARAM_VALUE). That said, if we go on this path I lean towards not being that backward compatible.

@hamishwillee do you know the right person at cube pilot to bring into the discussion?

Yes, if the proposal gets that far.

@hamishwillee
Copy link
Collaborator

@peterbarker et al. I am not underestimating the resistance, and not just from ArduPilot. There is also this assumption in QGC/PX4 that as long as we don't have to download parameters more than once you can live with the current protocol (because they keep a cache value for the downloaded set and track the individual changes).

The "business justification" for ArduPilot is that if this is adopted in the standard then fast parameter upload will go into QGC.

This is very much a case where what is better design might not be worth particular flight stacks adopting. I can see a future where PX4 primarily works around the problem by running only over faster links so doesn't spend the effort of having a faster parameter protocol. I can see ArduPilot never having fast parameter download on anything other than Mission Planner.

I hope that we can get somewhere with this. Not everyone in the world can upgrade to fast links, and some users would like a common protocol.

IMO all the rest of this discussion is IMO just resolvable technical detail/compromise. I'm not planning on investing too much time on it unless I can get broad agreement that this idea can be considered for the standard. That means at least two significant stakeholders need to show commitment to implement, or at least recognition its a good idea for the standard - i.e. if one party wants to go ahead with this that the other will not block it.

I'll raise at the dev call.

PS Yes, the issues with existing protocol and components should be fixed. I assume that should happen as bugs against the respective GCS.

@julianoes
Copy link
Collaborator

I think the way forward is to have a lightweight nice C or C++ library that implements MAVLink FTP and is easy to be integrated into a peripheral/component. That way we can all make use of MAVLink FTP which has proven to work and don't have to re-invent yet more protocols/solutions to do reliable transfer of bytes.

@hamishwillee
Copy link
Collaborator

@olliw42 This was discussed in the mav call and we don't think this should proceed.
In summary, the MAVFTP path is seen as the preferred approach by both PX4 and ArduPilot engineers present given that it is stable and well tested code. With no stakeholder autopilots likely to take this, we can't make it part of the standard.

I'm disappointed because I would have liked a path towards a single parameter protocol; even if PX4 adopts the ardupilot file based parameter transfer we still have param protocol, extended protocol, etc.

Thanks for taking the time to post this. Closing, but you can reopen if you believe I have been pre-emptory.

@olliw42
Copy link
Contributor Author

olliw42 commented Nov 24, 2022

@hamishwillee

many thx for taking the effort to bring it to discussion, really much appreciated. I am not totally surprised by the outcome. From the responses it seems that the disadvantages of param ftp have not fully be comprehended and dito the benefits of the proposed scheme. It is also not surprising that ArduPilot's appetite was low, I am however convinced that PX4 is going to make a wrong decission. Anyway, I don't regret having taken the time to put it up. The idea is now at least around.

Components continue to have a hard time :)

@hamishwillee
Copy link
Collaborator

PX4 will make a pragmatic decision for PX4; I just hope that this on balance does not mean that "standardization suffers".

Thanks for keeping the dream alive :-)

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

No branches or pull requests

6 participants