Switch aux_global_position to AuxGlobalPosition message type#180
Merged
Switch aux_global_position to AuxGlobalPosition message type#180
Conversation
haumarco
force-pushed
the
agp_interface
branch
from
3be6328 to
a130913
Compare
bkueng
reviewed
Feb 18, 2026
Comment on lines
+100
to
+101
| uint8_t _id; | ||
| uint8_t _source; |
Collaborator
There was a problem hiding this comment.
Suggested change
| uint8_t _id; | |
| uint8_t _source; | |
| const uint8_t _id; | |
| const uint8_t _source; |
| class GlobalPositionMeasurementInterface : public PositionMeasurementInterfaceBase { | ||
| public: | ||
| explicit GlobalPositionMeasurementInterface(rclcpp::Node& node, | ||
| explicit GlobalPositionMeasurementInterface(rclcpp::Node& node, uint8_t id = 1, |
Collaborator
There was a problem hiding this comment.
Can you add API docs for the extra args and describe how to set them?
Is the id not starting with 0?
And as a user I'd ask myself why do I have to set both source and id if they are just integers? Can't one be inferred from the other?
Contributor
Author
There was a problem hiding this comment.
GitBook PR
- ID is not starting at 0. Zero is the default value of unpopulated params for unused sensors
- The source type is propagated to downstream consumers in messages derived from this data. So mavlink etc want to know what the source type is. We decided against an ID "pattern" to store this knowledge in the ID. You can have multiple instances with the same source-type.
Contributor
Author
bkueng
reviewed
Feb 20, 2026
| /** | ||
| * @param id Unique identifier non-zero for this position source. PX4 uses this to demultiplex | ||
| * measurements from multiple external positioning sources on the same topic. | ||
| * @param source Source type of the position data, using AuxGlobalPosition::SOURCE_* |
Collaborator
There was a problem hiding this comment.
Would be good to change this to an enum then (defined here), which would make it easier to use.
enum class SourceType {
Vision = AuxGlobalPosition::SOURCE_VISION,
};
Contributor
Author
There was a problem hiding this comment.
The enum is defined in the AuxGlobalPosition message, do we really want to have duplicated enums?
Contributor
Author
There was a problem hiding this comment.
after discussion. yes it makes sense to use it
bkueng
approved these changes
Feb 20, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
GlobalPositionMeasurementInterfacefromVehicleGlobalPositionto the newAuxGlobalPositionmessage type for theaux_global_positiontopicidandsourceparameters to populate the new message fields. Both have defaults (id=1,source=SOURCE_VISION), so existing code compiles without changes but should be updated to set meaningful values.idfield to demultiplex measurements from multiple external positioning sources on the same topic