Feature/time sync #2216
Feature/time sync #2216
Conversation
…-fhwa-stol/carma-platform into fix/assorted-fix-for-yield
| ros1_clock_sub_ = rclcpp::create_subscription<rosgraph_msgs::msg::Clock>(node_topics_, "/clock", 1, | ||
| std::bind(&WMListenerWorker::ros1ClockCallback, worker_.get(), std::placeholders::_1), ros1_clock_options); | ||
|
|
||
| sim_clock_sub_ = rclcpp::create_subscription<rosgraph_msgs::msg::Clock>(node_topics_, "/sim_clock", 1, | ||
| std::bind(&WMListenerWorker::simClockCallback, worker_.get(), std::placeholders::_1), sim_clock_options); | ||
|
|
There was a problem hiding this comment.
These should be lambdas instead of std::bind. Lambdas can completely replace std::bind and is the recommended implementation pattern. Consider:
ros1_clock_sub_ = rclcpp::create_subscription<rosgraph_msgs::msg::Clock>(node_topics_, "/clock", 1,
[this](const rosgraph_msgs::msg::Clock::SharedPtr msg) {
this->worker_.simClockCallback(msg);
},
ros1_clock_options);There was a problem hiding this comment.
This is an optional suggestion, but it would be good to remove std::bind usage throughout this file.
| void setRos1Clock(rclcpp::Time time_now); | ||
|
|
||
| /*! \brief Set simulation clock clock (only used in simulation runs) | ||
| */ | ||
| void setSimulationClock(rclcpp::Time time_now); |
There was a problem hiding this comment.
These can be made const references: const rclcpp::Time &.
| void CARMAWorldModel::setRos1Clock(rclcpp::Time time_now) | ||
| { | ||
| ros1_clock_ = time_now; | ||
| } | ||
|
|
||
| void CARMAWorldModel::setSimulationClock(rclcpp::Time time_now) |
There was a problem hiding this comment.
These can be made const references: const rclcpp::Time &
| // NOTE: In simulation, ROS1 clock (often coming from CARLA) can have a large time ahead. | ||
| // the timing calculated here is in Simulation time, which is behind. Therefore, the world model adds the offset to make it meaningful to carma-platform: | ||
| // https://github.com/usdot-fhwa-stol/carma-platform/issues/2217 | ||
| if (is_simulation && ros1_clock_.has_value() && simulation_clock_.has_value()) |
There was a problem hiding this comment.
std::optional is implicitly convertible to bool, so we don't need the .has_value() calls. Being explicit in this context also seems to go against the ISO C++ code guidelines on verbosity (see here). Consider replacing with
if (is_simulation && ros1_clock_ && simulation_clock_)| auto ros1_clock_group = node_base_->create_callback_group(rclcpp::CallbackGroupType::MutuallyExclusive); | ||
| ros1_clock_options.callback_group = ros1_clock_group; | ||
|
|
||
| auto sim_clock_group = node_base_->create_callback_group(rclcpp::CallbackGroupType::MutuallyExclusive); | ||
| sim_clock_options.callback_group = sim_clock_group; |
There was a problem hiding this comment.
Consider assigning the create_callback_group to avoid having to copy the std::shared_ptr and an extra variable:
ros1_clock_options.callback_group = node_base_->create_callback_group(rclcpp::CallbackGroupType::MutuallyExclusive);
PR Details
Description
This PR supports usdot-fhwa-stol/carma-ns3-adapter#11
During integration testing of VRU, it was discovered that the CARLA's time which we call "game time" is not synced with the "simulation time" which is used and synced between sumo and mosaic.
CARLA's game time goes up until the sumo is hit start, which is when the all timelins are synced correctly, but the game time still has the initial offset from when CARLA started and up until sumo was hit started.
Upon trying to fix this issue, the ns3-adapter (which should be renamed to cdasim-adapter due to its new functionality) was identified as the potential source to broadcast the /clock for ROS network to provide the correct simulation time. However, doing so makes the carma-platform not behave the way it should and debugging could be non-trivial:
TODO: github link
Therefore, the only place the carma-platform has a time difference with the cdasim is when it receives SPAT message. Therefore, this specific logic is modified to adjust for that time difference.
Related GitHub Issue
Fixes this exact issue in this PR: #2230
Related Jira Key
CDAR-602
Motivation and Context
Integration testing of VRU
How Has This Been Tested?
cdasim integration tested on the simulation PC1
Types of changes
Checklist: