provide message validation check API

[GoesM]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#4177
Primary OS tested on	Ubuntu
Robotic platform tested on	gazebo
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

• Add message-validation check in nav2_util pkg
• Add check in nav2_amcl for map message to avoid heap-buffer-overflow bug mentioned in Validate messages in subscription callbacks for critical error in population #4177

Description of documentation updates required from your changes

• new file named validate_message.hpp, which provide a structure of message-validation-check in nav2_util
• add a validation check for map message of nav2_amcl

---

Future work that may be required in bullet points

For Maintainers:

• Check that any new parameters added are updated in navigation.ros.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

Looks good for a first go! I think it has some linting issues and needs to add unit tests for each of the functions, but overall with a few trivial changes this looks like a good starting point!

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[GoesM]

I've updated some conditions in validateMsg(), but I've encountered a few details that need addressing. I'll take some time to figure out how to resolve them. However, I'll go ahead and update my current code to share my progress here~ ^_^

[SteveMacenski]

Great, thanks! I'm giving it a once over but only thing that is clearly missing is just the unit tests / linting so that it meets our quality specs for Nav2 contributions! Thanks for this!

After this is done, I think this would make a really nice, short, confined undergraduate / jr graduate student project to finish this up with other messages and use it across the stack. It would let them think about some of the important needs for production systems and be able to touch alot of places in Nav2 to learn and grow. If you had any students in mind, let me know! Else, I'm sure I can find one.

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[GoesM]

About the time-stamp of map message

current code for updating the time-stamp

https://github.com/ros-planning/navigation2/blob/0be2f25782413677fedc58ea090c6ecfa8b9a6b8/nav2_map_server/src/map_server/map_server.cpp#L243-L248

in this way, we would NOT update the stamp successfully.

Though I'm not very clear about the reson , but we can prove it by following inserted code:

//goes insert
  updateMsgHeader();
  std::cout <<
    "[GOES|DEBUG] [map_io]: msg.header.stamp.sec: " << msg_.header.stamp.sec << std::endl
        << "msg_.header.stamp.nanosec" << msg_.header.stamp.nanosec << std::endl; 
  std::cout <<
    "[GOES|DEBUG] [map_io]: msg_.info.map_load_time.sec : " << msg_.info.map_load_time.sec << std::endl
        << "msg_.header.msg_.info.map_load_time.nanosec" << msg_.info.map_load_time.nanosec << std::endl; 
// insert end

then , we could catch the log as following:

[map_server-6] [GOES|DEBUG] [map_io]: msg.header.stamp.sec: 0
[map_server-6] msg_.header.stamp.nanosec0
[map_server-6] [GOES|DEBUG] [map_io]: msg_.info.map_load_time.sec : 0
[map_server-6] msg_.header.msg_.info.map_load_time.nanosec0

suggested way to update the time-stamp

I've tried to update the stamp in another way, like following:

void MapServer::updateMsgHeader()
{
  rclcpp::Clock clock(RCL_SYSTEM_TIME);
  msg_.info.map_load_time = clock.now();
  msg_.header.frame_id = frame_id_;
  msg_.header.stamp = clock.now();
}

then , we could catch the log as following, which shows a successful result.

[map_server-6] [GOES|DEBUG] [map_io]: msg.header.stamp.sec: 1712391915
[map_server-6] msg_.header.stamp.nanosec28685368
[map_server-6] [GOES|DEBUG] [map_io]: msg_.info.map_load_time.sec : 1712391915
[map_server-6] msg_.header.msg_.info.map_load_time.nanosec28684872

In addition, the message received by ros2 topic echo would have a correct time-stamp:

header:
  stamp:
    sec: 1712393583
    nanosec: 750836020
  frame_id: map
info:
  map_load_time:
    sec: 1712393583
    nanosec: 750835026
  resolution: 0.05000000074505806

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

That's not a solution unfortunately since we don't always run in ROS (i.e. simulation) time. The clock of the node should be set to the right time source. Does the timestamp not get populated from the service call as well or just on the on_configure first go? That is really odd

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[GoesM]

I fix some typo this time.

the reason why time-stamp of message from map-server is (zero,zero)

After some experiments, it seems clear for me that updateMsgHeader as following would bring non-zero time ONLY after on_activate() finished.
Because the node->now() would not start its time until it's activated.

