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

add C function to register callback for matching subscribers #236

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

filippobrizzi
Copy link
Contributor

Add z_publisher_matching_listener_callback function to register user callback when the publisher connects or disconnects to subscribers.

To complete exposing the full functionality I'll add z_publisher_matching_listener_callback
This will allow to expose recv_async.

@Mallets
Copy link
Member

Mallets commented Jan 23, 2024

Thanks @filippobrizzi for your PR. To be able to merge any PR you'd need to sign first the Eclipse Contribution Agreement.

@p-avital and @milyin would you mind having a look at this PR?

@filippobrizzi
Copy link
Contributor Author

Thanks @filippobrizzi for your PR. To be able to merge any PR you'd need to sign first the Eclipse Contribution Agreement.

I created an account at https://accounts.eclipse.org/, but I did not receive any confirmation email and cannot login. Do you know if the server is down or something?

@Mallets
Copy link
Member

Mallets commented Jan 23, 2024

I don't know tbh. But make sure that you used the same email address as the one used in GitHub for this PR. The Eclipse tool's checks require the email address to be the same.

Copy link
Contributor

@p-avital p-avital left a comment

Choose a reason for hiding this comment

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

Hi,

Thank you for your contribution. This seems sound to me.

However, we still consider that API to be unstable, so the prefix for all things pertaining to it should be zcu (c for "zenoh-c exclusivity" and u for "unstable"). I've marked every symbol that should be renamed with a "zcu" comment to help you track them all down :)

src/closures/matching_status_closure.rs Outdated Show resolved Hide resolved
src/closures/matching_status_closure.rs Outdated Show resolved Hide resolved
src/closures/matching_status_closure.rs Outdated Show resolved Hide resolved
src/closures/matching_status_closure.rs Outdated Show resolved Hide resolved
src/closures/matching_status_closure.rs Outdated Show resolved Hide resolved
src/publisher.rs Outdated Show resolved Hide resolved
src/publisher.rs Outdated Show resolved Hide resolved
src/publisher.rs Outdated Show resolved Hide resolved
/// After a move, `val` will still exist, but will no longer be valid. The destructors are double-drop-safe, but other functions will still trust that your `val` is valid.
///
/// To check if `val` is still valid, you may use `z_X_check(&val)` or `z_check(val)` if your compiler supports `_Generic`, which will return `true` if `val` is valid.
#[repr(C, align(8))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@p-avital here I followed what was done for the other struct like z_owned_subscriber_t but for it there is a differentiation for arm. Am I supposed to add it also here? How can I test it?

@filippobrizzi
Copy link
Contributor Author

Hi,

Thank you for your contribution. This seems sound to me.

However, we still consider that API to be unstable, so the prefix for all things pertaining to it should be zcu (c for "zenoh-c exclusivity" and u for "unstable"). I've marked every symbol that should be renamed with a "zcu" comment to help you track them all down :)

@p-avital Thanks for the feedback, I have completed the refactor.

Copy link
Contributor

@p-avital p-avital left a comment

Choose a reason for hiding this comment

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

LGTM, but I think your formatter broke the docs formatting a bit. I'll recommit the trailing spaces (md syntax) and merge your PR :)

src/publisher.rs Outdated Show resolved Hide resolved
@p-avital p-avital merged commit 725fa8e into eclipse-zenoh:main Jan 24, 2024
5 checks passed
@filippobrizzi filippobrizzi deleted the matching_listener branch January 24, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants