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

get valid subscription handle even if cft failed #106

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

Conversation

iuhilnehc-ynos
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos commented Feb 27, 2023

rcl need to continue to try rcl cft fallback if rmw implementation failed to call rmw cft.

@fujitatomoya fujitatomoya requested a review from asorbini March 7, 2023 18:03
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.

one comment needs to be confirmed but lgtm.

rmw_connextdds_common/src/common/rmw_impl.cpp Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator

@asorbini we would like to request the another review on this.

Copy link
Collaborator

@asorbini asorbini left a comment

Choose a reason for hiding this comment

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

@iuhilnehc-ynos thank you for submitting these changes, and apologies for the late reply.

The change looks good to me, but I'm not sure why they are needed.

Is the ROS filtering syntax different than the standard DDS one?

If not, I would assume a failure to create the CFT signals some problem with the parameters, and it should not be ignored.

If there are filters that are only supported by the rcl implementation, then this change is fine.

@iuhilnehc-ynos
Copy link
Collaborator Author

@asorbini

Thanks for your reply. No matter when you reply, it's never too late.

Is the ROS filtering syntax different than the standard DDS one?

No, I think it's the same for now, maybe RCL content filter fallback can support extra syntax in the future.
The extra reason for patching this is the RCL content filter fallback could be used if rmw_connextdds is unsupported while using the rtime version.

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