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

content filtered topic for action_feedback and parameterevent #1592

Open
wants to merge 14 commits into
base: rolling
Choose a base branch
from

Conversation

iuhilnehc-ynos
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos commented Mar 23, 2021

related to ros2/design#282

/parameter_events enhancement
action feedback enhancement

@fujitatomoya
Copy link
Collaborator

the same question with ros2/rcl#901 (comment) here, this PR includes #1561.

@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Mar 24, 2021

the same question with ros2/rcl#901 (comment) here, this PR includes #1561.

Yes. Due to the #1561 not merged, I think I need to do it in this way. Do you have any suggestions?

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

overall, seems to be okay with nitpicks.

@@ -263,6 +263,41 @@ class SubscriptionBase : public std::enable_shared_from_this<SubscriptionBase>
bool
exchange_in_use_by_wait_set_state(void * pointer_to_subscription_part, bool in_use_state);

/// Check if subscription instance can support content filter topic feature.
/**
* Depending on the middleware and the message type, this will return true if the middleware
Copy link
Collaborator

Choose a reason for hiding this comment

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

oversight from internal review, is this dependent on message type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing it out.

@@ -260,6 +278,7 @@ void TimeSource::destroy_clock_sub()

void TimeSource::on_parameter_event(const rcl_interfaces::msg::ParameterEvent::SharedPtr event)
{
RCLCPP_DEBUG(logger_, "this node should not get others' parameter event if CFT is supported");
Copy link
Collaborator

Choose a reason for hiding this comment

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

if CFT is not supported, this prints every single time on parameter event happens. probably we would want to print this message if CFT is supported only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I'll update it.

@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-content_filtered_topic-action-parameterevent branch from d1d3c34 to ba4ece3 Compare March 24, 2021 05:54
@wjwwood wjwwood added the enhancement New feature or request label Mar 29, 2021
Chen Lihui and others added 14 commits October 11, 2021 15:01
Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Chen Lihui <[email protected]>
Signed-off-by: Chen Lihui <[email protected]>
to support content filter for action client feedback subscription

Signed-off-by: Chen Lihui <[email protected]>
@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-content_filtered_topic-action-parameterevent branch from ba4ece3 to 50aeb0a Compare January 17, 2022 08:21
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants