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

[ROS2] increase history_depth for subscrition #168

Merged
merged 2 commits into from
Mar 3, 2021

Conversation

mitsudome-r
Copy link
Contributor

This resolves issue : #167.

This makes the following modfiication:

  • The new parameter history_depth for diagnostic_aggregator node to enable users to set history depth size(i.e. queue size) for subscription.
  • Change default depth size to 1000 from 1.

Signed-off-by: mitsudome-r ryohsuke.mitsudome@tier4.jp

@Karsten1987
Copy link
Contributor

This looks good to me. Please target the PR against the ros2 branch. I'll take care of a backport to existing distributions and cut a release.

@mitsudome-r mitsudome-r changed the base branch from foxy to ros2-devel November 13, 2020 05:04
@mitsudome-r
Copy link
Contributor Author

@Karsten1987
Thank you for the review.
I changed the target to ros2-devel. I also rebased to the branch as well.

@tgreier
Copy link

tgreier commented Dec 30, 2020

@Karsten1987 Hi Karsten. We need this PR for Foxy. But, it looks like this PR has stalled. What needs to be done to progress it?

Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

Just a little nitpick, otherwise all almost good, except the linters.

diagnostic_aggregator/src/aggregator.cpp Outdated Show resolved Hide resolved
@Karsten1987
Copy link
Contributor

@tgreier The changes reflected as is in this PR can't be backported to Foxy due to ABI incompatibility.
One way I would be okay with is to "simply" hardcode the QoS to a keep_last(1000) which should solve the problem described in #167 and achieve same behavior as ROS1.
Opinions?

Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

actually, I've just seen that uncrustify and cpplint aren't happy. Mind addressing them?

You'll find the details in the GH actions, e.g.:

  [18.595s] 12: --- src/aggregator.cpp
  [18.595s] 12: +++ src/aggregator.cpp.uncrustify
  [18.595s] 12: @@ -103 +103,2 @@
  [18.595s] 12: -    "/diagnostics", rclcpp::SystemDefaultsQoS().keep_last(history_depth_), std::bind(&Aggregator::diagCallback, this, _1));
  [18.595s] 12: +    "/diagnostics", rclcpp::SystemDefaultsQoS().keep_last(history_depth_),
  [18.595s] 12: +    std::bind(&Aggregator::diagCallback, this, _1));

mitsudome-r and others added 2 commits March 1, 2021 14:24
…gator

Signed-off-by: mitsudome-r <ryohsuke.mitsudome@tier4.jp>
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.

3 participants