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

Solve of issue #111 #128

Merged
merged 9 commits into from May 10, 2024
Merged

Solve of issue #111 #128

merged 9 commits into from May 10, 2024

Conversation

shivangvijay
Copy link
Contributor

Based on my understanding, I attempt to address issue #111. If it doesn't align with your expectations, please provide further clarification on this matter.

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

@shivangvijay appreciate the effort to address the outstanding issue but the implementation here is not quite what is expected. I highly recommend commenting on the ticket to discuss implementation strategies before working on a PR in case something is unclear. It's also a good way to let others know you're working on this ticket to prevent duplicated efforts. Please look at my comments below.

rmw_zenoh_cpp/src/detail/liveliness_utils.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/liveliness_utils.cpp Outdated Show resolved Hide resolved
@shivangvijay
Copy link
Contributor Author

Thank you, @Yadunund , for your guidance. In the future, I will comment on the ticket.

I am unable to understand why my build pipeline is failing, I am not changing anything in the CMakelists.txt file. Please let me know what I am doing wrong.

rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
shivangvijay and others added 2 commits March 12, 2024 13:04
Co-authored-by: Yadu <yadunund@gmail.com>
Signed-off-by: Shivang Vijay <shivangvijay@gmail.com>
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback. One more minor change.

rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
shivangvijay and others added 2 commits March 12, 2024 14:01
Co-authored-by: Yadu <yadunund@gmail.com>
Signed-off-by: Shivang Vijay <shivangvijay@gmail.com>
@shivangvijay
Copy link
Contributor Author

@Yadunund Is there any work remaining in this PR? Is not get merged till now.

@Yadunund
Copy link
Member

@shivangvijay sorry for the delay.

I think we should add a / separator between the domain id and mangled topic name such that the keyepxression would look like 0/%chatter. This way, if we ever need to get the original constituents of a keyexpression, we can split over /. We adopt this approach for the lifecycle token's keyexpression.

So could you append a / to the domain id string?

@shivangvijay
Copy link
Contributor Author

shivangvijay commented Mar 27, 2024

@shivangvijay sorry for the delay.

I think we should add a / separator between the domain id and mangled topic name such that the keyepxression would look like 0/%chatter. This way, if we ever need to get the original constituents of a keyexpression, we can split over /. We adopt this approach for the lifecycle token's keyexpression.

So could you append a / to the domain id string?

If I add '/' in the domain ID string, I am facing an issue. I have attached a screenshot for your reference.
rmw

I tried below thing:-
Screenshot from 2024-03-27 18-34-31
But the return of z_keyexpr is not a "z_owned_keyexpr_t" type.

@shivangvijay
Copy link
Contributor Author

Now, I am exploring https://github.com/eclipse-zenoh/zenoh-cpp to understand z_owned_keyexpr_t and implementation of z_keyexpr, and how to convert it's return value to z_owned_keyexpr_t.

@Yadunund
Copy link
Member

Yadunund commented Apr 3, 2024

Why not simply avoid calling z_keyexpr_join?

const std::string keyexpr_str = std::to_string(domain_id) + "/" + liveliness::mangle_name(topic_name);
return z_keyexpr(keyexpr_str.c_str());

@shivangvijay
Copy link
Contributor Author

shivangvijay commented Apr 3, 2024

Why not simply avoid calling z_keyexpr_join?

const std::string keyexpr_str = std::to_string(domain_id) + "/" + liveliness::mangle_name(topic_name);
return z_keyexpr(keyexpr_str.c_str());

z_keyexpr return z_keyexpr_t type not z_owned_keyexpr_t type.

Error

@Yadunund
Copy link
Member

Yadunund commented Apr 3, 2024

Sorry I meant z_keyexpr_new.

Signed-off-by: Yadunund <yadunund@openrobotics.org>
@Yadunund Yadunund merged commit 152ab94 into ros2:rolling May 10, 2024
6 checks passed
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.

None yet

2 participants