Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 11, 2025

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented Dec 1, 2023

…_sim_time"

address #2370

depends on #2742

@fujitatomoya
Copy link
Collaborator Author

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

@jmachowinski
Copy link
Collaborator

jmachowinski commented Apr 9, 2025

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

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:

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
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
Collaborator Author

@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
Collaborator Author

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

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:

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 force-pushed the fujitatomoya/rclcpp-issues-2370 branch from 762bf58 to 85f656c Compare April 11, 2025 03:57
@fujitatomoya
Copy link
Collaborator Author

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
Collaborator Author

windows failure is unrelated.

@Yadunund Yadunund merged commit c01602f into rolling Apr 11, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants