fix: GNSS ROS publisher never set NavSatFix.status.status#106
Conversation
Found via end-to-end MOLA + MVSIM simulation testing: the simulated GNSS observation was converted to sensor_msgs::NavSatFix by hand, field by field (latitude/longitude/altitude/covariance only), and never touched msg.status.status. ROS leaves that field at NavSatStatus::STATUS_UNKNOWN (-2) by IDL default -- not STATUS_FIX (0) as a naive zero-init might suggest. Consumers that treat anything other than a definite fix as invalid (e.g. mola_state_estimation_smoother's GNSS fusion, which checks CObservationGPS's GGA fix_quality field, itself derived from status.status by the MRPT ROS bridge) then silently reject *every* GPS reading -- even though the simulated fix quality (mrpt::obs::gnss::Message_NMEA_GGA::fields::fix_quality, set in Sensors/GNSS.cpp) was perfectly valid the whole time. Fixed by delegating to the existing, already-correct mrpt2ros::toROS(CObservationGPS, Header, NavSatFix&) bridge function, matching the pattern the adjacent IMU handler already used, instead of reimplementing the conversion. The GPS bridge header (<mrpt/ros1bridge/gps.h> / <mrpt/ros2bridge/gps.h>) wasn't even included before.
📝 WalkthroughWalkthroughThe GPS publishing path now uses the shared ChangesGNSS ROS bridge conversion
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Related PRs, found together while validating GNSS+IMU relocalization end-to-end in MOLA against a geo-referenced map: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mvsim_node_src/mvsim_node.cpp (1)
1296-1304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider trimming the inline comment to reference
agents.mdinstead of duplicating the full bug narrative.The 9-line comment block duplicates the documentation already added to
agents.md(lines 129–146). A brief comment pointing to the docs would reduce inline noise and maintenance burden.📝 Suggested comment trim
- // Convert observation MRPT -> ROS. Delegate to the library conversion - // (as the IMU handler above already does) instead of reimplementing it - // field-by-field: the hand-rolled version here never set - // msg->status.status, which ROS leaves at NavSatStatus::STATUS_UNKNOWN - // (-2) by IDL default; consumers that treat anything other than a - // definite fix as invalid (e.g. mola_state_estimation_smoother's GNSS - // fusion) would then silently reject every single reading, even - // though the simulated fix quality (mrpt::obs::gnss::Message_NMEA_GGA - // ::fields::fix_quality) was perfectly valid. + // Delegate MRPT -> ROS conversion to the shared bridge (as the IMU + // handler does). See agents.md "GNSS" section for why this is needed + // (status.status was previously left at STATUS_UNKNOWN).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mvsim_node_src/mvsim_node.cpp` around lines 1296 - 1304, Trim the verbose inline comment above the MRPT-to-ROS observation conversion to a brief note referencing agents.md for the rationale, while retaining enough context to identify that the library conversion must be used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mvsim_node_src/mvsim_node.cpp`:
- Around line 1309-1310: Check the boolean result of mrpt2ros::toROS(obs,
msg_header, *msg) in the GPS observation handling code before publishing the
message; on false, handle the conversion failure appropriately and return or
skip publishing the malformed observation.
---
Nitpick comments:
In `@mvsim_node_src/mvsim_node.cpp`:
- Around line 1296-1304: Trim the verbose inline comment above the MRPT-to-ROS
observation conversion to a brief note referencing agents.md for the rationale,
while retaining enough context to identify that the library conversion must be
used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf4e18b1-9dd1-430b-b961-4d35a9053afc
📒 Files selected for processing (2)
agents.mdmvsim_node_src/mvsim_node.cpp
| auto msg = mvsim_node::make_shared<Msg_GPS>(); | ||
| mrpt2ros::toROS(obs, msg_header, *msg); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the return type of mrpt2ros::toROS for GPS observations
# and check how the IMU handler calls the equivalent bridge function.
# Find the GPS bridge header and inspect the toROS declaration
fd -i 'gps.h' | xargs rg -n 'toROS.*GPS\|toROS.*NavSatFix\|toROS.*CObservationGPS' 2>/dev/null | head -20
# Alternatively search for the function signature in the mrpt ros bridge
fd -i 'gps' -e h -e hpp | xargs rg -n 'toROS' 2>/dev/null | head -20
# Check how the IMU handler calls toROS (for pattern consistency)
rg -n -A2 'toROS.*imu\|toROS.*IMU\|toROS.*Imu' mvsim_node_src/ 2>/dev/null | head -20Repository: MRPT/mvsim
Length of output: 2003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the mrpt2ros bridge declarations/definitions for GPS and related overloads.
rg -n --hidden --no-ignore-vcs 'namespace mrpt2ros|toROS\s*\(' . \
-g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**' | head -300
# Narrow to likely bridge files if present.
fd -i 'mrpt2ros' . -t f
fd -i 'ros' . -t f | head -200Repository: MRPT/mvsim
Length of output: 2422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the guidance around the GNSS bridge usage.
cat -n agents.md | sed -n '120,155p'
# Check whether this repo contains any mrpt ros bridge headers or generated docs.
fd -HI 'ros1bridge' .
fd -HI 'ros2bridge' .
fd -HI 'NavSatFix' .Repository: MRPT/mvsim
Length of output: 2090
🌐 Web query:
MRPT mrpt::ros1bridge toROS CObservationGPS NavSatFix return type
💡 Result:
The function mrpt::ros1bridge::toROS, used to convert an mrpt::obs::CObservationGPS object into a sensor_msgs::NavSatFix message, has a return type of bool [1][2]. The function returns true upon a successful conversion and false if an error occurs [1]. The full function signature is: bool mrpt::ros1bridge::toROS(const mrpt::obs::CObservationGPS& obj, const std_msgs::Header& msg_header, sensor_msgs::NavSatFix& msg); [1]
Citations:
- 1: https://docs.mrpt.org/reference/2.3.0-bis/group_mrpt_ros1bridge_grp.html
- 2: https://docs.mrpt.org/reference/latest/group_mrpt_ros1bridge_grp.html
🌐 Web query:
site:github.com MRPT ros1bridge toROS CObservationGPS NavSatFix bool
💡 Result:
The MRPT library provides bridging functionality to convert between its mrpt::obs::CObservationGPS data structure and ROS navigation message types (specifically sensor_msgs/NavSatFix and gps_msgs/msg/GPSFix) [1][2][3]. In the mrpt_ros_bridge (for ROS 2) and mrpt_bridge (for ROS 1), bidirectional conversion functions are provided [1][2][3]. Specifically, for sensor_msgs/NavSatFix integration: 1. Conversion Functions: The bridge typically uses functions named toROS() and fromROS() (or navSatFixToObservation()) to translate data [2][4]. 2. Boolean Flags: When performing bulk conversions—such as using rosbag2ToGPS—the bridge often utilizes a boolean flag (e.g., isGpsFix) to distinguish whether the target message type is sensor_msgs/NavSatFix or gps_msgs/msg/GPSFix [5]. 3. Data Mapping: mrpt::obs::CObservationGPS contains internal fields such as fix_type (using the GnssFixType enum) and gnss_service_mask (using GnssService) which are mapped to ROS message status fields to ensure compatibility [6][4]. Modern versions of the bridge (MRPT 2.15.11+) include version-conditional logic to handle these mappings accurately, maintaining compatibility with older ROS message definitions [4]. For implementation details, refer to the mrpt::ros2bridge or mrpt_bridge headers, such as ros_to_mrpt_obs.h or gps.h, which define these conversion interfaces [5][2].
Citations:
- 1: https://github.com/MRPT/mrpt_ros_bridge/
- 2: Add support for gps_msgs types too mrpt_ros_bridge#2
- 3: https://github.com/mrpt-ros-pkg/mrpt_bridge/blob/master/CHANGELOG.rst
- 4: Support new MRPT 2.15.11 GPS data fields mrpt_ros_bridge#5
- 5: Support converting GpsFix messages too mrpt_ros_bridge#4
- 6: https://github.com/MRPT/mrpt/blob/master/libs/obs/include/mrpt/obs/CObservationGPS.h
🌐 Web query:
MRPT ros2bridge gps.h CObservationGPS toROS return type bool
💡 Result:
Yes, the return type of the toROS function in the mrpt::ros2bridge namespace, when converting a mrpt::obs::CObservationGPS object, is bool [1][2][3]. This function returns true upon a successful conversion and false if any error occurs during the process [1][2][4]. The bridge provides overloads for toROS to handle different target ROS message types, such as sensor_msgs::msg::NavSatFix and gps_msgs::msg::GPSFix [5][3][6].
Citations:
- 1: https://docs.mrpt.org/reference/2.14.15/group_mrpt_ros2bridge_grp.html
- 2: https://docs.mrpt.org/reference/2.11.8/group_mrpt_ros2bridge_grp.html
- 3: https://docs.ros.org/en/humble/p/mrpt_libros_bridge/generated/function_group__mrpt__ros2bridge__grp_1ga71803d466f1f15647e5b8f94b7b82b97.html
- 4: https://docs.mrpt.org/reference/2.3.0-bis/group_mrpt_ros1bridge_grp.html
- 5: https://docs.ros.org/en/humble/p/mrpt_libros_bridge/generated/namespace_mrpt__ros2bridge.html
- 6: https://docs.ros.org/en/humble/p/mrpt_libros_bridge/generated/file_include_mrpt_ros2bridge_gps.h.html
🌐 Web query:
site:docs.mrpt.org mrpt::ros2bridge::toROS CObservationGPS NavSatFix bool
💡 Result:
The function mrpt::ros2bridge::toROS used to convert an mrpt::obs::CObservationGPS object to a sensor_msgs::msg::NavSatFix ROS 2 message has the following signature [1]: bool mrpt::ros2bridge::toROS( const mrpt::obs::CObservationGPS& obj, const std_msgs::msg::Header& msg_header, sensor_msgs::msg::NavSatFix& msg); The function returns a boolean value indicating whether the conversion was successful [2][1]. Because the CObservationGPS class does not natively store ROS-specific header information, the user is required to provide an explicit std_msgs::msg::Header object, which is then copied into the resulting NavSatFix message [3][1]. Note: As of MRPT 3.0, the mrpt-ros2bridge library has been separated from the core MRPT repository and is maintained in its own repository at https://github.com/MRPT/mrpt_ros_bridge [4].
Citations:
- 1: https://docs.mrpt.org/reference/2.14.15/group_mrpt_ros2bridge_grp.html
- 2: https://docs.mrpt.org/reference/2.11.8/group_mrpt_ros2bridge_grp.html
- 3: https://docs.mrpt.org/reference/2.3.0-bis/group_mrpt_ros1bridge_grp.html
- 4: https://docs.mrpt.org/reference/latest/group_mrpt_ros2bridge_grp.html
🌐 Web query:
site:github.com/MRPT/mrpt_ros_bridge gps.h toROS CObservationGPS NavSatFix
💡 Result:
In the context of the MRPT (Mobile Robot Programming Toolkit) ROS bridge, conversion between MRPT's CObservationGPS and ROS's NavSatFix messages is handled through specific utility functions defined within the mrpt_ros_bridge package [1][2]. Header and Implementation Conversion logic is typically found in the header file mrpt/ros2bridge/gps.h and implemented in the corresponding source file mrpt/ros2bridge/src/gps.cpp [1]. Key Functionality The bridge provides bidirectional conversion functions: 1. toROS: Converts an mrpt::obs::CObservationGPS object into a sensor_msgs::msg::NavSatFix message [1]. 2. fromROS: Converts a sensor_msgs::msg::NavSatFix message into an mrpt::obs::CObservationGPS observation [1]. These functions manage the mapping between NMEA-based data (GGA, RMC, VTG, GSA) within MRPT and the standard ROS NavSatFix fields, including timestamp synchronization, coordinate mapping, and status/covariance propagation [1][3]. Recent updates to the bridge have added support for more advanced GPS messages (such as gps_msgs::msg::GPSFix) and version-conditional logic to support newer MRPT GNSS fix types while maintaining backward compatibility [1][3][4]. Usage Pattern While specific headers may vary by version, the standard pattern for using these bridge functions involves including the appropriate header and passing the source object to the conversion utility, similar to other MRPT-ROS type conversions [2]: // Conceptual example #include <mrpt/ros2bridge/gps.h> #include <sensor_msgs/msg/nav_sat_fix.hpp> #include <mrpt/obs/CObservationGPS.h> // MRPT to ROS mrpt::obs::CObservationGPS mrpt_obs; sensor_msgs::msg::NavSatFix ros_msg; mrpt::ros2bridge::toROS(mrpt_obs, ros_msg);
Citations:
- 1: Add support for gps_msgs types too mrpt_ros_bridge#2
- 2: https://github.com/MRPT/mrpt_ros_bridge/
- 3: Support new MRPT 2.15.11 GPS data fields mrpt_ros_bridge#5
- 4: Support converting GpsFix messages too mrpt_ros_bridge#4
Check mrpt2ros::toROS(...) result here
It returns false on conversion errors, so ignoring it can silently drop malformed GPS observations. Handle the failure path before publishing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mvsim_node_src/mvsim_node.cpp` around lines 1309 - 1310, Check the boolean
result of mrpt2ros::toROS(obs, msg_header, *msg) in the GPS observation handling
code before publishing the message; on false, handle the conversion failure
appropriately and return or skip publishing the malformed observation.
jlblancoc
force-pushed
the
fix/gnss-navsatfix-status-field
branch
from
5f9f0a9 to
fdd2df3
Compare
1 participant
Summary
Found via end-to-end MOLA + MVSIM simulation testing of GNSS-based robot localization: the simulated GNSS observation is converted to
sensor_msgs::NavSatFixby hand, field by field (latitude/longitude/altitude/covariance only), and never touchesmsg.status.status. ROS leaves that field atNavSatStatus::STATUS_UNKNOWN(-2) by IDL default — notSTATUS_FIX(0) as a naive zero-init might suggest.Any consumer that treats anything other than a definite fix as invalid (e.g.
mola_state_estimation_smoother's GNSS fusion) silently rejects every single simulated GNSS reading, even though mvsim's own internal fix quality (Message_NMEA_GGA::fields::fix_quality, set inSensors/GNSS.cpp) was perfectly valid the whole time. This makes any ROS 2 consumer relying on GNSS status filtering unable to use mvsim's simulated GNSS at all, silently (no error, no warning — the messages publish fine, just with an always-invalid status).Fix
Delegates to the existing, already-correct
mrpt2ros::toROS(CObservationGPS, Header, NavSatFix&)bridge function — matching the pattern the adjacent IMU handler already used — instead of reimplementing the conversion by hand. The GPS bridge header (<mrpt/ros1bridge/gps.h>/<mrpt/ros2bridge/gps.h>) wasn't even included before.Test plan
colcon test --packages-select mvsim: 13/13 passedros2 topic echo/DEBUG logs that GNSS readings are now accepted by a real ROS 2 consumer (mola_state_estimation_smoother'sfuse_gnss()), where every single reading was previously silently rejectedSummary by CodeRabbit