search

ros2 / ros_astra_camera

ROS wrapper for Astra camera
9 stars 12 forks source link

Astra driver revamp #19

Open clalancette opened 7 years ago

clalancette commented 7 years ago

The whole of the astra driver really needs to be revamped for ROS2. It is fairly ugly right now, doesn't follow best practices, and has a lot of code commented out. We should probably start by cleaning up the upstream orbbec driver in ROS1, then proceed to fix up the ROS2 version of it.

rjcortese commented 4 years ago

I'm interested in this (or more generally, getting astra camera to work with the latest ros 2 version) but I'm pretty new to this kind of thing and don't know where to begin. If you could provide some guidance, maybe I can do some of the legwork?

clalancette commented 4 years ago

So there are two fairly large pieces of work that probably need to be done here:

  1. Fix up the upstream OpenNI2 driver. You may notice that we are currently patching that driver (in https://github.com/ros2/ros_astra_camera/blob/ac20ab9eea9ff14d4b2dd841d580cf9b3c875110/astra_openni2-patch.diff), but that's just hiding the problems. It looks like https://github.com/orbbec/OpenNI2/pull/6 actually fixes the issues, but that PR has been open for 1.5 years with no response. The repository that that was originally forked from is https://github.com/occipital/OpenNI2, so the correct thing to do may be to submit that above PR to the occipital repository, and then switch the ExternalProject call in here to point to the occipital repository.
  2. Heavily rewrite this ROS 2 driver. I did this several years ago (before Ardent), I did it quickly, and it was the first ROS 2 driver I did (so I didn't know what I was doing). To really get this up to snuff it probably needs:
    1. Removal of Boost (this goes along with what most of the other ROS 2 packages are doing).
    2. Fixes to make this compile on Dashing and Eloquent (I'm sure this is using outdated APIs). At the very least, parameter configuration has changed heavily since Ardent days.
    3. The AstraDriver class should end up becoming a sub-class of rclcpp::Node, rather than having the node passed in. This will allow it to become composable (which is particularly important for a driver like this).
    4. Swap out the calls to ROS_{WARN,ERROR,INFO,DEBUG} with their RCLCPP counterparts.
    5. All of the code needs a thorough looking over, to remove bits that don't belong and to generally make the code cleaner and easier to read.

I know that is a bit hand-wavy, but its kind of the best I can do for now. If you want to look at a reasonably complex driver that does most of this, take a look at https://github.com/ros-drivers/velodyne/tree/dashing-devel .

If you do decide to do any of this work, I'm happy to do reviews and help merge them. But unfortunately I won't have any time of my own to work on this.

rjcortese commented 4 years ago

Thanks for the write up. I'll give it a shot. For Step 1, I think I need to understand the issues with the upstream driver better but I can probably figure it out by exploring those OpenNI2 repositories. Then Step 2 seems fairly straight forward and doable.

rjcortese commented 4 years ago

Looks like the https://github.com/occipital/OpenNI2 repo is in a similar state... tons of unmerged pull requests :|

I was able to compile OpenNI2 on Ubuntu 18.04 with gcc 7.4 from this repo https://github.com/uncc-visionlab/OpenNI2 which is where that orbbec/OpenNI2#6 request originated. Maybe we could just use it as the ExternalProject?

I'll try to proceed with Step 2 using it.

Karsten1987 commented 4 years ago

One remark I'd like to vocal here is that I think that a camera driver (in fact all drivers) with heavy load messages should actually be a rclcpp_lifecycle::Node instead of a regular node. I could see quite some benefits from having the opportunity to easily start/stop the publishing of these images or pointclouds.

rjcortese commented 4 years ago

Progress update: I've taken an approach of taking https://github.com/lukaszmitka/ros_astra_camera, which works on Dashing, and porting it to Eloquent. I have it compiling without error, all boost dependencies removed and converted to std. I can plug in my camera, run the node, and echo the topics and data seems to be coming through. I tried to sub to the /image topic with image_tools/showimage but that hung forever.... I also need to refactor stuff and make it look like an actual ros2 Eloquent package based on components etc. (steps 2.iii through iv)

clalancette commented 4 years ago

Excellent, thanks for the progress update.

mjcarroll commented 4 years ago

I just realized that I have one of these in my office, so I may be able to help test when it comes to that point.

clalancette commented 4 years ago

Yep, same here, I have one sitting around that I could use for testing.

Hi-Zed commented 3 years ago

Hi all, I have a robot with an Astra Pro that I would like to port to ROS2. I am happy to contribute, but I would like to understand what is the current status of this package.

clalancette commented 3 years ago

It still needs the steps I outlined in https://github.com/ros2/ros_astra_camera/issues/19#issuecomment-572817308 . I've been taking some time to improve the backend OpenNI2 driver (here), but that is going slowly (I'm working on it in my spare time). Other than that, this driver is still pretty old, and so would need a bunch of work to make it work properly on modern ROS 2.

rjcortese commented 3 years ago

Hey all, I haven't made any more progress on this due to combination of me being a noob in this space and I haven't had the time to invest to learn and continue to try.

Hi-Zed commented 3 years ago

@clalancette thanks for the work you are doing with OpenNI2, the last time I had to deal with it, it was a nightmare. I also opened an issue on the original repo (the ROS1 version of the driver) to understand if they have any plan to do an official port to ROS2. Especially considering they have been somewhat active there (this repo is now 100 commits behind). It could be worth it to start again from there to build the ROS2 driver.

pedroperrusi commented 3 years ago

Hello guys, I just opened a PR #31 with the latest updates I made to compile the driver in ROS2 Foxy. I also merged the latest contributions of the original ORBEC repo to ensure my ASTRA PRO cameras would be working as well as in their ROS1 launch file.

This is not yet the case... I have issues with OpenNI and its callbacks. When running the node or the launch file I have these messages of corruption:

...
[astra_camera_node-1]   6441291 WARNING    Read: Depth buffer is corrupt. Size is 511856 (!= 512000)
[astra_camera_node-1]   6466764 WARNING    Depth Frame Buffer overflow! current size: 511856
[astra_camera_node-1]   6466786 WARNING    Depth frame is corrupt!
...

In order to debug it I reintegrated the test file test_wrapper.cpp only to learn the issues are the same in it and the callbacks are never called.

...
6048785 WARNING    Read: Depth buffer is corrupt. Size is 511856 (!= 512000)
6072781 WARNING    Depth Frame Buffer overflow! current size: 511856
6072793 WARNING    Depth frame is corrupt!
...
Number of called to IRCallback: 0
Number of called to ColorCallback: 0
Number of called to DepthCallback: 0

To my surprise the ROS1 version of the test file results in the same messages and errors, although their astrapro.launch works perfectly.

If anyone have insights on how to tacke this issues, I'd appreciate very much.

PS: I still haven't managed to integrate and test the improved version of OpenNI from @clalancette . I can try to do it if it can improve the node and fix these issues.