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

Add blacklisted topics #1

Merged
merged 2 commits into from
Aug 31, 2022
Merged

Add blacklisted topics #1

merged 2 commits into from
Aug 31, 2022

Conversation

dimaxano
Copy link

Purpose

Sometimes we do not want to record some heavy topics like Image or Pointcloud.

Approach

Add ROS param blacklist_topics. If any of existing topics is in that list, do not subscribe to it.

Disclaimers

That implementation is not able to subscribe to the topics with non-default QoS policy, which makes it unable to subscribe to some sensor data (Imu, Images). So I add dirty fix for that here. Allow it to live here, please, until I return back with proper fix like described in this issue.

@dimaxano
Copy link
Author

image
@guilyx @cmraaron

Copy link

@cmraaron cmraaron left a comment

Choose a reason for hiding this comment

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

Its not a biggie, but it seems a bit fiddly with these repeated checks and temporary variables eg:

  if (topics.size() > 0 || blacklist_topics.size() > 0) {
    std::vector<std::string> topics_to_parse;
    if(topics.size() > 0){
      topics_to_parse = topics;
      options_.all_topics_ = false;
...

There are two ways to simplify this that immediately stand out to me.
One is to just use topics_, and treat it as either a whitelist if options_.all_topics_ == false, or a blacklist if options_.all_topics_ == true

Or if you prefer keeping separate topics_ and blacklist_topics_, then trust that the exception throwing will guarantee that we will never have both. Extract a function from the options_.[blacklist_]_topics_.insert( code, and call it twice. Once with (topics_, topics), the other with (blacklist_topics_, blacklist_topics)

@dimaxano
Copy link
Author

@cmraaron I like the idea of having a single argument with topics names, so I added is_blacklist ROS parameter (all_topics you proposed, is an internal variable, not a ROS param, that is why I created new param)

@dimaxano dimaxano merged commit de971df into galactic-devel Aug 31, 2022
@arekin19 arekin19 deleted the feat/blacklist_topic branch April 16, 2023 22:25
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