-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
version: Expose PX4 git tag string #11676
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
fa2b1ae
version: Expose PX4 git tag string
sfalexrog 0788433
Update src/systemcmds/ver/ver.c
dagar 3a5e73d
logger: Save PX4 git tag to ver_git_tag field
sfalexrog e588f35
Update src/modules/logger/logger.cpp
dagar de39afd
Merge branch 'master' into WIP/expose_tag_str
dagar 91628c4
logger: Style fix
sfalexrog File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we need the tag, as we already have it through
px4_firmware_version()
, https://github.com/PX4/Firmware/pull/11676/files#diff-64b21add574f1f3f059c2abc18eae7d6R2121.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, the
px4_firmware_version()
does not, in fact, return a tag, but rather a version number. That's fine for mainline firmwares, but we have modified firmware builds with additional information in the tag (our tags look likev1.8.2-clever.5
). We'd like to see the whole tag to not guess which exact firmware we're using.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have custom version tags for this: https://dev.px4.io/en/setup/building_px4.html#firmware_version. Does that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting feature, but unfortunately I don't think it does. We use different firmwares for different models of drones, and it'd be more convenient to just use the full tag name (which contains the target drone name) instead of trying to differentiate by numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having different firmwares for different models does not sound great either. Why not distinguish by using different SYS_AUTOSTART id's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch name is already in the log, did you see that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually look through the log after the flight. Git tag string (and the branch name inside) is important before the flight, e.g. during the manual vehicle inspection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not give me a reason why the branch is not enough.
What I want to avoid here is to have redundant information in different formats in the log. That is very confusing for anyone looking at the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The possibility of getting tag and branch name BEFORE flight as a pre-flight check would be very useful because we often have to test different features (e. g. precision landing and different camera modules) on one prototype at a close periods of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which you do with
ver all
.