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: add actual_compid field #1394

Closed

Conversation

ndepal
Copy link
Contributor

@ndepal ndepal commented Jun 9, 2020

As discussed in https://px4.slack.com/archives/C52DXKYLV/p1591641605371800

This is useful to communicate component information for components that do not have their own mavlink interface. For example, a GPS device connected to a flight controller.

@julianoes I made it part of the message since it's still marked WIP, but I can make it an extension if you prefer.

@julianoes
Copy link
Collaborator

I think this is a good start to discuss in the MAVLink call tomorrow, 9am CEST. The chance to get it in is best if you can show up.

@ndepal
Copy link
Contributor Author

ndepal commented Jun 9, 2020

Ok, I'll try to attend.

@julianoes julianoes added the Dev Call Issues to be discussed during the Dev Call label Jun 9, 2020
@hamishwillee
Copy link
Collaborator

Not sure about this. FWIW I think I'd rather that either:

  1. We made it easy (in general) for an autopilot to emulate the behaviour of mavlink components with heartbeat etc.
  2. We made it easy to fake component id for outbound messages, and told the autopilot (or other system) it has to accept inbound commands to those components.
  3. We allow an autopilot (actually any component in system) to output the component metadata for any/all other components if it chooses (sorted by type/component id). That way the autopilot can output this information for the attached "not real mavlink" components. If there is conflict the one with the component source id is used.

I tend towards 3 because it is the minimal solution.

@ndepal
Copy link
Contributor Author

ndepal commented Jun 10, 2020

I was unfortunately unable to attend. Did you get a chance to discuss this?

@hamishwillee If I understand your proposal 3. correctly, would that not also require an additional component id field?

@hamishwillee
Copy link
Collaborator

hamishwillee commented Jun 11, 2020

Hi @ndepal
No, because you wouldn't request the component specifically. You'd request "all metadata from autopilot" and the autopilot (presumably) would send back not just its own information, but that for all components.

We talked about this in the dev call. The problem with option three is that it requires quite a lot of infrastructure to get the information - MAVFTP for a start, and the format of that metadata isn't defined. Essentially we think it is a good idea, but not when all the pieces of the story are still in flux.

The right way to do this is to be able to update the generated libraries to support setting an arbitrary component ID, but that may be contrary to the spec. We may well need this in future anyway, but we don't have it now.

So even though none of us liked this proposal, but it is the only one that delivers what you need in a reasonable timeframe. So we thought maybe merge as WIP but it might well change at some point before it stabilises.

Leaving to @julianoes to confirm that and merge if he is happy.

@ndepal
Copy link
Contributor Author

ndepal commented Jun 11, 2020

Ok thanks for the explanation.

@julianoes
Copy link
Collaborator

@hamishwillee I found one more problem with this. I'm gonna try to explain it:

So basically @ndepal needs the version of a component which is not a MAVLink thing itself.

You suggested with 3. to enable the autopilot to share some sort of json file with versions of components. This would then be downloaded using MAVLink FTP. I already brought up in the call that while I see how all this meta data can be useful and makes sense, that it is also basically a way to share data that is less (or differently) standardized, so it is more flexible for specific use cases. Basically, the meta data is a workaround for the fact that the MAVLink messages don't really have versioning (apart from extensions) and arbitrary size, whereas the json schema will have a version which can be iterated and more data can be added more easily (and MAVLink FTP takes care of the retransmission of the file chunks).

Anyway, as we're discussing the COMPONENT_INFORMATION in #1346, part of the proposal is to move the version fields into metadata as well (search for firmwareVersion). This means that once #1346 goes in the version field that @ndepal requires is removed 😭.

This means that we either need to leave the version field in the message and not move it to the json "metadata" file, or we need to go towards that new proposal already meaning that we need to have a way for any component to be represented in the json file and @ndepal needs to implement MAVLink FTP in whatever client he is using (if it's MAVSDK it's already included) to get the version.

@DonLakeFlyer
Copy link
Contributor

This is superceded by #1408

@hamishwillee
Copy link
Collaborator

@DonLakeFlyer I'm not sure the new PR addresses the use case (though it does make this one not work anymore)

The need here is to be able to get the version information from a non-mavlink component in a system, allowing all the components in a system to be enumerated. This basically added a field to say that I really want the details for the specified component id.

I did suggest above we could have the autopilot returning version info for all attached components. Maybe that would work.

Suggestions?

@julianoes
Copy link
Collaborator

julianoes commented Jun 23, 2020

@DonLakeFlyer @hamishwillee can we bring the version field back to the COMPONENT_INFORMATION? It's something needed for @ndepal and the v2 gimbal proposal as mentioned in #1370.

@DonLakeFlyer
Copy link
Contributor

I'm not sure the new PR addresses the use case (though it does make this one not work anymore)

I was thinking for some reason the version json had component id which it doesn't. So my comment is wrong.

@stale
Copy link

stale bot commented Aug 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Aug 22, 2020
@hamishwillee
Copy link
Collaborator

@julianoes As you know the component information message has pretty much turned into a system for supplying metadata file locations - ie the message itself doesn't carry much other than information about what metadata is supported and where you can get it. There isn't space in the payload for this any more. Also, this wouldn't be the "right" way to do this.

I think now what you would do is send MAV_CMD_REQUEST_MESSAGE to the system and component you want specifying the id for the COMPONENT_INFORMATION message. All components should respond with their version information in this format https://github.com/mavlink/mavlink/blob/master/component_information/version.schema.json

Components will have to support MAVFTP and include a version file. I suspect that an autopilot that wants to supply information for "not real" components will have to fake the source component ID.

Thoughts?

@stale stale bot removed the stale label Aug 24, 2020
@stale
Copy link

stale bot commented Oct 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Oct 23, 2020
@hamishwillee
Copy link
Collaborator

This is no longer appropriate, but might be considered for COMPONENT_INFORMATION_BASIC.

@stale stale bot removed the stale label Feb 16, 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.

None yet

4 participants