Implement UdpSocket for both Syn. & Async modes (Send & Receive functionality) #31

reza-ebrahimi · 2021-02-09T12:11:25Z

This PR:

Implements UDP send functionality with boost::asio library in UdpSender class.
Refactors UDP receiver functionality to a new UdpReceiver class.

This PR closes issue #12.

reza-ebrahimi · 2021-02-09T12:14:30Z

~~I will add UdpSender tests to this PR by tomorrow.~~

There is still a requirement to get UdpSender to work, Since UdpReceiver receiving data in blocking mode, calling UdpSender in same thread will not work, I'm thinking to convert receiving part to Non-blocking.

reza-ebrahimi · 2021-02-15T13:39:16Z

Both synchronous and asynchronous send and receive functionality are supported.
Every UdpSocket in a process using just one instance of IoContext object that can be used as single/multi thread IO context.
IoContext
- UdpSocket
- TcpSocket (Implementable)
- Serial (Implementable)

reza-ebrahimi · 2021-02-18T14:40:02Z

Current PR Status:

Implemented IoContext (The main I/O Context that can be configured as single or multi thread with async task execution support)
Implemented UdpSocket (The UDP socket functionality that working in blocking and non-blocking (Async with task execution) modes)
Implemented UdpDriver Facade (Facade design pattern implementation that acts as a sender and receiver UDP socket pair container)
Implemented ROS2 and raw buffer message converters for Int, UInt and Float variants in std_msgs namespace.
Implemented UdpDriverNode example and its test.
Implemented IoContext, UdpSocket and UdpDriver tests.

flynneva · 2021-02-19T04:12:26Z

@reza-ebrahimi is this desired to move udp_driver up into the parent transport_drivers directory? @JWhitleyWork any thoughts?

was this done to use IoContext for both the serial_driver and udp_driver?

reza-ebrahimi · 2021-02-19T08:56:47Z

@flynneva Both serial port and UDP socket are using boost::asio::io_service, So potentially all IO operations (UDP, TCP and Serial) could be implemented by using IoContext.

This is my plan for project structure:

- transport_drivers (root)
    - examples
        - UdpDriverNode
        - TcpDriverNode
        - SerialPortNode
    - include (header files)
        - Converters
            - StdMsgs
        - IoContext (This instance is shared among all sockets in a single process)
        - UdpSocket (Using Shared IoContext)
        - TcpSocket (Using Shared IoContext)
        - SerialPortSocket (Using Shared IoContext)
        - UdpDriver (Facade for sender & receiver pair sockets)
        - TcpDriver (Facade for sender & receiver pair sockets)
        - SerialPortDriver (Facade for sender & receiver pair sockets)
    - src (source files)
        - Converters
            - StdMsgs
        - IoContext
        - UdpSocket 
        - TcpSocket
        - SerialPortSocket
        - UdpDriver
        - TcpDriver
        - SerialPortDriver
    - test
        - IoContextTest
        - UdpSocketTest
        - TcpSocketTest
        - SerialPortTest
        - UdpDriverTest
        - TcpDriverTest
        - SerialPortDriverTest
        - UdpDriverNodeTest
        - TcpDriverNodeTest
        - SerialPortDriverNodeTest
package.xml
CMakeLists.txt

flynneva · 2021-02-19T15:57:24Z

i know @JWhitleyWork wanted to still have the option to be able to inherit the UdpDriver class and make a node right off of that. I think you should still be able to do that using this directory structure though.....what do you think @JWhitleyWork?

JWhitleyWork

Added high-level concerns/problems with this PR.

CHANGELOG.rst

README.md

flynneva · 2021-03-01T18:42:16Z

@reza-ebrahimi i think you're desire to use a single IoContext for both the serial_driver and udp_driver is still possible. the IoContext has no ROS-specific dependencies and could be made into a pure cpp shared library in a peer-directory to serial_driver and udp_driver, where both of those ROS packages then depend on that shared library.

JWhitleyWork · 2021-03-01T20:40:54Z

@reza-ebrahimi There are a lot of linting and test errors on your branch. To view the errors, please run colcon build --packages-select udp_driver serial_driver && colcon test --packages-select udp_driver serial_driver && colcon test-result --verbose. These will need to be fixed before this PR can be merged.

flynneva · 2021-03-01T23:11:09Z

@JWhitleyWork ive fixed most of them on my branch, i can make a PR from mine into @reza-ebrahimi to knock out two birds with one stone

