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

Port the ros bridge to galactic #279

Merged
merged 10 commits into from
Jan 16, 2022
Merged

Port the ros bridge to galactic #279

merged 10 commits into from
Jan 16, 2022

Conversation

mateusz-lichota
Copy link
Contributor

@mateusz-lichota mateusz-lichota commented Jan 11, 2022

This is my initial attempt at porting the ros bridge to galactic. Noteworthy changes:

  • [Update: no longer true. See 4d9baef] the port of fs_msgs is not a submodule, but just the files in a folder. It should probably be later merged into the fs_msgs repo, but I don't know what the right way of doing that would be.
  • I changed the sensor_msgs::msg::Image headers to be the same as the static TF frame ids that are published (this is needed for rviz to be able to display camera image. For the rviz camera plugin to work we would also need to publish a CameraInfo message)
  • [Update: no longer true. See 7a7b6e9] I did not understand what the difference between node handle and a private node handle is, so both are now ported as shared pointers to rclcpp::Node
  • Since in ROS2 the launch files can be written in python I've greatly simplified the cameralauncher and the other launch files
  • UDP_control parameter doesn't yet do anything. I think it should be implemented via apropriate QoS policies.
  • IDK what those AirLib async spinners were, but the code works without them so that's what it is for now.
  • There's no (AFAIK) valid rosdep definition for opencv2 on galactic, but my sysytem installation seems to work fine, so that'll have to do for now
  • I haven't built this on a clean system, so I'm not 100% sure all other rosdeps are specified (although I didn't add any new dependencies, so there's a high chance they are)

On the upside, the whole build is now purely ROS2, without any bridging, catkin or other ROS1 dependencies.
From my initial tests, the lidar, imu, gps, transforms, cameras and sending control commands work. Try it out and lmk if there's something I missed.

@wouter-heerwegh
Copy link
Member

Hi @mateusz-lichota,

Thanks a lot! I'll try to build and run this version later today. Is there a specific reason why the fs_msgs are copied? Other wise we just make it a submodule aswell

@mateusz-lichota
Copy link
Contributor Author

The fs_msgs are copied because they are now built with ament_cmake and rosidl_generators instead of catkin, so we cannot reuse the exact same repo as a submodule, and cannot make changes to the fs_msgs repo because it would break compatibility with the ros1 version. If we want this to be a submodule, we could either make a new repo called fs_msgs2 or a 'ros2' branch on the fs_msgs repo, and I'm rather leaning towards having a new branch, because a large majority of the messages code is unchanged

@wouter-heerwegh
Copy link
Member

wouter-heerwegh commented Jan 11, 2022

I agree, let's make it a separate branch, both will also be a lot easier to merge, since there is not a lot of code anyway

@mateusz-lichota
Copy link
Contributor Author

mateusz-lichota commented Jan 11, 2022

I don't think I can make a separate branch in the fs_msgs repo, as I'm not a collaborator there. Would you mind creating it (as an exact copy of master), so that I can make a pull request to that branch?

@wouter-heerwegh
Copy link
Member

I don't have the right permissions either, @davidoort or @SijmenHuizenga would you be able to add a ros2 branch to the fs_msgs repo?

@SijmenHuizenga
Copy link
Member

SijmenHuizenga commented Jan 11, 2022

Nice work here! @wouter-heerwegh you should have an invitation for access to the fs_msgs repo. With that you should be able to create branches and such.

@wouter-heerwegh
Copy link
Member

Thanks @SijmenHuizenga! @mateusz-lichota a ros2 branch has been made, so you can go ahead and make a pull request

@wouter-heerwegh
Copy link
Member

Just built and tested basic running of the simulator, I had 2 issues for now:

  • When I had built the repo and sourced the install/setup.bash script, I could not for some reason find the package fsds_ros2_bridge. After running the setup.bash file inside the fsds_ros2_bridge folder, I was able to find it. Did you have this issue too?
  • When trying to add a parameter to the launch file (manual_mode), no effect took place. I had this before in some other project, so I added a function that fixed it for me back then.

I do not have a lot of experience with ROS2 so feel free to educate me 😄

@mateusz-lichota
Copy link
Contributor Author

I've had the setup.bash issue you describe. Surprisingly, running local_setup.bash fixed it for me. I've had this issue in another project and never figured out how to solve it, or even what causes it.

@mateusz-lichota
Copy link
Contributor Author

I've made a few improvements. In the commit order:

  1. b72df7a: Instead of first declaring and then getting parameters they are retrieved directly druing declaration
  2. 7a7b6e9: Since nh_ and nh_private_ were pointers to the same object I've removed nh_private_ completely and replaced all usages with nh_
  3. 7c51418: Some messages were published with this->get_clock()->now() timestamp, and some with the airsim_client timestamp. For consistency, and compliance with best practices, I made all timestamps use the airsim_client time.
  4. be17691: On slower computers (like mine) the simulation will run slower than real time, and other nodes running in real time may have timing issues. The issue I've encountered was a robot_localization ekf node incorrectly extrapolating vehicle position from the /testing_only/odom velocities, leading to fatally unstable behaviour of the whole system. This commit introduces a clock publisher, publishing to the /clock topic at 100Hz the airsim_client time. Airsim_client doesn't seem to expose a method to get just the time, so as a workaround I just request GSS data and extract just the timestamp. My testing showed that this change (and setting the use_sim_time parameter on the robot_localization node) made the behaviour much more stable.

Copy link
Member

@wouter-heerwegh wouter-heerwegh left a comment

Choose a reason for hiding this comment

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

Found why the package kept not getting found. The directory to the package did not get added in the AMENT_PREFIX_PATH, which needs an export in the package.xml file.

I haven't been able to test all the sensors yet, but as far as I can see, everything looks fine. So unless @mateusz-lichota you've still got any comments, we can merge this 😄

This was an experiment that shouldn't have ended up on gihtub
@mateusz-lichota
Copy link
Contributor Author

I've reverted settings.json to what they were before I accidentally overwrote them. Other than that, that's all from me and we're ready to merge.

@wouter-heerwegh wouter-heerwegh merged commit 8662f43 into FS-Driverless:master Jan 16, 2022
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.

None yet

3 participants