ros-drivers / transport_drivers

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

initial udp_component exectuable #24

Closed flynneva closed 3 years ago

flynneva commented 3 years ago

this PR is meant to setup a initial framework for creating a udp_manager node that can provide ROS2 services that can create udp_driver nodes using this library. this should eventually provide a quick and easy way for anyone to use the udp_driver class

flynneva commented 3 years ago

@JWhitleyWork just to knock out two birds with one stone, would you care if I merge in the CI updates with this branch? ive already fixed the copyright & other lint errors on that branch on my repo and it would just be easier for me to merge that into this branch

JWhitleyWork commented 3 years ago

would you care if I merge in the CI updates with this branch?

That's fine. This repo is still under fairly heavy development so I'm not as worried about seperation-of-concerns just yet.

kscottz commented 3 years ago

@flynneva I thought you were going to e-mail me first. It is amazing to come in and see this. We really appreciate it.

flynneva commented 3 years ago

@kscottz no worries! im sorry its taking awhile....im catching up on my knowledge about ROS2 components now. actually doing some changes lastnight/today to this branch.

JWhitleyWork commented 3 years ago

@flynneva No problem! Please let me know if you have any questions. We use them everywhere in Autoware.Auto.

flynneva commented 3 years ago

@JWhitleyWork, i decided to first try to get a dead simple udp_component up and running and then use that in a udp_manager node to provide services for creating it if needed. that way the user can decide if they want just a simple udp_component or a full-on udp_manager with all the gizmos.

im running into an undefined symbol error that i cant seem to track down though when trying to use the ros2 component load command using this new udp_component. im guessing im doing something wrong in the CMakeList file but i cant be sure.

JWhitleyWork commented 3 years ago

@flynneva Can you please give me details of your environment (Ubuntu version / ROS version) and paste the full contents of the error message?

Additionally, you may want to consider adding the service callback to the UdpComponent and just letting rclcpp build an executable for the component automatically instead of having a separate manager. You can do that with a different rclcpp CMake macro:

rclcpp_components_register_node(udp_component
  PLUGIN "udp_driver::UpdComponent"
  EXECUTABLE "udp_node"
)

NOTE: Notice the singular node instead of nodes.

JWhitleyWork commented 3 years ago

@flynneva I figured it out. It's because you don't have definitions for the functions that you declare as part of the UdpComponent class. If you add junk definitions for them in UdpComponent.cpp, the component will load.

flynneva commented 3 years ago

@JWhitleyWork whoops 😓 haha ok i added dummy functions like you said but still getting a weird error referencing the constructor of the component.

ros2 component load /ComponentManager udp_driver udp_component::UdpComponent
Failed to load component: Component constructor threw an exception: expected [integer] got [not set]

this is one thing I wasnt sure about since the UdpComponent class inherits from the UdpDriver class...how should the constructor be setup?

JWhitleyWork commented 3 years ago

@flynneva This error is a bit misleading. The actual problem is that a ROS parameter (which is being pulled in the constructor) is missing - an integer value. Specifically, the constructor of UdpDriverNode that does not take a UdpConfig object is declaring parameters for an IP address (ip) and port (port) and they are required. You can either pass them via the command line or via a params.yaml file. See https://index.ros.org/doc/ros2/Tutorials/Node-arguments/ for details.

flynneva commented 3 years ago

@JWhitleyWork i might be missing something here but ive been trying to load this udp_component via the ros2 component load command. is there a way to pass ros parameters to a component during that step? or do I need to set the parameters before loading the component somehow?

JWhitleyWork commented 3 years ago

@flynneva Setting parameters for a component loaded inside a manager can't be done via the command line - it has to be done with a YAML file. It's much easier to ros2 run the executable that was generated from rclcpp_components_register_node. However, I just noticed that your branch still has the s on nodes which won't throw an error but it also won't generate the executable. Once you change that and rebuild, you can do something like ros2 run udp_driver udp_node --ros-args -p ip:=192.168.1.1 -p port:=9000

flynneva commented 3 years ago

ahhh thats why the executable wasnt showing up. I was so confused - thank you! weird it doesnt throw an error?

flynneva commented 3 years ago

so it looks like the executable and passing arguments at runtime are working and it seems to be initializing OK.

is there something that needs to be done in the constructor to start the driver to receive the packets? I tested it out with a local UDP sensor that is just streaming packets but it doesnt seem like its triggering anything. maybe its just my setup?

i started switching over to the udp_msgs::msg::UdpPacket type, that way we could just publish to ROS any packet that anyone wants to use by default & if someone wants to write their own then they can just use the udp_driver library.

flynneva commented 3 years ago

figured it out - working now to sort out the bugs after calling run from the constructor

flynneva commented 3 years ago

@JWhitleyWork i did a super hacky way of calling the run function to kick off the udp_driver library for receiving a packet. not sure if this is how it should be done for a "generic" udp stream where we just want it to publish straight onto a topic.

im unsure as to how to structure the Packet class so I started moving towards using just a std::vector of bytes that should work for all packets I think. the only problem is when I test it locally I would expect it to print out something from the received packet in the convert function but nothing gets printed. would you mind testing it out and seeing what you get?

JWhitleyWork commented 3 years ago

Here is an example of what a Packet struct might look like: https://gitlab.com/autowarefoundation/autoware.auto/AutowareAuto/-/blob/master/src/drivers/velodyne_driver/include/velodyne_driver/velodyne_translator.hpp#L90

However, since we are currently using this as a "generic" UDP receiver, we can't assume anything about the structure of the data so I would recommend just using a std::vector<uint8_t>.

flynneva commented 3 years ago

for some reason it doesnt seem like the destructor for the UdpComponent class is being called...? debugging it now

flynneva commented 3 years ago

so digging into this tonight it looks like for some reason the convert function thinks that output was never initialized.

the first warn message is from the init_output function checking to see if 1) its actually being called and 2) if we can access anything in the output variable. its actually printing out output.address but since it was just initialized its blank.

the second warning message is something i added to the udp_driver code since get_packet actually returns the size of the packet. im just printing it out there to confirm we are actually receiving something.

the last few prints are from inside the convert method that looks like its being created someplace else entirely and cannot even access any of the previous variables that we confirmed are actually there.

image

im not really sure where to go with this other than to look at how the udp_driver library actually initializes/calls all of its variables/methods. seems like something to do with missing a this somewhere....but not entirely sure.

flynneva commented 3 years ago

to help with debugging this I went ahead and tried to add another test for the udp_driver library using the ros param constructor. this should help going forward

flynneva commented 3 years ago

closing this to branch off of #31 which seems to have a lot of the functionality that i was looking for