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

Added devices API #331

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added devices API #331

wants to merge 4 commits into from

Conversation

drbh
Copy link

@drbh drbh commented Mar 6, 2021

added the devices api to match functionality in pyzmq

I tried to follow the style of the library - please let me know if there are any changes needed before merging

@tdudz
Copy link

tdudz commented Jun 28, 2021

any updates on this? would like to see it get merged

@tdudz
Copy link

tdudz commented Aug 5, 2021

bump on this again, thanks

@drbh
Copy link
Author

drbh commented Aug 5, 2021

@erickt, any update on reviewing/merging this? Thanks

QUEUE => zmq_sys::ZMQ_QUEUE,
};
raw as c_int
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could this return a result rather than a panic?

Also, could you add a newline before this line?

updated `from_raw` to return a result rather than a panic, and added a new line between `to_raw` and `from_raw`
Copy link
Owner

@erickt erickt left a comment

Choose a reason for hiding this comment

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

Looking through the pyzmq API docs, it looks like the device api was dropped in ZMQ 3.2, and replaced with http://api.zeromq.org/master:zmq-proxy. We do already have support for this in

pub fn proxy(frontend: &Socket, backend: &Socket) -> Result<()> {
.

I wouldn't consider myself the main maintainer of this library (cc @rotty), but it looks like we're currently depending on zeromq 4.1 and above, so could you switch to proxy?

Also, do you know if zmq 4.1 and above actually exposes zmq_device? It might not actually be exposed, which might cause a compilation issue.

Finally, if we do decide to support this device API, can you add some tests for it?

@tdudz
Copy link

tdudz commented Aug 14, 2021

ah ok i didn't see that device was deprecated. ill try switching over to proxy, thanks

@erickt
Copy link
Owner

erickt commented Aug 14, 2021

@tdudz no problem! Please let me know if you still need this functionality, and we can try to come up with a way to implement it (since it seems pyzmq somehow does it)

@tdudz
Copy link

tdudz commented Aug 15, 2021

@erickt i just tested my project using proxy and it seems to work fine. sorry for all the trouble, i should have read the docs a little closer! appreciate the help though and thanks for the great zmq package

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