void MapServer::updateMsgHeader() 
 { 
   msg_.info.map_load_time = now(); 
   msg_.header.frame_id = frame_id_; 
   msg_.header.stamp = now(); 
 } 

SO, here's still the trouble that :

map_server only send map-message once during on_activate() but NOT after activated, so that the time-stamp would be zero and rejected by our validation_message check, then nav2_amcl would not start its work.
(how map_server send map-message shown as following:

https://github.com/ros-planning/navigation2/blob/677e6accb3aea9380678da7d623b3387578d3549/nav2_map_server/src/map_server/map_server.cpp#L128-L144

Here's some different ways I come up with to solve the trouble:

• send the map-message first time after on_activate() (for example let nav2_amcl call the service "/nav2_map_server/load_map" provided by nav2_map_server)
• remove validation check for zero-time-stamp
• using the following way to update header:

void MapServer::updateMsgHeader()
{
  rclcpp::Clock clock(RCL_SYSTEM_TIME);
  msg_.info.map_load_time = clock.now();
  msg_.header.frame_id = frame_id_;
  msg_.header.stamp = clock.now();
}

I'm not very sure if there are any better methods available, or which one among these methods is most suitable. I should primarily rely on your advice regarding which direction to proceed with modifications. : )

[SteveMacenski]

remove validation check for zero-time-stamp

Lets do that.

Also please pull in main or rebase so CI can run. you also have a couple linting problems that the Actions job identified. After that, this should be good to merge!

[SteveMacenski]

No need for the comments -- checking the normalization is the same as any distance calculation sqrt(componentA*componentA + componentB*componentA + ...)

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[GoesM]

Oops, I noticed that the CI test failed due to the spin_behavior_test, which doesn't seem to be caused by the validation_message.

[mergify]

This pull request is in conflict. Could you fix it @GoesM?

[SteveMacenski]

Dont worry about the spin test - its flaky. You do though have a conflict you need to resolve before we can merge & I see that there's a collision checker test that failed, that is not known to be flaky but I restarted CI to see if it comes back. If so, then you should look into that.

Also, your pull / rebase did not work correctly, you have all of the commits from the work where this should only contain your commits by rebasing the branch on main or pulling in changes. Please fix this so that we only have your contributions in this PR 😄

Edit: It looks like that test was also flaky, you may ignore.

[codecov]

Codecov Report

Attention: Patch coverage is 80.96886% with 110 lines in your changes are missing coverage. Please review.

Project coverage is 89.64%. Comparing base (d509fbf) to head (9517883).
Report is 44 commits behind head on main.

❗ Current head 9517883 differs from pull request most recent head 8322fe2. Consider uploading reports for the commit 8322fe2 to get more accurate results

Files	Patch %	Lines
..._mppi_controller/src/critics/constraint_critic.cpp	48.83%	22 Missing ⚠️
...ontroller/src/critics/velocity_deadband_critic.cpp	52.27%	21 Missing ⚠️
...ntroller/src/regulated_pure_pursuit_controller.cpp	25.00%	12 Missing ⚠️
nav2_mppi_controller/src/critics/cost_critic.cpp	84.21%	6 Missing ⚠️
nav2_mppi_controller/src/critics/goal_critic.cpp	60.00%	4 Missing ⚠️
...2_mppi_controller/src/critics/obstacles_critic.cpp	71.42%	4 Missing ⚠️
nav2_smac_planner/src/smac_planner_hybrid.cpp	60.00%	4 Missing ⚠️
nav2_util/src/base_footprint_publisher.cpp	0.00%	4 Missing ⚠️
...oller/include/nav2_mppi_controller/tools/utils.hpp	90.32%	3 Missing ⚠️
..._mppi_controller/src/critics/goal_angle_critic.cpp	62.50%	3 Missing ⚠️
... and 16 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4206      +/-   ##
==========================================
- Coverage   90.01%   89.64%   -0.37%     
==========================================
  Files         432      435       +3     
  Lines       19406    19738     +332     
==========================================
+ Hits        17469    17695     +226     
- Misses       1937     2043     +106     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mergify]

@GoesM, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[GoesM]

I think my rebase did not work correctly and I believe it's more wise for me to open another PR #4276