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 CapnProto Publisher and Subscriber to Python API #1089

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

Conversation

CruxDevStuff
Copy link

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the new behavior?

  • Subscribe to CapnProto Messages from Python
  • Publish CapnProto Messages from Python

Does this introduce a breaking change?

  • Yes
  • No

@rex-schilasky
Copy link
Contributor

rex-schilasky commented May 5, 2023

Thank you @CruxDevStuff for contributing ! We will check this next week. If you have python samples for sending and receiving capnproto messages like those ones in cpp, it would be great too.

@CruxDevStuff
Copy link
Author

Thank you @CruxDevStuff for contributing ! We will check this next week. If you have python samples for sending and receiving capnproto messages like those ones in cpp, it would be great too.

I've added an Example

@rex-schilasky
Copy link
Contributor

Thank you @CruxDevStuff for contributing ! We will check this next week. If you have python samples for sending and receiving capnproto messages like those ones in cpp, it would be great too.

I've added an Example

Great. Thank you.

@rex-schilasky
Copy link
Contributor

@CruxDevStuff can you please create an eclipse account, then we can merger your PR after the review has been done.

@CruxDevStuff
Copy link
Author

@CruxDevStuff can you please create an eclipse account, then we can merger your PR after the review has been done.

Sorry for the delay, I've created an Eclipse account that's linked my Github(I can view this PR from my eclipse account page).

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the long wait.
Thanks for making this contribution, however I do have a few remarks:

  • Dependencies: we now have a dependency to pycapnp, it needs to be added to requirements.txt for ecal.
  • Before running the samples, the user will have to run the capnproto compiler on the schema file themselves. I'd prefer if we'd check in the generated code here as we do in the protobuf case. Or, alternatively, have a way to generate this on python import? At least it should be in the README.md

@@ -131,9 +131,116 @@ def _on_receive(self, topic_name, msg, time):
proto_message.ParseFromString(msg)
self.callback(topic_name, proto_message, time)

class ProtoSubscriber(MessageSubscriber):
Copy link
Contributor

Choose a reason for hiding this comment

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

There are now two ProtoSubscriber classes. Can you please remove one of them?

@@ -0,0 +1,8 @@
@0xb6249c5df1f7e512;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use the same message like is used for the c++ samples? this way we can communicate the samples with python send / c++ receive and vice versa.

@KerstinKeller
Copy link
Contributor

Ah and one more thing, the samples should be placed in a subfolder samples/python/pubsub/capnp.
Also I am not sure if i'd prefer
CapnpPublisher or CapnprotoPublisher over CapnPublisher (e.g. same for subscribers).

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

3 participants