-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: introduce the advanced publisher and subscriber (backport #368) #469
Conversation
* feat: introduce the advanced publisher/subscriber * chore: ament_uncrustify and ament_cpplint * trigger CI * refactor: remove the unneeded querying callbacks * fix: reorder the advanced pub/sub options to match the ROS2 QoS * chore: add tmp * chore: apply the QoS mapping * style: ament_cpplint + ament_uncrustify * trigger CI * refactor: remove the recovery option as it exceeds typical ROS use cases * chore: remove the QoS for end-to-end reliability * style: ament_uncrustify * Cleanup Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> Co-authored-by: Yadunund <[email protected]> (cherry picked from commit 77561d8) # Conflicts: # rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp
Cherry-pick of 77561d8 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
auto options = zenoh::Publisher::PutOptions::create_default(); | ||
options.attachment = create_map_and_set_sequence_num( | ||
sequence_number_++, | ||
entity_->copy_gid()); | ||
int64_t source_timestamp = rmw_zenoh_cpp::get_system_time_in_ns(); | ||
auto opts = zenoh::ext::AdvancedPublisher::PutOptions::create_default(); | ||
opts.put_options.attachment = rmw_zenoh_cpp::AttachmentData( | ||
sequence_number_++, source_timestamp, entity_->copy_gid()).serialize_to_zbytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're switching from create_map_and_set_sequence_num()
to AttachmentData
in this PR which was not part of #368.
Do we still need create_map_and_set_sequence_num
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best if we do a backport of #294 but make modifications to drop the tracetools
dependency. That should ensure we're consistent with our usage of AttachmentData
instead of create_map_and_set_sequence_num
. Please hold off on merging this and the humble backport in the meanwhile.
@Yadunund the tracetools PR was merge and this one was updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with all tests passing!
Sorry this seems unrelated to this PR. It was just the first thing to break. I need to debug my setup. My problem is that flags such as |
With this PR, we replace the
QueryingSubscriber
andPublicationCache
(+ a normal publisher)with the
AdvancedSubscriber
andAdvancedPublisher
.NOTE: A deadlock is found when undeclaring advanced subscribers. It will be fixed once eclipse-zenoh/zenoh#1685 is merged and synced with zenoh-c.
This is an automatic backport of pull request #368 done by Mergify.