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

ROS2: Add UDP support #140

Merged
merged 1 commit into from
Oct 12, 2021
Merged

Conversation

kmhallen
Copy link
Contributor

@kmhallen kmhallen commented Oct 7, 2021

No description provided.

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks generally good to me, though I have a comment/question about the doWrite/doRead templated methods.

Can we use the asio::write() and stream_->async_read_some() ASIO methods for UDP? If not, why not? Isn't the point of ASIO to paper over these differences so we don't have to have separate template methods like this?

@kmhallen
Copy link
Contributor Author

Some lower level functions weren't defined for UDP sockets with write() and async_read_some(). Comment out the added lines to async_worker.h to see the build errors. I couldn't find a better way.

@clalancette
Copy link
Collaborator

Some lower level functions weren't defined for UDP sockets with write() and async_read_some(). Comment out the added lines to async_worker.h to see the build errors. I couldn't find a better way.

All right, that's fine. I'm not that upset with having the extra template methods, it just seems odd given the usage ASIO. This looks good to me, thanks for the contribution!

@clalancette clalancette merged commit 7456c7b into KumarRobotics:foxy-devel Oct 12, 2021
@kmhallen kmhallen mentioned this pull request Oct 19, 2021
@kmhallen kmhallen deleted the ros2/udp branch November 18, 2021 01:06
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