Skip to content

use NodeParameterInterface instead of /parameter_event to update "use…#2378

Merged
Yadunund merged 3 commits intorollingfrom
fujitatomoya/rclcpp-issues-2370
Apr 11, 2025
Merged

use NodeParameterInterface instead of /parameter_event to update "use…#2378
Yadunund merged 3 commits intorollingfrom
fujitatomoya/rclcpp-issues-2370

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya commented Dec 1, 2023 •
edited
Loading

…_sim_time"

address #2370

depends on #2742

fujitatomoya
fujitatomoya commented Dec 1, 2023
View reviewed changes
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Dec 1, 2023

CC: @Barry-Xu-2018 @iuhilnehc-ynos

@fujitatomoya fujitatomoya mentioned this pull request Dec 1, 2023
Open
@fujitatomoya fujitatomoya self-assigned this Dec 1, 2023
@fujitatomoya fujitatomoya mentioned this pull request Feb 12, 2025
Closed
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/rclcpp-issues-2370 branch from 23b8739 to 9b0881b Compare February 12, 2025 04:47
@fujitatomoya fujitatomoya marked this pull request as ready for review February 12, 2025 04:47
@fujitatomoya fujitatomoya mentioned this pull request Apr 4, 2025
Merged
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/rclcpp-issues-2370 branch from 9f6c2c7 to c727556 Compare April 8, 2025 20:40
@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented Apr 9, 2025 •
edited
Loading

Looks good to me. While reviewing this, I stumbled over

rclcpp/rclcpp/src/rclcpp/time_source.cpp

Line 400 in 892cae9

clock_executor_->spin_until_future_complete(future);

This could also be simplified by just spinning. The future has no way of waking up the executor, and the cancel:

rclcpp/rclcpp/src/rclcpp/time_source.cpp

Line 433 in 892cae9

clock_executor_->cancel();

Is what stops the thread in the end...
I really don't like this spin until future complete api, I have seen a lot of wrong usages...

@ahcorde
Copy link
Copy Markdown
Contributor

ahcorde commented Apr 9, 2025

Pulls: #2378
Gist: https://gist.githubusercontent.com/ahcorde/79e6ba5c051490b0daa01133625efc73/raw/0e41a6ea5e570ef85bb5bc4d62ab11b03e90532d/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15636

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Apr 9, 2025

@ahcorde @jmachowinski thanks for reviewing this. i need to rebase and take care of the failures.

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/rclcpp-issues-2370 branch from c727556 to ef901a9 Compare April 9, 2025 16:27
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Apr 9, 2025

Looks good to me. While reviewing this, I stumbled over

rclcpp/rclcpp/src/rclcpp/time_source.cpp

Line 400 in 892cae9

clock_executor_->spin_until_future_complete(future);

This could also be simplified by just spinning. The future has no way of waking up the executor, and the cancel:

rclcpp/rclcpp/src/rclcpp/time_source.cpp

Line 433 in 892cae9

clock_executor_->cancel();

Is what stops the thread in the end...
I really don't like this spin until future complete api, I have seen a lot of wrong usages...

@jmachowinski agree, i can patch that with another PR (and probably backports), because this one is dependent on current rolling only. i will include you once that is ready.

fujitatomoya
fujitatomoya commented Apr 9, 2025
View reviewed changes
fujitatomoya
fujitatomoya commented Apr 9, 2025
View reviewed changes
@fujitatomoya fujitatomoya mentioned this pull request Apr 9, 2025
Merged
This was referenced Apr 10, 2025
Closed
Closed
@fujitatomoya
use NodeParameterInterface instead of /parameter_event to update "use…
10c81fe 
…_sim_time"

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
re-enable ParameterMutationRecursionGuard.
c89b4ca 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
adjust test cases.
85f656c 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/rclcpp-issues-2370 branch from 762bf58 to 85f656c Compare April 11, 2025 03:57
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Apr 11, 2025

Pulls: #2378
Gist: https://gist.githubusercontent.com/fujitatomoya/5c5385f4f86450bdac3203a85de2770f/raw/0e41a6ea5e570ef85bb5bc4d62ab11b03e90532d/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15661

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Apr 11, 2025

windows failure is unrelated.

@Yadunund Yadunund merged commit c01602f into rolling Apr 11, 2025
2 of 3 checks passed
@fujitatomoya fujitatomoya mentioned this pull request Apr 11, 2025
Closed
@Crola1702 Crola1702 mentioned this pull request Aug 7, 2025
Closed
@christophebedard christophebedard deleted the fujitatomoya/rclcpp-issues-2370 branch August 14, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmachowinski jmachowinski jmachowinski approved these changes

Assignees

@fujitatomoya fujitatomoya

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fujitatomoya @jmachowinski @ahcorde @Yadunund