fix(agnocastlib): use dedicated executor thread for clock#1054
Open
fix(agnocastlib): use dedicated executor thread for clock#1054
Conversation
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for a dedicated executor thread for handling clock messages in the agnocastlib NodeTimeSource implementation. The change introduces a new optional parameter use_clock_thread (defaulting to true) that creates a separate thread with its own single-threaded executor to process /clock topic messages. This prevents clock callbacks from blocking or being delayed by other node operations.
Changes:
- Added
use_clock_threadparameter to NodeTimeSource constructor to enable/disable dedicated clock thread - Created dedicated callback group, executor, and thread for clock subscription when enabled
- Added proper thread cleanup in
destroy_clock_sub()method
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/agnocastlib/include/agnocast/node/node_interfaces/node_time_source.hpp | Added member variables for clock executor thread, callback group, and use_clock_thread flag; added use_clock_thread parameter to constructor |
| src/agnocastlib/src/node/node_interfaces/node_time_source.cpp | Implemented dedicated executor thread creation and cleanup logic; added callback group configuration for clock subscription |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add use_clock_thread option to NodeTimeSource to process /clock subscription on a dedicated executor thread, aligning with rclcpp's TimeSource implementation.
Changes:
use_clock_threadparameter (default: true)AgnocastOnlySingleThreadedExecutorand thread for clock callbacksdestroy_clock_sub()Related links
https://github.com/ros2/rclcpp/blob/rolling/rclcpp/src/rclcpp/time_source.cpp
How was this PR tested?
bash scripts/e2e_test_1to1(required)bash scripts/e2e_test_2to2(required)Notes for reviewers
Version Update Label (Required)
Please add exactly one of the following labels to this PR:
need-major-update: User API breaking changesneed-minor-update: Internal API breaking changes (heaphook/kmod/agnocastlib compatibility)need-patch-update: Bug fixes and other changesImportant notes:
need-major-updateorneed-minor-update, please include this in the PR title as well.fix(foo)[needs major version update]: barorfeat(baz)[needs minor version update]: quxrun-build-testlabel. The PR can only be merged after the build tests pass.See CONTRIBUTING.md for detailed versioning rules.