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

Update publishing API documentation. #270

Merged
merged 10 commits into from
Sep 4, 2020
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Sep 1, 2020

Precisely what the title says.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 2, 2020

Alright, I think this is now complete. PTAL !

@hidmic hidmic requested a review from wjwwood September 2, 2020 15:27
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 2, 2020

Ok, now it is ready. A couple things were either not constrained enough or not generic enough.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
* \param[in] publisher The publisher object registered to send the message.
* \param[in] serialized_message The serialized message holding the byte stream.
* \param[in] allocation Specify preallocated memory to use (may be NULL).
* Send a ROS message serialized in a byte stream directly over the wire to
Copy link
Contributor Author

@hidmic hidmic Sep 2, 2020

Choose a reason for hiding this comment

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

What about implementations that do not use serialization (e.g. shared memory-based ones)? Do we allow them to give back the ROS message as a blob, with no changes?

Copy link
Member

@ivanpauno ivanpauno Sep 2, 2020

Choose a reason for hiding this comment

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

I think in those cases they can implement "serialize/deserialize" as a noop, and publish/taking the serialized message would be the same that the non-serialized one.

(edit) more than a noop, the serialized message would be an exact copy of the original one. I think that's what you meant with "give back the ROS message as a blob" now that I read again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

more than a noop, the serialized message would be an exact copy of the original one.

Exactly. I didn't think we should make any promises either, but I wanted to make sure others agree.

Copy link
Member

Choose a reason for hiding this comment

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

I would just say "publish a serialized ROS message", because several things could occur at this point:

  • it could go to the wire
  • it could need to be deserialized and converted to a new type (DDS type maybe if serialized publishing is not supported properly)
  • it could be sent over shared memory or within the process as a serialized blob
  • other things possibly...

So I'd just say "published" where that means "delivered to all matched subscriptions", without saying more than that.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

lgtm

rmw/include/rmw/rmw.h Show resolved Hide resolved
* \param[in] publisher The publisher object registered to send the message.
* \param[in] serialized_message The serialized message holding the byte stream.
* \param[in] allocation Specify preallocated memory to use (may be NULL).
* Send a ROS message serialized in a byte stream directly over the wire to
Copy link
Member

@ivanpauno ivanpauno Sep 2, 2020

Choose a reason for hiding this comment

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

I think in those cases they can implement "serialize/deserialize" as a noop, and publish/taking the serialized message would be the same that the non-serialized one.

(edit) more than a noop, the serialized message would be an exact copy of the original one. I think that's what you meant with "give back the ROS message as a blob" now that I read again.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm other than the one comment thread

* \param[in] publisher The publisher object registered to send the message.
* \param[in] serialized_message The serialized message holding the byte stream.
* \param[in] allocation Specify preallocated memory to use (may be NULL).
* Send a ROS message serialized in a byte stream directly over the wire to
Copy link
Member

Choose a reason for hiding this comment

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

I would just say "publish a serialized ROS message", because several things could occur at this point:

  • it could go to the wire
  • it could need to be deserialized and converted to a new type (DDS type maybe if serialized publishing is not supported properly)
  • it could be sent over shared memory or within the process as a serialized blob
  • other things possibly...

So I'd just say "published" where that means "delivered to all matched subscriptions", without saying more than that.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, thanks this is really a great improvement!

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 3, 2020

Alright, that should be it.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

lgtm

*
* \pre Given `publisher` must be a valid publisher, as returned by rmw_create_publisher().
* \pre Given `type_support` must be a valid `rosidl` message type support, matching the
* one registered with the `publisher` on creation.
Copy link
Member

@ivanpauno ivanpauno Sep 4, 2020

Choose a reason for hiding this comment

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

unrelated, but it's weird that we're passing the typesupport again here when the publisher already stores one internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, it is weird.

@hidmic
Copy link
Contributor Author

hidmic commented Sep 4, 2020

Going in!

@hidmic hidmic merged commit 2d6ea69 into master Sep 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/update-publish-docs branch September 4, 2020 20:31
ahcorde pushed a commit that referenced this pull request Oct 13, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 13, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
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