-
Notifications
You must be signed in to change notification settings - Fork 57
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 liveliness bindings #189
Conversation
Hello @sashacmc , in order to merge the PR we will need you to sign the Eclipse Contribution Agreement. |
cf215fd
to
5145f29
Compare
*/ | ||
typedef struct zc_owned_liveliness_declaration_options_t { | ||
uint8_t _inner; | ||
} zc_owned_liveliness_declaration_options_t; |
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.
The zenoh-commons.h
header is meant to be common across zenoh-c
and zenoh-pico
.
All functions starting with zc_
are inherently meant to be specific to zenoh-c
.
All these function declarations should hence go into zenoh-concrete.h.
@p-avital @milyin I see that other zc_
functions slipped in zenoh-commons.h
in the past. I think they should be moved into zenoh-concrete.h
as well. Opinions?
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.
Yeah, the original goal was to be able to copy-paste this header into pico to ensure we always have the same signatures, but it didn't end up working out that way.
I think we can make the splitter logic a bit better if we want to stick to the original ideal. Otherwise, we could remove the splitter entirely, or just leave it in that state. Any odd these solutions is fine by me :)
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.
zenoh-pico
has evolved quite a bit since we initially made that split and I see how the split could hardly work out in practice. Therefore I'm more in favour of unifying all the includes in a single file rather than introducing artificial splits (although correct in principle).
For what concerns the scope of this specific PR, let's not change anything for the time being... A different PR may follow in the future.
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.
Agreed, though note that we'll never be single filed because the zenoh-macros file is still hand-written
fyi I ran into some issues when relying on |
Is this the first time you've had issues with these macros? I seem to remember some incompatibility between C's generic macros and C++... The macros failing only on liveliness is strange considering the implementation of the macro is the same for them as for the rest... |
yup first time running into problems with the macros. I've been relying on them to check/drop owned sessions, publishers, subscribers without any problems. |
@Yadunund the I created ros2/rmw_zenoh#69 that hopefully will build successfully once workflows are approved. |
Add the liveliness methods bindings:
Along with the examples: