search

ros-drivers / transport_drivers

A set of ROS2 drivers for transport-layer protocols.
Apache License 2.0
92 stars 56 forks source link

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

Closed reza-ebrahimi closed 3 years ago

reza-ebrahimi commented 3 years ago

This PR:

This PR closes issue #12.

reza-ebrahimi commented 3 years ago

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 commented 3 years ago
reza-ebrahimi commented 3 years ago

Current PR Status:

flynneva commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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?

flynneva commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

reza-ebrahimi commented 3 years ago

@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 commented 3 years ago

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

flynneva commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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.

flynneva commented 3 years ago

just made a new one with udp_component removed. i still left the io_context in its own directory though since i think that should be included in this PR.

https://github.com/reza-ebrahimi/transport_drivers/pull/2

im still doing something wrong though trying to create the io_context lib. maybe either of you can spot where i messed up

reza-ebrahimi commented 3 years ago

@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.

@flynneva You are right. But this could happen when all drivers take their places in just one package, I reverted that structure that you know.

For now we have two drivers:

If we want to share IoContext in both drivers, we should share it in root of the repository, it could be a static or dynamic library, or just both drivers reference to it's files statically using linking to object files (normal include).

I will help you to implement serial_driver based on this design if you want.

reza-ebrahimi commented 3 years ago

@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.

@JWhitleyWork I'm appreciate @flynneva helps, There is no problem. All I'm trying to do is to never mistaken a PR with a repository.

I will review @flynneva changes.

flynneva commented 3 years ago

If we want to share IoContext in both drivers, we should share it in root of the repository, it could be a static or dynamic library, or just both drivers reference to it's files statically using linking to object files (normal include).

@reza-ebrahimi so thats what i was trying to do and I think have 90% there....just not linking correctly. maybe you can figure out whats wrong in my PR

reza-ebrahimi commented 3 years ago

@JWhitleyWork Please take a review, All linter errors are fixed.

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

Summary: 1 package finished [0.43s]
Starting >>> udp_driver
Finished <<< udp_driver [6.07s]          

Summary: 1 package finished [6.21s]
Summary: 100 tests, 0 errors, 0 failures, 0 skipped
JWhitleyWork commented 3 years ago

@reza-ebrahimi Thanks for working with @flynneva to get these resolved! However, it looks like one of your branches may have been out-of-date when you started working on it as there are now conflicts that must be resolved before I can merge. See the warning below.

reza-ebrahimi commented 3 years ago

Thanks @JWhitleyWork, let me check it.

reza-ebrahimi commented 3 years ago

@JWhitleyWork I have just one main branch. I see these conflicts here in PR, but I don't know where they are happening.

JWhitleyWork commented 3 years ago

@reza-ebrahimi I'll take a look today.

JWhitleyWork commented 3 years ago

@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 commented 3 years ago

@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: https://github.com/ros-drivers/transport_drivers/commit/76b9626c736a9c5148f6b325810c7c07992a1eaf

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 commented 3 years ago

@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 commented 3 years ago

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

flynneva commented 3 years ago

@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 commented 3 years ago

@flynneva I started working on a Boost-less version right after sending hitting send :sweat_smile: I've submitted a draft PR https://github.com/ros-drivers/transport_drivers/pull/39, the only missing part is reimplementing boost::thread_group in C++11/14/17

reza-ebrahimi commented 3 years ago

@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 commented 3 years ago

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