Refactoring: Splitting periodicUpdate method into smaller methods #761
Conversation
| @@ -199,14 +199,14 @@ class RosFilter : public rclcpp::Node | |||
| //! @param[out] message - The standard ROS odometry message to be filled | |||
| //! @return true if the filter is initialized, false otherwise | |||
| //! | |||
| bool getFilteredOdometryMessage(nav_msgs::msg::Odometry * message); | |||
| bool getFilteredOdometryMessage(nav_msgs::msg::Odometry & message); | |||
There was a problem hiding this comment.
I am struggling to find it, but at some point, there was definitely a document that suggested that in ROS 2, passing parameters as pointers was preferred. Was there a reason for this change? I'm not against it - I prefer reference parameters - but I am trying to determine if this was required for some reason, or you were just looking to be consistent.
There was a problem hiding this comment.
Raw pointers are more dangerous than references - pointer could be a NULL or contains wrong address, so it requires more guards in the method (e.g. https://pvs-studio.com/en/docs/warnings/v595/). So only reason is reference is safer because a compiler guards the argument.
I am struggling to find it, but at some point, there was definitely a document that suggested that in ROS
Unfortunately I didn't find that - but I found:
- https://design.ros2.org/articles/intraprocess_communications.html#number-of-message-copies - based on what message should be used in intraprocess communication, subscriber with shared pointer generates less copies of message instead of subscriber with unique pointer.
- https://github.com/ros2/rclcpp/blob/rolling/rclcpp/include/rclcpp/any_subscription_callback.hpp#L935 only callbacks with shared pointer generate less copies.
These links are not related with this comment but they could help with subscriptions in the future :)
Anyway I see, the best way is use std::optional instead of reference and returning bool. Could you review my next commit?
| //! getFilteredOdometryMessage. | ||
| //! @return true if data is corrected otherwise false. | ||
| //! | ||
| bool publishPositionWithOdometry(nav_msgs::msg::Odometry filtered_position); |
There was a problem hiding this comment.
If we're sticking with reference types, is there any reason this isn't const ref?
There was a problem hiding this comment.
Half year ago I'm was experimenting with std::move() and it was bad.
I have changed the argument to const ref.
| //! | ||
| void updateFilterWithMeasurements(const rclcpp::Time & time); | ||
|
|
||
| //! @brief publish world to base link transform and position |
There was a problem hiding this comment.
Would this go over 100 characters without the line break?
src/ros_filter.cpp
Outdated
| { | ||
| nav_msgs::msg::Odometry filtered_position; | ||
| if (getFilteredOdometryMessage(filtered_position)) { | ||
| corrected_data = publishPositionWithOdometry(std::move(filtered_position)); |
There was a problem hiding this comment.
If you make this method take a const ref, there's no need for the std::move. Not sure what that does in the context of a value parameter anyway.
src/ros_filter.cpp
Outdated
| } | ||
|
|
||
| if (frame_id != map_frame_id_) { | ||
| std::cerr << "Odometry message frame_id was " << |
There was a problem hiding this comment.
As long as you're here, you can replace any cerr statements with the actual ROS 2 logging macros, and uncomment/update the ROS 1 macros further down.
firemark
force-pushed
the
periodic-update-refactoring
branch
from
1622b3d to
3d8534a
Compare
…rn std:optional instead of bool
Because method
RosFilter::periodicUpdateis a big method, I cut into smaller methods.In some methods (
validateFilterOutput,getFilteredAccelMessageandgetFilteredOdometryMessage) I changed an argument type from pointer to reference, because a pointer could be dangerous.