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.

Sign up for GitHub

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

Jump to bottom

component_information_basics: add serial number #1788

Merged
merged 7 commits into from
Dec 7, 2022
Merged

component_information_basics: add serial number #1788

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c09f9cc
component_information_basics: add serial number
julianoes Feb 1, 2022
127558e
development: addressed review comments
julianoes Apr 13, 2022
d42d04f
Apply suggestions from code review
hamishwillee Apr 14, 2022
041a350
Update message_definitions/v1.0/development.xml
hamishwillee Apr 14, 2022
2fe5513
Update message_definitions/v1.0/development.xml
hamishwillee Dec 7, 2022
2ee011d
Update message_definitions/v1.0/development.xml
hamishwillee Dec 7, 2022
5a7c54f
Merge branch 'master' into pr-serial-number
hamishwillee Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions message_definitions/v1.0/development.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,13 +424,14 @@
<field type="uint32_t" name="status_flags" display="bitmask" enum="MAV_BATTERY_STATUS_FLAGS">Fault, health, readiness, and other status indications.</field>
</message>
<message id="396" name="COMPONENT_INFORMATION_BASIC">
<description>Basic component information data.</description>
<description>Basic component information data. Should be requested using MAV_CMD_REQUEST_MESSAGE on startup, or when required.</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>
<field type="uint8_t[32]" name="model_name">Name of the component model</field>
<field type="char[24]" name="software_version">Software version. The version format can be custom, recommended is SEMVER 'major.minor.patch'.</field>
<field type="char[24]" name="hardware_version">Hardware version. The version format can be custom, recommended is SEMVER 'major.minor.patch'.</field>
<field type="uint64_t" name="capabilities" enum="MAV_PROTOCOL_CAPABILITY" display="bitmask">Component capability flags</field>
<field type="char[32]" name="vendor_name">Name of the component vendor. Needs to be zero terminated. The field is optional and can be empty/all zeros.</field>
<field type="char[32]" name="model_name">Name of the component model. Needs to be zero terminated. The field is optional and can be empty/all zeros.</field>
<field type="char[24]" name="software_version">Software version. The recommended format is SEMVER: 'major.minor.patch' (any format may be used). The field must be zero terminated if it has a value. The field is optional and can be empty/all zeros.</field>
<field type="char[24]" name="hardware_version">Hardware version. The recommended format is SEMVER: 'major.minor.patch' (any format may be used). The field must be zero terminated if it has a value. The field is optional and can be empty/all zeros.</field>
Comment on lines +432 to +433
Copy link
Contributor

@olliw42 olliw42 Nov 23, 2022 •
edited by hamishwillee
Loading

Choose a reason for hiding this comment

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

Suggested change
<field type="char[24]" name="software_version">Software version. The recommended format is SEMVER: 'major.minor.patch' (any format may be used). The field must be zero terminated if it has a value. The field is optional and can be empty/all zeros.</field>
<field type="char[24]" name="hardware_version">Hardware version. The recommended format is SEMVER: 'major.minor.patch' (any format may be used). The field must be zero terminated if it has a value. The field is optional and can be empty/all zeros.</field>
<field type="char[24]" name="software_version_str">Software version. The recommended format is SEMVER: 'major.minor.patch' (any format may be used). The field must be zero terminated if it has a value. The field is optional and can be empty/all zeros.</field>
<field type="char[24]" name="hardware_version_str">Hardware version. The recommended format is SEMVER: 'major.minor.patch' (any format may be used). The field must be zero terminated if it has a value. The field is optional and can be empty/all zeros.</field>
<field type="uint32_t" name="software_version">Version of the component software, encoded as: (Dev &amp; 0xff) &lt;&lt; 24 | (Patch &amp; 0xff) &lt;&lt; 16 | (Minor &amp; 0xff) &lt;&lt; 8 | (Major &amp; 0xff).</field>
<field type="uint32_t" name="hardware_version">Version of the component hardware, encoded as: (Dev &amp; 0xff) &lt;&lt; 24 | (Patch &amp; 0xff) &lt;&lt; 16 | (Minor &amp; 0xff) &lt;&lt; 8 | (Major &amp; 0xff).</field>

I am really getting somewhat annoyed with the many different types of versions in the different mavlink messages, and it is not always clear how they relate to each other. Here is now introduced yet another approach to provide a version. I very much like the str version, as the product specific versioning can be used, which should help users and vendors. On the other hand, the numeric version is already there for some components, and also may have advantages for more automated procedures. The suggested addition serves two purposes

  • it shall help clarifying which variations of versions are available, i.e. are intended to be provided by typical mavlink components.
  • it helps clarifying what the relation of the different variations of version is. One would not have to start a research program to figure out how the vendor str relates to the x.x.x.x version number.

I think it just helps taking confusion and ambiguity out of the game.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to send information once, if at all possible.

How should the information used? My thinking is that 99.9% of the time it is simply displayed, and sending it in the string format makes the data easy to consume without having to do any formatting. In that case you don't need to really force the format, and it allows the sender to do any formatting of the version they like.

  • Are there cases where you really need to know the version to use for something other than display?
  • Can we get away with forcing semantic versioning on everyone?

If the answer is no, I think we're over thinking this.

Copy link
Contributor

@olliw42 olliw42 Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I hear you, and to be clear I vote much more strongly for custom_name than for this here.

I just note that "version" comes in quite some flavors, and that the number of flavors is being increased yet again. I find it confusing, and I suspect that I may not be the only one. As said, I too like the string flavor, as it may give the number which the customer needs to speak to the vendor. Yet, fact is, these other numbers are around.

Are there cases where you really need to know the version to use for something other than display?

I think that we may judge incorrectly, since humans sometimes can get creative and do unexpected things, for the good. Yes, I too would think that they would be for displaying. But do we know that it may not be used in some smarter product by some smarter on-board computer to check all firmwares and provide the customer a convennient upgrade for all the pieces on the vehicle, kind of 3DR Solo like?

Can we get away with forcing semantic versioning on everyone?

This would very much take away the benefit of str versions, and I would not understand why when not to go with the unit32 flavor in use already elsewhere.

I guess, if one wants to keep this here as is, I would suggest to at least add comments to the other messages with version info that the older fields are kind of deprecated and that this new message with the new version flavor should be used instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't want to make this decision. I am going to merged as-is to get this in and put a second PR in to address this separately.

<field type="char[32]" name="serial_number">Hardware serial number. The field must be zero terminated if it has a value. The field is optional and can be empty/all zeros.</field>
</message>
<message id="414" name="GROUP_START">
<description>Emitted during mission execution when control reaches MAV_CMD_GROUP_START.</description>
Expand Down