1. Dataspeed Inc. Avatar Dataspeed Inc.
  2. ROS
  3. velodyne_simulator
  4. Issues

I have created a ROS 2 Foxy port, but can't create a pull request

Issue #27 resolved
Joep Tool created an issue

As the title says, I’ve created a ROS 2 Foxy port of the velodyne description. I thought it would be a good idea to create a pull request for it. However, I can’t create pull requests. I still think it would be valuable to create this, to make it easier to use in ROS 2 Foxy. Would it be possible to create this pull request?

Comments (11)

  1. Kevin Hallenbeck

    I’ll take a look at your Foxy port this week. I’m not sure what would prevent creating a pull request. This repository has had several pull requests in the past.

  2. Joep Tool reporter

    Great, if you have any questions, let me know.

    As for the pull request, when I hover over it, it’s grayed out and says: You do not have permission to create a pull request

  3. Kevin Hallenbeck

    I pushed it to a branch here: https://bitbucket.org/DataspeedInc/velodyne_simulator/branch/joe28965/foxy-devel

    Why did you extract the xacro path to the meshes to an xacro property? Was that necessary?

    It looks like you used an existing plugin and commented out the velodyne_gazebo_plugins package. The goal of this package is to output a PointCloud2 message in the exact format as the live velodyne driver. The generic gazebo plugin is missing the ring and time fields.

    There is another pull request here that updated the C++ plugin code, but I haven’t looked it over yet.

  4. Joep Tool reporter

    For the xacro path, in ROS 2 package:// is no longer properly registered by Gazebo. In other words, it won’t show the meshes when I do that. That’s why I do it like that. I believe in ROS 1 Gazebo also didn’t understand package:// however, it was something the gazebo_ros spawner took care of. EDIT, turns out at some point this was fixed, so it’s unnecessary.

    In regards to the second point, you are right. I was unaware of the actual difference between the two and wanted to bring that up during the PR. It shouldn’t be too hard to convert it, so I can still do that

  5. Joep Tool reporter

    I also believe that the other PR was originally for Dashing and not Foxy. I can check if it works on Foxy as well and otherwise use the best of both to create one proper Foxy branch

  6. Joep Tool reporter

    Is there actually a reason to add the gaussian noise to the plugin and setting the noise of the gazebo sensor to 0? Would it not be easier to just use the gaussian noise provided by gazebo?

  7. Joep Tool reporter

    Okay, so I could make a PR just had to do it from my fork, not the PR tab here. Not very familiar with bitbucket, my bad.

    The PR is here: https://bitbucket.org/DataspeedInc/velodyne_simulator/pull-requests/13

    As stated in the PR, I have made changes to Gonzalo’s branch that DID use the velodyne plugin and both updated deprecation warnings and updated it to be more in line with ROS 2 Foxy standards. I think the combined work is the best of both our versions and should become the leading foxy-devel branch.

  8. Kevin Hallenbeck

    It is beneficial to know the true range before noise for the min/max range exclusion logic.

    I merged your PR and pushed a minor cleanup commit.

    I plan to implement lazy subscribers so Gazebo doesn’t calculated everything if nothing is subscribing to the output. Then we’ll have a combined foxy branch ready to submit to the official buildfarm.

  12. Log in to comment
Assignee
–
Type
enhancement
Priority
major
Status
resolved
Votes
0
Watchers
1