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

Component information protocol - move into standard #1806

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Feb 24, 2022

This PR is being used to scope precisely what of the Component Information Protocol is stable and can move into the standard.

In this case stable means:

  • Under change control/do not update without same approval as common.xml
  • For schema it also means an approved update requires a new version number.

The things that are stable:

The following types/schema are not stable, and have had wip added:

  • COMP_METADATA_TYPE_COMMAND
  • COMP_METADATA_TYPE_ACTUATORS
  • COMP_METADATA_TYPE_EVENTS

Non blocking but relevant:

Outside scope of this discussion:

@bkueng
Copy link
Member

bkueng commented Feb 25, 2022

Makes sense. COMP_METADATA_TYPE_PERIPHERALS and COMP_METADATA_TYPE_EVENTS should be wip too.
We really need to work through the translation, I'd like to get this worked out (as in: implemented) as well as part of stabilizing it.

@hamishwillee
Copy link
Collaborator Author

Thanks @bkueng . Updated to reflect your confirmations.

Yes, lets discuss translation in #1656 (comment)
I

@hamishwillee hamishwillee marked this pull request as ready for review March 2, 2022 04:42
@hamishwillee hamishwillee added the Dev Call Issues to be discussed during the Dev Call label Mar 2, 2022
@hamishwillee
Copy link
Collaborator Author

Questions for James to ensure are answered in docs:

  1. CRCs for everything in general and linked from general implies that they can/must be statically built at compile time.

    what about translations? Are we forcing the translation file to be known at compile time, and impossible to update?

@auturgy
Copy link
Collaborator

auturgy commented Mar 2, 2022

How would this deal with dynamic parameter creation (ie https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Scripting/examples/orbit_follow.lua)?

@hamishwillee
Copy link
Collaborator Author

@bkueng We discussed stability and fitness for purpose of this in the dev call. There was some concern around assumptions made that everything in general metadata was static, and whether it should be. A few points for discussion below.

Note, @julianoes I asked you to do this, but taking a shot myself. Can you sanity check I did not miss anything?

Translations

Translations are defined in the general metadata and therefore the theory is that they can be statically built at runtime. This prevents the translations being extended and improved after release - an unreasonable restriction.

I am not sure about the solution, but here are some kind of "requirements":

  1. It should be possible to update translations after compile time and make them still available and updatable by the GCS
  2. The vehicle should not need to know about the updates. So we need a scheme where the GCS can get the information from the definition file, a convention, or the file that is serving it. All it can get from the vehicle is perhaps the initial path.
  3. For the above reasons I don't think the vehicle should ever provide a translation CRC, though it might supply the location of a CRC.

I don't know how others handle this. My thinking is perhaps we:

  1. Make the assumption that the server will always serve the latest version it has at the requested path or filename.
  2. Make the assumption that the GCS will always attempt to update it's set of metadata on reboot. It always over-writes whatever was there.
  3. There is no attempt to cache translations - we just accept the cost of downloading them.

Is parameter metadata really static

Is the assumption that parameter metadata can be all be known at compile time actually true?

We discussed that while some parameters may not exist at compile time, as they are enabled at runtime by the values of other parameters. Don't think that matters - a GCS that has all the metadata can apply it as needed when the parameters become visible. I presume that was the intent (we should make clear in docs)?

What about dynamically added parameters? For example ArduPilot allows a script like https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Scripting/examples/orbit_follow.lua to add a parameter at runtime. It would be good if we could provide a way to allow the metadata to be updated by the vehicle using the component information message. If this is not possible we need to explicitly preclude it.

Suggestions

It could be that you already have answers to all this, but just my two bits:

  1. I think we need to add support/assumption that any component information might be dynamic. That includes "general" stuff like translations and parameters.
  2. But it should also be possible (and recommended) if you know that something isn't dynamic to hard code at compile time.
  3. My gut feeling is that we'd be better off with a more (the original) granular approach where COMPONENT reflected a specific component file. We might include all supported types in there too - or that could be in the general file (but no CRC. Then on change we emit the same message with the particular file type that has changed a new CRC.
  4. I don't think we should emit CRC for translations. I'd be tempted to suggest that the location of translations be in the associated metadata file, without any CRC - see comments above.

@bkueng
Copy link
Member

bkueng commented Mar 8, 2022

Re parameters

I presume that was the intent (we should make clear in docs)?

Yes that's how it's implemented.

What about dynamically added parameters? For example ArduPilot allows a script like https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Scripting/examples/orbit_follow.lua to add a parameter at runtime. It would be good if we could provide a way to allow the metadata to be updated by the vehicle using the component information message. If this is not possible we need to explicitly preclude it.