JWhitleyWork · 2021-03-01T23:17:48Z

@flynneva That would be awesome! Thanks! Mention me in it and I'll take a quick look as well, if you want.

reza-ebrahimi · 2021-03-02T13:22:46Z

@reza-ebrahimi There are a lot of linting and test errors on your branch. To view the errors, please run colcon build --packages-select udp_driver serial_driver && colcon test --packages-select udp_driver serial_driver && colcon test-result --verbose. These will need to be fixed before this PR can be merged.

@JWhitleyWork Is there any tool for these lint errors to do some of them automatically (Such as formatting)?

reza-ebrahimi · 2021-03-02T13:23:48Z

@flynneva If you are interested, Please work on serial driver using this package IoContext class.

flynneva · 2021-03-02T14:14:52Z

@reza-ebrahimi i just made a PR to your 'main' branch. If you merge it, it should fix the linter errors for this PR.

The whole point of git is to be able to track changes in parallel and build towards a common goal. We're all just trying to get this package off the ground here....same team.

reza-ebrahimi · 2021-03-02T14:36:15Z

@flynneva PRs are just for pushing changes between branches by PR creator, They are not a tool for collaboration between different people, This is not a best practice.

Besides if you are trying to push your changes to this PR, that's Ok from my side.

flynneva · 2021-03-02T14:39:23Z

@reza-ebrahimi I think we're all on the same page here. I've made a PR to your main branch, I couldn't get the io_context lib to link correctly so maybe you or @JWhitleyWork could help me out there. that pr also fixes the lint errors.

here is a link to it over on @reza-ebrahimi's repo. once merged, the commits/changes will be here on this PR

reza-ebrahimi · 2021-03-02T14:41:44Z

Since this PR is just for udp_driver please just push your linter fixes in a different PR. I'm recommending create another PR for your changes on serial driver.

flynneva · 2021-03-02T14:47:19Z

@reza-ebrahimi to be fair you just mentioned above that you wanted to use the io_context for both the serial_driver and for udp_driver. if this PR still has that goal, you'll need to move the io_context to its own shared library so that a future PR can add this to the serial_driver.

the PR I just made not only fixes the linter errors but also begins to move the io_context to its own shared lib. this PR does not make any changes yet to the serial_driver.

if you'd like i can remove the udp_component bit I added, but really i think it should also be included in this PR. let me know and I can go from there.

reza-ebrahimi · 2021-03-02T14:50:12Z

@flynneva Please just add lint errors fixes to this PR. Your changes contains entities like udp_component that doesn't related to current design in this PR.

flynneva · 2021-03-02T14:59:27Z

there are other minor changes (like moving udp_driver to a sub-directory within the include directory) to follow the standards found here in the docs https://docs.ros.org/en/foxy/Tutorials/Ament-CMake-Documentation.html#id3

JWhitleyWork · 2021-03-02T15:01:15Z

@reza-ebrahimi You're being somewhat difficult about this. @flynneva is trying to help by making your branch work correctly and fixing the problems that you either haven't or won't. It's fine for him to remove the additional code for udp_component because I agree that is orthogonal to this PR but the linter and formatting fixes are required for this PR to be merged.

Once @flynneva completes his PR to your branch, please review his changes. If they are not acceptable, you at least need to fix the linting and formatting errors in this PR.

…ters

JWhitleyWork · 2021-03-03T15:00:20Z

@reza-ebrahimi So here's the root of the problem: You made your changes based on a very old version of the main branch of this repository - back from version 0.0.2. @flynneva then made his changes based on your changes. If I revert the PR that @flynneva made and rebase your branch on the current main, your changes are all fine. However, since @flynneva's changes were based on your changes, which were based on an old version of main, merging his changes causes conflicts. In order to solve this, I have reverted the merge request from @flynneva on your branch. I also reverted the name change from udp_driver to transport_driver (we want to maintain both separate packages - udp_driver and serial_driver). I will now apply the linter fixes to your branch as well and you can review the PR when I'm done to make sure that your changes are as you expect.

reza-ebrahimi · 2021-03-03T15:04:42Z

@JWhitleyWork ~~You can simply copy and paste my last changes to the PR you want to make.~~
@flynneva Changes are older than my last changes, I have made lots of changes on top of them.

My fork is not old @JWhitleyWork , It was based on master when I started to work on, The problem is your force push to my fork 2 days ago: 76b9626

