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 2 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 @@ -271,13 +271,14 @@
<field type="uint8_t" name="security" enum="WIFI_NETWORK_SECURITY">WiFi network security type.</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">Sofware 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.</field>
hamishwillee marked this conversation as resolved.
Show resolved Hide resolved
<field type="char[32]" name="model_name">Name of the component model. Needs to be zero terminated.</field>
hamishwillee marked this conversation as resolved.
Show resolved Hide resolved
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[32]" name="model_name">Name of the component model. Needs to be zero terminated.</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[32]" name="custom_name">Custom name of the component given to it by the user. Needs to be zero terminated. The field is optional and can be empty/all zeros.</field>

I suggest to add a field custom_name. The motivation is the same as for it in e.g. GIMBAL_DEVICE_INFORMATION: Some components may allow users to set a name for that particular component, which can be very handy. For instance, several components of the same brand/model may be installed in a system, and the custom name can make it much easier for the user to recall which is which (otherwise she had to distinguish by comp ID)(e.g. dual gimbal, dual camera). As another example, a user may own multiple components of the same brand/model which can swapped e.g. by a quick release. Here they probably would be configured to all have the same comp ID, so a custom name would make distinguishing much easier too. So, I think it is just a useful thing (needless to say that STorM32 supports a custom name).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is probably convenient to want to name your components, though you might argue that this is a property of the whole vehicle, or something that could be set in a GCS based on some other unique property of the component.

We talked about the message in the dev call last night. Once concern that has come up several time is "where do you stop". There are millions of fields you might add to the message. Are they all equally important? Are they worth the effort? Will someone actually use any of these or are they "speculative".

The consensus was that for this message it doesn't matter so much. The message is provided only on request (not streamed). If it ends up being inefficient, then so be it.
But we still don't want to be adding spurious fields that aren't used, making it more complicated for component implementers to understand.

So are you really planning to use this, and provide an implementation through to GCS. IF not, then lets hold off.

PS If we choose not to include this and your other suggestion then we can merge this PR into development

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.

@hamishwillee
I have much sympathy with carefully thinking about "where do you stop" (and I made/make that argument "stop" with passion in other cases like comp info or mavftp LOL). So, I agree that one should carefully balance.

The consensus was that for this message it doesn't matter so much. The message is provided only on request (not streamed). If it ends up being inefficient, then so be it.

my thought too, but I also agree that this shouldn't be argument to add all sort of spurious stuff

So are you really planning to use this, and provide an implementation through to GCS. IF not, then lets hold off.

I do know that some STorM32 users use its ability to give specific names to their builds, do have dual gimbal builds, and do swap. I do expose this custom name via the custom_name field in GIMBAL_DEVICE_INFORMATION. I do expose it in "my" MAVLink4OpenTX "GCS". What I cannot tell is if anyone is actually using it ... I usually get info from users only when something does not work :)

If your question is if I would add code to QGC, when: I think currently there would be not much to add, but what I can say for sure is that when QGC has support and e.g. indeed shows name and vendor somewhere in its pages I would love to see also a custom name beeing displayed ...

<field type="char[24]" name="software_version">Sofware version. The version format can be custom, recommended is SEMVER 'major.minor.patch'. This field needs to be zero terminated and is optional and can therefore be empty/all zero.</field>
hamishwillee marked this conversation as resolved.
Show resolved Hide resolved
<field type="char[24]" name="hardware_version">Hardware version. The version format can be custom, recommended is SEMVER 'major.minor.patch'. Needs to be zero terminated. This field is optional and can be empty.</field>
hamishwillee marked this conversation as resolved.
Show resolved Hide resolved
<field type="char[32]" name="serial_number">Hardware serial number. Needs to be zero terminated. This field is optional and can be empty.</field>
hamishwillee marked this conversation as resolved.
Show resolved Hide resolved
</message>
<message id="414" name="GROUP_START">
<description>Emitted during mission execution when control reaches MAV_CMD_GROUP_START.</description>
Expand Down