I see several options for this:

  1. update the metadata on bootup, which means it looks static to a client (this has the best trade-offs to me)
  2. ignore those params, i.e. having no metadata (obviously the simplest solution)
  3. allow for dynamic updates. There is a path to extend the protocol for that (COMPONENT_INFORMATION: dynamic updates #1790), w/o breaking, so it can be added later on. We might make that more granular to send individual CRC's for only the types that updated. However if you're already dynamically generating potentially large json files, the additional burden to update the general json file (which is small) is acceptable to me.

Generally I want to make sure we reduce dynamic updates as much as possible, because:

  • it puts an additional burden on the clients (i.e. a GCS needs to update all existing metadata references at any point in time)
  • you cannot reasonably do compression on an FMU (this is a non-issue for linux companions), i.e. for PX4 uncompressed parameter metadata size is 1.2MB, whereas compressed it's 70KB.
  • the more dynamic it gets, the more core aspects of the protocol are defeated (simplicity, caching+minimal number of transfers, translation)

@hamishwillee
Copy link
Collaborator Author

@auturgy (As we surmised in the dev call) Beat confirmed that the expectation is that the GCS is provided with all metadata for all parameters known at compile time, whether or not they are enabled at a particular time. So the intent is that the GCS will only display metadata for active parameters.

I assume that is what things like Mission Planner and QGC currently do via whatever mechanism they are currently supply metadata.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Mar 9, 2022

@bkueng Your response

update the metadata on bootup, which means it looks static to a client (this has the best trade-offs to me)"

is the same answer we had for lua-based actuator metadata. We don't require that the files are compile-time generated - just that they are fixed after boot. In other words we don't preclude using metadata for Lua but nor do we explicitly support it, and we don't support plugin/plugout after boot.
@auturgy This argument convinced you last time :-)

In principle I'm OK with the same answer here "for Lua". IMO it would be stupid to block huge benefit of this approach for parameter metadata delivery because it doesn't support Lua parameter metadata, especially I don't think Lua has a way to even set such metadata internally yet.

But in practice I don't fully believe it will work for all cases. In particular, this forces you to have the parameter metadata on the vehicle and not on a server somewhere. So adding your own parameters on top of that would be hard.

Further, we keep on running into cases where some level of dynamic behaviour would help. So I'd like us to think through whether this could realistically be done with the dynamic updates, and whether there is anything else we might do to improve that proposal to help with cases like this. And because we're trying to get this approved, I'd like to do it now before we freeze the interface.

The fundamental flaw in our current dynamic design is that it is all or nothing. If someone adds a Lua script with 4 new parameters, we have to regenerate the 1000 static ones as well, and compression is "hard". Ditto for a new smart battery. It seems silly to regenerate the whole parameters file we already compressed for a small addition. What might be better is if we could specify two files for metadata - one fixed and one dynamic.
So if you saw parameter metadata in both general_metadata and in peripherals (let's call it "dynamic") you'd know that they should be treated additively? You could probably compress the small dynamic file if you want, but you might not bother.

Thoughts?

@julianoes
Copy link
Collaborator

julianoes commented Mar 15, 2022

It would all get a bit simpler - in my opinion - if we had a flat list of things that can be queried, some might be static, others might be dynamic. Having the overall file that pulls them all together in yet another file makes it harder indeed, I also realized that while I was doing the implementation. It becomes quite complicated with multiple steps, some of them involving caching which I did not even bother about yet (see MAVSDK source).

Wouldn't it be simpler if an autopilot could just send out a COMPONENT_INFORMATION(_UPDATE) with a URL and CRC announcing one file with one type at a time, static or dynamic. This could be the static list, peripherals, lua additions, whatever.

And for the metadata that we have in general, I would use COMPONENT_INFORMATION_BASICS instead. I think no one can be bothered to implement MAVLink FTP just to send the version and vendor name.

@hamishwillee
Copy link
Collaborator Author

@julianoes I went around a few loops with @bkueng on this over the week. This is more or less what I proposed at one point. Several options discussed below, including this one. Let's discuss in the dev call and agree the costs and benefits of each approach.

Proposed change to original design

These changes to the original design that address the issues we had with translation files, files that must be built at runtime, dynamic updates, in a compatible way. In summary it removes dynamic CRCs from the metadata and provides a more efficient and tunable mechanism for change notification. Everything can be done in the same way, but it is more consistent and scalable if you need that functionality.

Detail:

  • general information schema lists all supported types (static and dynamic), their URLS and their translation URLs (if relevant). CRCs are listed for meta that are statically generated. CRCs for translations and dynamically generated metadata are not listed.
    • This is queried in the same way as currently in order to understand what types are available and which are dynamic
    • Dynamic files would not be updated until there is a notification (see further down)
    • Nothing has to change - though we could update the schema to remove the translation CRC that we on't use
  • We no longer use COMPONENT_INFORMATION.peripherals_url. We keep it so we don't have to break existing systems, but add comment it is deprecated.
  • We define COMPONENT_META_UPDATE that is an efficient message for streaming the CRCs of dynamically updated files.
    • This might just have small array of metadata type and matching array for CRC. The array would not have to be big because you could stream this with different sets of data at appropriate rates as the data grew.
  • Translation urls are provided in component metadata. New translations overwrite old ones. The GCS is expected to poll the location on a regular basis. If the file is xz compressed it might as well just read it. Otherwise it could check for changes using an HTTP HEAD request and check the Last-Modified header for changes.
  • For cases like parameter metadata where the bulk of data is static, but we have some runtime values we define specific dynamic metadata types and a mechanism to merge them. For example, in the parameter case for Lua scripts we would have parameter_metadata_dynamic that would be dynamically updated. The merge process would be to append to existing set of parameters.

So in summary, for a purely static system nothing changes

  • you read the general metadata to get all the relevant files then you download those individually.
  • You also get the translation URLs which you poll for changes or just update regularly
  • you get the CRCs for the static files, but these only really matter if you update your firmware, meaning that they don't need to be downloaded in that case.

For a dynamic system you read the general metadata and you wait for a matching COMPONENT_META_UPDATE. If the CRC has changed (which it will the first time you get this message) you read the file and update.

The update will be streamed at an appropriate rate for the data and use case. So for example you could treat the lua parameter update case as a single runtime update on boot, and emit that at incredibly low rate. Or you could emit it regularly and literally plug and play lua scripts.

Note: Dynamic updates are expensive in terms of compression (in particular) and streaming, so you can be more dynamic if you like but you have to accept the costs.

Julian/separated approach

This is kind of what I was thinking as discussed with Beat, merged with Julian's points:

  • COMPONENT_INFORMATION message has type (of data), crc, url, and maybe translation_url
  • COMPONENT_INFORMATION is polled by GCS on boot - it can request all supported types or can poll for any it is particularly interested in.
    • you could even still get general_metadata to list all the types supported instead/as well.
  • COMPONENT_META_UPDATE gets sent if the metadata changes.
    • @julianoes I suggested emitting COMPONENT_INFORMATION originally on change. @bkueng pointed out this is unreliable. To be reliable it needs to be streamed and this is a heavyweight message. So emitting something smaller but at a controllable rate is more reliable. Overhead can be tuned to near zero for systems that don't care about dynamic behaviour.
  • Maybe we split out the actual data from general metadata and leave it in COMPONENT_INFORMATION_BASICS

Pros and cons

To be very clear, both approaches will work and are actually not dissimilar except in the mechanism used to discover the supported metadata types. I "think" the main benefit of @bkueng approach is that it could be done without a break. If you were willing to break the message then I'm not sure which would be better.

TLDR;

Beat's approach queries just one metadata file, and from that gets all the information about all other metadata files used by the system. Julian's approach gets the metadata file details in multiple messages (which is slightly less reliable).

If you accept that we have to do something like COMPONENT_META_UPDATE to get reliable dynamic updates [is this valid?] then the methods are otherwise pretty much identical.

My "intuitive preference" is for a design like Julian's - but this is mostly because it is a familiar pattern. That said, I can't think of great reasons why you need to be able to make the message more granular.

Perhaps:

  • A GCS can implement just what it likes - a GCS just interested in parameter metadata just polls uses that. It doesn't need to parse a file to get the additional information.
  • Julian's approach does not preclude Beat's - you can still choose to get the files from metadata rather than a separate message.
  • It would allow a rename to COMPONENT_METADATA :-)