You can check it in logs of this PR:

JWhitleyWork force-pushed the reza-ebrahimi:main branch from 76b9626 to 1be3816 2 days ago

reza-ebrahimi · 2021-03-03T15:22:54Z

@JWhitleyWork I applied linter fixes again. Please review it.

$ colcon build --packages-select udp_driver && colcon test --packages-select udp_driver && colcon test-result --verbose
Starting >>> udp_driver
Finished <<< udp_driver [0.27s]                    

Summary: 1 package finished [0.37s]
Starting >>> udp_driver
Finished <<< udp_driver [10.8s]            

Summary: 1 package finished [10.9s]
Summary: 100 tests, 0 errors, 0 failures, 0 skipped

esteve · 2021-03-08T14:19:33Z

@reza-ebrahimi @JWhitleyWork @flynneva jumping a little late to the party, but I have two questions:

Why use Boost.Asio instead of the non-Boost version of Asio?
In https://github.com/ros-drivers/transport_drivers/pull/31/files#diff-82eba09261662aabb4bbab508fe1f6306f6bd9daaf022774149c6f627f33d6d4R57-R58, it seems to me that only Int32 data can be processed. Is that correct? Does the API accept other types of messages?

flynneva · 2021-03-08T14:44:08Z

@esteve switching to just the plain Asio would make a lot of sense moving forward. Make a PR? And yes I'm working on switching to a more generic receiver and sender node this week

esteve · 2021-03-08T15:06:10Z

@flynneva I started working on a Boost-less version right after sending hitting send 😅 I've submitted a draft PR #39, the only missing part is reimplementing boost::thread_group in C++11/14/17

reza-ebrahimi · 2021-03-08T15:20:45Z

@flynneva The current design supports every message type, Just implementing From & To converters is enough.

For sensor_msgs::NavSatFix message type:

namespace drivers
{
namespace common
{

void convertToRos2Message(const MutSocketBuffer & in, sensor_msgs::NavSatFix & out)
{
}

void convertFromRos2Message(const sensor_msgs::NavSatFix::SharedPtr & in, MutSocketBuffer & out)
{
}

}  // namespace common
}  // namespace drivers

JWhitleyWork · 2021-03-08T17:41:49Z

Much appreciated, gentlemen! Yes, I agree that the stand-alone ASIO is a better choice.

reza-ebrahimi changed the title ~~Implement UdpSender and refactor UdpReceiver~~ Implement UdpSender and UdpReceiver Feb 15, 2021

reza-ebrahimi changed the title ~~Implement UdpSender and UdpReceiver~~ Implement UdpSocket for both Syn. & Async modes (Send & Receive functionality) Feb 15, 2021

This was referenced Feb 17, 2021

initial udp_component exectuable #24

Closed

Feature/udp component #32

Closed

JWhitleyWork requested changes Feb 25, 2021

View reviewed changes

CHANGELOG.rst Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

JWhitleyWork closed this Mar 1, 2021

JWhitleyWork reopened this Mar 1, 2021

JWhitleyWork force-pushed the main branch from 76b9626 to 1be3816 Compare March 1, 2021 20:36

JWhitleyWork force-pushed the main branch from 8659ff2 to e77d104 Compare March 1, 2021 20:37

JWhitleyWork force-pushed the main branch from 1be3816 to c3dc488 Compare March 1, 2021 20:38

reza-ebrahimi and others added 17 commits March 3, 2021 08:40

Add std_msgs/Int* variants converters

ffdf760

Update converters to ROS2 message in function names

9b94f80

Refactor converters to their ROS2 namespaces in converters directory

3ac4d67

Add common converters header

3566e44

Update licenses

3b46d3a

Reformat code

3899b6a

Remove unnecessary print

f1bdef6

Update google test header includes to library convention

94f4933

Implement unsigned integer and floating point message variants conver…

d078a81

…ters

Fix comment in converters

a9eb38b

Update test and example

092c5d9

Update changelog - Bump major version to 0.1.0

7d01bd9

Update readme

3ea435f

Revert back changelog file

90584fb

Update readme

22dfca4

Move udp_driver to separate package

c85ab19

Revert package name change

0b84856

JWhitleyWork force-pushed the main branch from 120896a to 0b84856 Compare March 3, 2021 14:58

Apply linter fixes

80269fc

JWhitleyWork approved these changes Mar 3, 2021

View reviewed changes

JWhitleyWork merged commit 71fdbe4 into ros-drivers:main Mar 3, 2021

JWhitleyWork mentioned this pull request Mar 7, 2021

Add ability to send UDP/Serial data #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement UdpSocket for both Syn. & Async modes (Send & Receive functionality) #31

Implement UdpSocket for both Syn. & Async modes (Send & Receive functionality) #31

reza-ebrahimi commented Feb 9, 2021 •

edited

reza-ebrahimi commented Feb 9, 2021 •

edited

reza-ebrahimi commented Feb 15, 2021

reza-ebrahimi commented Feb 18, 2021

flynneva commented Feb 19, 2021 •

edited

reza-ebrahimi commented Feb 19, 2021 •

edited

flynneva commented Feb 19, 2021

JWhitleyWork left a comment

flynneva commented Mar 1, 2021

JWhitleyWork commented Mar 1, 2021

flynneva commented Mar 1, 2021

JWhitleyWork commented Mar 1, 2021

reza-ebrahimi commented Mar 2, 2021

reza-ebrahimi commented Mar 2, 2021

flynneva commented Mar 2, 2021 •

edited

reza-ebrahimi commented Mar 2, 2021

flynneva commented Mar 2, 2021 •

edited

reza-ebrahimi commented Mar 2, 2021

flynneva commented Mar 2, 2021 •

edited

reza-ebrahimi commented Mar 2, 2021 •

edited

flynneva commented Mar 2, 2021

JWhitleyWork commented Mar 2, 2021

JWhitleyWork commented Mar 3, 2021

reza-ebrahimi commented Mar 3, 2021 •

edited

reza-ebrahimi commented Mar 3, 2021 •

edited

esteve commented Mar 8, 2021

flynneva commented Mar 8, 2021

esteve commented Mar 8, 2021

reza-ebrahimi commented Mar 8, 2021 •

edited

JWhitleyWork commented Mar 8, 2021

Implement UdpSocket for both Syn. & Async modes (Send & Receive functionality) #31

Implement UdpSocket for both Syn. & Async modes (Send & Receive functionality) #31

Conversation

reza-ebrahimi commented Feb 9, 2021 • edited

reza-ebrahimi commented Feb 9, 2021 • edited

reza-ebrahimi commented Feb 15, 2021

reza-ebrahimi commented Feb 18, 2021

flynneva commented Feb 19, 2021 • edited

reza-ebrahimi commented Feb 19, 2021 • edited

flynneva commented Feb 19, 2021

JWhitleyWork left a comment

Choose a reason for hiding this comment

flynneva commented Mar 1, 2021

JWhitleyWork commented Mar 1, 2021

flynneva commented Mar 1, 2021

JWhitleyWork commented Mar 1, 2021

reza-ebrahimi commented Mar 2, 2021

reza-ebrahimi commented Mar 2, 2021

flynneva commented Mar 2, 2021 • edited

reza-ebrahimi commented Mar 2, 2021

flynneva commented Mar 2, 2021 • edited

reza-ebrahimi commented Mar 2, 2021

flynneva commented Mar 2, 2021 • edited

reza-ebrahimi commented Mar 2, 2021 • edited

flynneva commented Mar 2, 2021

JWhitleyWork commented Mar 2, 2021

JWhitleyWork commented Mar 3, 2021

reza-ebrahimi commented Mar 3, 2021 • edited

reza-ebrahimi commented Mar 3, 2021 • edited

esteve commented Mar 8, 2021

flynneva commented Mar 8, 2021

esteve commented Mar 8, 2021

reza-ebrahimi commented Mar 8, 2021 • edited

JWhitleyWork commented Mar 8, 2021

reza-ebrahimi commented Feb 9, 2021 •

edited

reza-ebrahimi commented Feb 9, 2021 •

edited

flynneva commented Feb 19, 2021 •

edited

reza-ebrahimi commented Feb 19, 2021 •

edited

flynneva commented Mar 2, 2021 •

edited

flynneva commented Mar 2, 2021 •

edited

flynneva commented Mar 2, 2021 •

edited

reza-ebrahimi commented Mar 2, 2021 •

edited

reza-ebrahimi commented Mar 3, 2021 •

edited

reza-ebrahimi commented Mar 3, 2021 •

edited

reza-ebrahimi commented Mar 8, 2021 •

edited