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

Updates for new COMPONENT_INFORMATION support #1408

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

DonLakeFlyer
Copy link
Contributor

Changes for #1346

@DonLakeFlyer DonLakeFlyer merged commit a8c6eb0 into master Jun 22, 2020
@DonLakeFlyer DonLakeFlyer deleted the ComponentInformation branch June 22, 2020 21:04
@hamishwillee
Copy link
Collaborator

@olliw42 FYI changes to component_information

@@ -6672,13 +6681,11 @@
<!-- This message is work-in-progress it can therefore change, and should NOT be used in stable production environments -->
<description>Information about a component. For camera components instead use CAMERA_INFORMATION, and for autopilots use AUTOPILOT_VERSION. Components including GCSes should consider supporting requests of this message via MAV_CMD_REQUEST_MESSAGE.</description>
<field type="uint32_t" name="time_boot_ms" units="ms">Timestamp (time since system boot).</field>
<field type="uint8_t[32]" name="vendor_name">Name of the component vendor</field>
Copy link
Collaborator

@hamishwillee hamishwillee Jun 22, 2020

Choose a reason for hiding this comment

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

@DonLakeFlyer @julianoes The theory is that there will be metadata to get the values of these fields.

  1. Removing these before said metadata exists may well break something more seriously than just "I need to update my messages".
  2. There is a question about what should be in metadata and what should be in the message itself. The theory is that the message should have stuff that everyone will need - getting the metadata should be a choice. [So for camera, the common settings are in message, and the extended are in metadata]. Is this stuff something that we should not force users to implement the whole mavftp etc infrastructure to get.

I'm to some extent playing devils advocate, but for next time perhaps add our new fields as extensions. Then remove the old fields and make the new fields non-extensions when we have a complete story

@olliw42
Copy link
Contributor

olliw42 commented Jun 23, 2020

@hamishwillee thx for the FYI (I've seen it). IMHO this should win the gold medal for the greatest nonsense of the year. I can't believe it, what a nonsense. This could be achieved much easier by, e.g., simply reading the ftp dir without any need of all this cruft and overhead. It's sad to see a good idea turned into a terrible result. IMO.

@@ -6672,13 +6681,11 @@
<!-- This message is work-in-progress it can therefore change, and should NOT be used in stable production environments -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DonLakeFlyer If using MAV_CMD_REQUEST_MESSAGE, what param maps to the metadata type you are after?

@hamishwillee
Copy link
Collaborator

@olliw42 The authors obviously thought the "cruft" had purpose. This provides:

  • location of metadata
  • ability to query what metadata is supported
  • ability to query specific metadata
  • Ability to query whether file has been updated and needs to be re-queried.
  • Information about whether translations files exist and need to be re-queried.

Yes you could have a folder for component metadata, perhaps hard coded and allow the vehicle to "see what is there". Is that what you were envisaging?

Or to put it another way, if you want to educate, perhaps you should expand a bit on how your solution would allow us to do the same kinds of things (or why the kinds of things we want to do are unnecessary).

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.

None yet

3 participants