Skip to content
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

refactor: Introduce PublisherOptions #372

Merged
merged 3 commits into from
Feb 14, 2025
Merged

refactor: Introduce PublisherOptions #372

merged 3 commits into from
Feb 14, 2025

Conversation

bdm-k
Copy link
Contributor

@bdm-k bdm-k commented Feb 10, 2025

Description

#306 の実装に向けて、とりあえず do_always_ros2_publishPublisherOptions 構造体に入れました。(see #326 (comment))

Related links

How was this PR tested?

  • Autoware (required)
  • bash scripts/e2e_test_1to1_with_ros2sub (required)
  • bash scripts/e2e_test_2to2 (required)
  • sample application

Notes for reviewers

  • 具体的にどのような時に do_always_ros2_publish が必要になるか、コメントがあるといいと思いました。

@bdm-k
Copy link
Contributor Author

bdm-k commented Feb 10, 2025

@Koichi98
Autoware でのテストやっぱりうまくいかなかったのでこちらもお願いしたいです。

@Koichi98 Koichi98 changed the title refactor: Introduce PublisherOptioins refactor: Introduce PublisherOptions Feb 10, 2025
@Koichi98
Copy link
Contributor

@bdm-k

具体的にどのような時に do_always_ros2_publish が必要になるか、コメントがあるといいと思いました。

ありがとうございます!
APIに関するところなのでゆくゆくはドキュメントにもしっかり記載することにはなると思いますが、確かに
// For transient local.だけだとちょっとすくないかもですね。
@veqcc いかがでしょう?

@Koichi98
Copy link
Contributor

Koichi98 commented Feb 10, 2025

Autoware でのテストやっぱりうまくいかなかったのでこちらもお願いしたいです。

動作確認完了です。

@veqcc
Copy link
Contributor

veqcc commented Feb 10, 2025

APIに関するところなのでゆくゆくはドキュメントにもしっかり記載することにはなると思いますが、確かに
// For transient local.だけだとちょっとすくないかもですね。
@veqcc いかがでしょう?

// If true, publish() does both Agnocast publish and ROS 2 publish, regardless of the existence of ROS 2 subscriptions.
くらいですかね?

atsushi421
atsushi421 previously approved these changes Feb 10, 2025
@Koichi98
Copy link
Contributor

@bdm-k

// If true, publish() does both Agnocast publish and ROS 2 publish, regardless of the existence of ROS 2 subscriptions.

これで良さそうなら、これも今回のPRに加えてしまいましょう。

Copy link
Contributor

@veqcc veqcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@veqcc veqcc added the run-build-test Run build-test in CI label Feb 14, 2025
Copy link

------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
File                                       Lines    Exec  Cover   Missing
------------------------------------------------------------------------------
src/agnocastlib/include/agnocast.hpp           3       3   100%   
src/agnocastlib/include/agnocast_callback_info.hpp
                                              13      10    76%   92,94-95
src/agnocastlib/include/agnocast_executor.hpp
                                               1       0     0%   40
src/agnocastlib/include/agnocast_publisher.hpp
                                              32      27    84%   78,110-111,115-116
src/agnocastlib/include/agnocast_smart_pointer.hpp
                                              35      35   100%   
src/agnocastlib/src/agnocast.cpp              59      12    20%   19,22,24-30,34-35,38-42,46-47,51-54,57,60,62,65,67-68,73,75-77,80-83,86,88-94,96,108-109
src/agnocastlib/src/agnocast_callback_info.cpp
                                              16       0     0%   12,16-17,20-24,28-31,34-37
src/agnocastlib/src/agnocast_component_container.cpp
                                              12       0     0%   5,7-8,11-13,16,18-20,22-23
src/agnocastlib/src/agnocast_component_container_mt.cpp
                                              28       0     0%   7,11-12,14-16,18-20,22-24,26-28,30,32-33,35-37,40,42,44-46,48-49
src/agnocastlib/src/agnocast_executor.cpp
                                              90       0     0%   11,13,15-17,19-21,25,27,30,32-33,36-40,43-46,48-49,52-54,56-59,62-63,68,71,74,76-80,83,87-88,91-92,95,97-101,104,111-116,119,122-125,127-130,134-138,141,144-145,155,158,160,163,167-168,171-173,177,183,186,188-189
src/agnocastlib/src/agnocast_multi_threaded_executor.cpp
                                              53       0     0%   9,13,15-17,19-20,25-26,29-33,36,38-41,44,48,51,53-55,58-60,64,66-67,71,74,77,79-80,82-83,87-88,91,96,100,103-104,107,109-110,118-121,124
src/agnocastlib/src/agnocast_publisher.cpp
                                              54       7    13%   10,12,23,26,31,33,35-41,44,47,52-60,69-70,72-75,77-80,82,87,91-92,97,100,102-107,110
src/agnocastlib/src/agnocast_single_threaded_executor.cpp
                                              19       0     0%   10-12,16,18-21,24,28-30,33,35-37,40-42
src/agnocastlib/src/agnocast_smart_pointer.cpp
                                              18       0     0%   6,9-16,20,23-30
src/agnocastlib/src/agnocast_subscription.cpp
                                              51       9    17%   6-8,10,13,15,19-22,24-29,32-36,39,42,46-51,53-58,60,62,65,69-70,72-73
src/agnocastlib/src/agnocast_utils.cpp        25      22    88%   50,52-53
------------------------------------------------------------------------------
TOTAL                                        509     125    24%
------------------------------------------------------------------------------

@bdm-k bdm-k merged commit 33481c9 into main Feb 14, 2025
4 checks passed
@bdm-k bdm-k deleted the 306-publisher-option branch February 14, 2025 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-build-test Run build-test in CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants