-
Notifications
You must be signed in to change notification settings - Fork 15k
EKF2: Multi-instance AGP fusion #26151
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
Conversation
Enable DDS bridge to handle multi-instance uORB topics by mapping multiple topic instances to a single DDS topic with instance field.
🔎 FLASH Analysispx4_fmu-v5x [Total VM Diff: 2616 byte (0.13 %)]px4_fmu-v6x [Total VM Diff: 2680 byte (0.14 %)]Updated: 2025-12-19T14:14:27 |
f939832 to
c42be08
Compare
Add dedicated message type for auxiliary global position sources. Separates aux_global_position from VehicleGlobalPosition topic.
Add per-instance parameters (ID, CTRL, MODE, DELAY, NOISE, GATE) for up to 4 auxiliary global position sources.
- Support up to 4 independent AGP sources simultaneously - Dynamic parameter lookup without boilerplate switch statements - Per-source state tracking and fusion logic - Update simulator to use new AuxGlobalPosition message
c42be08 to
8542261
Compare
AGP altitude is expected in AMSL but sensor_agp_sim fills it with above ellipsoid data |
beniaminopozzan
left a comment
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.
Thanks @haumarco !
the uxrce_dds_client change alone is very much welcome (it could even be a standalone PR)!
Could you also update the documentation please? 🙏🏻
Thanks!
| # Auxiliary global position | ||
|
|
||
| # This message provides global position data from an external source such as | ||
| # radio-triangulation, viusal navigation, or other positioning system. |
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.
| # radio-triangulation, viusal navigation, or other positioning system. | |
| # radio-triangulation, visual navigation, or other positioning systems. |
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.
thx
|
|
||
| float64 lat # [deg] Latitude in WGS84 | ||
| float64 lon # [deg] Longitude in WGS84 | ||
| float32 alt # [m] Altitude above mean sea level (AMSL) |
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.
Is there a specific reason for using AMSL instead of ellipsoid altitude?
From the ROS 2 PoV, the NavSatFix message type would convert to AuxGlobalPosition in an easier way if the AGP altitude were w.r.t. WGS84
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.
in EKF2 we use AMSL, so to keep consistency. we can discuss this, but it's not in the scope of this PR.
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 though WGS84 -> AMSL conversion would have been trivial inside the EKF, but actually
PX4-Autopilot/src/modules/ekf2/EKF2.hpp
Line 214 in 4757158
| float altEllipsoidToAmsl(float ellipsoid_alt) const; |
| float64 lon # [deg] Longitude in WGS84 | ||
| float32 alt # [m] Altitude above mean sea level (AMSL) | ||
|
|
||
| float32 eph # [m] Standard deviation of horizontal position error |
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.
Not striclty related to this PR, but wouldn't splitting eph into North and East be more useful?
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.
yes. that's something on our list.
| @[ for idx, sub in enumerate(subs)]@ | ||
| {ORB_ID(@(topic_simple))}@('' if idx == len(subs)-1 else ',') | ||
|
|
||
| @[ end for]@ |
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.
| @[ for idx, sub in enumerate(subs)]@ | |
| {ORB_ID(@(topic_simple))}@('' if idx == len(subs)-1 else ',') | |
| @[ end for]@ | |
| @[ for idx, sub in enumerate(subs)]@ | |
| {ORB_ID(@(topic_simple))}@('' if idx == len(subs)-1 else ',') | |
| @[ end for]@ |
| ekf.resetAidSourceStatusZeroInnovation(aid_src); | ||
| reset = true; | ||
| } | ||
| if (!(getCtrlParam(slot) & static_cast<int32_t>(Ctrl::kHPos))) { |
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.
with all get***Param functions calling snprintf at some point, will this add not-negligible delays?
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 dont think using snprintf adds relevant delay. i now also improved the handling of the parameter caching without overloading _params...
| _state = State::kStopped; | ||
| } | ||
| } else { | ||
| ekf.disableControlStatusAuxGpos(); |
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.
shouldn't this (_control_status.flags.aux_gpos) be slot-specific too?
one slot could fail and stop aiding but the others could still be healthy.
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.
yes. I'll disable the cs-flag as soon as all sources are stopped. thanks
| } | ||
|
|
||
| float test_ratio_filtered() const { return _test_ratio_filtered; } | ||
| float test_ratio_filtered(uint8_t id = 0) const |
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 rest of the EKF2 module only ever uses id=0 (like in
PX4-Autopilot/src/modules/ekf2/EKF/ekf_helper.cpp
Lines 544 to 545 in 8542261
| test_ratio = math::max(test_ratio, fabsf(_aux_global_position.test_ratio_filtered())); | |
| } |
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.
you are right!
bkueng
left a comment
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 just went through the DDS part and it looks good, nice work.
| # This message provides global position data from an external source such as | ||
| # radio-triangulation, viusal navigation, or other positioning system. | ||
|
|
||
| uint32 MESSAGE_VERSION = 0 |
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.
If you want to version it, it should be under msg/versioned/
Solved Problem
EKF2 could only fuse a single Auxiliary Global Position (AGP) source, limiting the ability to use multiple external positioning systems simultaneously, e.g. Visual-Map-Matching-Navigation, Radio-Triangulaiton.
Solution
This PR enables EKF2 to fuse up to 4 independent AGP sources simultaneously by implementing multi-instance support throughout the entire stack:
New uORB message type (
AuxGlobalPosition)VehicleGlobalPositionfor cleaner architectureidfield to identify the sourceMulti-instance DDS bridge support
/fmu/in/aux_global_position_A/B/C/Dtoaux_global_positionuORB topicPer-instance EKF2 parameters (EKF2_AGP0-3_*)
EKF2_AGP{N}_ID: Sensor ID mapping (parameter slot → message ID).EKF2_AGP{N}_CTRLetc...Independent fusion state machines
idfield routes to parameter slotChangelog Entry
For release notes:
Test coverage
Tested both with ROS topics:
/fmu/in/aux_global_position_A,/fmu/in/aux_global_position_Band also as used with one source:
/fmu/in/aux_global_positionAdditional Notes
Extension to of multi-sensor capability Optical Flow or also GNSS is planned. I think I can generalize quite some things to avoid having duplicate code.