@julianoes
Copy link
Collaborator

To address the reliability I would add an id in each COMPONENT_INFORMATION and a max_ids in COMPONENT_INFORMATION_UPDATE, so then you can request individual ones that you missed. In my view it gets quite ugly if we have this COMPONENT_INFORMATION message with a big peripherals field that is deprecated, and that's before it's even in common.

I understand this would cause churn in QGC but I'd argue it's worth it to clean it up.

@hamishwillee
Copy link
Collaborator Author

@julianoes I agree the message is ugly, but then we don't have to manage the churn. Saying that, if I was @bkueng I'd rename it to COMPONENT_METADATA, or similar (maybe move it to px4.xml) and have the messages live in parallel

Either way, we'd still need to agree the form of the messages - both ways work. Good discussion for the dev call. I've asked @bkueng if he can attend, but he might not get the message.

@hamishwillee
Copy link
Collaborator Author

Discussed in 20220317-Dev-Meeting. Summary:

  1. We're largely going with the "proposed" change to the original design discussed in Component information protocol - move into standard #1806 (comment).
  2. This will be done in a new message COMPONENT_META (or COMPONENT_METADATA - up to @bkueng). The new message will start WIP and the old one will be deprecated.
  3. The main reason for this choice is that there is not room enough in a single message to include url, fallback url and translation URL. Having all the information in the general information file and making that always available from the vehicle means that we can reliably ensure the information is retrievable.
  4. Some changes may be desirable for the general schema - e.g. removing translation CRC and possibly removing the duplicated info in COMPONENT_INFORMATION_BASICS (@julianoes to confirm the later). This is not mandatory, but if done, schema version should be iterated and please let us know Beat.

New round of prototyping, then we should be able to get this in.

@julianoes
Copy link
Collaborator

Given a couple of people I have talked to in the past that "just wanted to share a version and not bother with FTP" I would suggest we leave the BASICS message. Otherwise it makes the hurdle to create a MAVSDK component unnecessary high which hinders adoption.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Mar 22, 2022

Thanks @julianoes - makes sense. And I think we can leave it in the general metadata too, since a GCS is happy to get all the data in one go

@hamishwillee hamishwillee mentioned this pull request Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Call Issues to be discussed during the Dev Call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants