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_basics: add serial number #1788

Merged
merged 7 commits into from
Dec 7, 2022

Conversation

julianoes
Copy link
Collaborator

This enables a use case where the serial number of a component is required, in our case it's a winch.

@julianoes
Copy link
Collaborator Author

I think this is worthwhile, will self merge if there is no strong opposition.

@hamishwillee
Copy link
Collaborator

I have no strong opposition. I added a few general comments above about the whole message that it would be good to document/resolve.

If possible would definitely not do as an extension - should not be needed in development.xml right?

@julianoes
Copy link
Collaborator Author

I'll have a look tomorrow, thanks @hamishwillee.

@julianoes
Copy link
Collaborator Author

@hamishwillee I changed this according to your comments. Let me know if this is ok now.

@hamishwillee
Copy link
Collaborator

Thanks @julianoes I applied some consistency changes. I assume the other two fields must be supplied in all cases?

Otherwise I think this is good and would be happy to merge for prototyping.

@olliw42
Copy link
Contributor

olliw42 commented Nov 17, 2022

@hamishwillee @julianoes
it seems this had been approved. Any reason it's not yet merged?

I'd like to suggest the addition of a custom_name field, and see two ways to do that: either to wait for this PR to be merged and raise a PR with my suggestion on top of it or to raise a PR which includes the changes here together with my suggestion.

:)

@hamishwillee
Copy link
Collaborator

@olliw42 We got distracted. @julianoes I added a comment on invalid values, but otherwise this OK by me.

I'd like to suggest the addition of a custom_name field, and see two ways to do that: either to wait for this PR to be merged and raise a PR with my suggestion on top of it or to raise a PR which includes the changes here together with my suggestion.

Or you could just add your suggested change as a github change "suggestion" in a comment. That way Julian can merge it if he agrees. If not can be proposed separately.

@olliw42
Copy link
Contributor

olliw42 commented Nov 23, 2022

Or you could just add your suggested change as a github change "suggestion" in a comment.

oh ... cool ... I first was confused but google helped :) ... didn't knew that ... many THX for the teaching :)
I will do.

<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>
<field type="char[32]" name="model_name">Name of the component model. Needs to be zero terminated.</field>
Copy link
Contributor

@olliw42 olliw42 Nov 23, 2022

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

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 ...

Comment on lines +279 to +280
<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>
Copy link
Contributor

@olliw42 olliw42 Nov 23, 2022

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

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.

@olliw42
Copy link
Contributor

olliw42 commented Dec 5, 2022

could we make progress on this?
:)

@hamishwillee
Copy link
Collaborator

could we make progress on this?

I work MAVLink/PX4 Weds and Thurs. So yes. Now :-)

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Approving this version as discussed in mav call. This is in development, so can still evolve.

@hamishwillee hamishwillee merged commit 0811208 into master Dec 7, 2022
@julianoes julianoes deleted the pr-serial-number branch December 7, 2022 08:28
TSC21 pushed a commit to Dronecode/air-iop-definitions that referenced this pull request Feb 28, 2023
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