anqixu / ueye_cam

A ROS nodelet and node that wraps the driver API for UEye cameras by IDS Imaging Development Systems GMBH.
Other
60 stars 102 forks source link

ROS2 Port #108

Closed stonier closed 2 years ago

stonier commented 3 years ago

Thought I'd reach out and ask before duplicating effort.

Is anyone aware of a ROS2 port for this driver? Or are there plans to do so here?

anqixu commented 3 years ago

Personally I don't have cycles for this task, but I think it'll be generally beneficial. Maybe the other contributors might be interested?

jmackay2 commented 3 years ago

I have been thinking about this quite a bit recently. This would be great. I originally did some initial work on this but never finished it up (I think I still needed to update all of the ROS parameters and launch files).

Ideally we could do this similar to how other packages have updated to ROS2 and either create a ros2 branch or make ros2 the master branch and make a noetic branch off of the current master.

You are definitely not duplicating effort at this point. I'd be up for giving any help or assistance with this.

stonier commented 3 years ago

I think a ros2 branch would do for now, at least till we're ready to green light it as ready for general consumption.

I can try to seed it with something that is 'roughly working' over the next few days, so we've got a starting point that you can dive in and help iterate with. Do you have a link to the sources you already experimented with (couldn't find anything here or in your fork)?

jmackay2 commented 3 years ago

@stonier My changes were just local at one point so I didn't have a branch. I tried to get back to a similar state this evening just to see if I could, but I didn't get back to a nice build. I did go ahead and push my current state (which is very rough) to the ros2 branch of my fork. Feel free to use it or not. At the moment it just fixed up a lot of the ROS logger errors and changed some ros:: to rclcpp:: as well as some small general cleanup. Things that would still need to be done:

stonier commented 3 years ago

Just an update ... slowly making progress and getting familiar with those cams at the same time. A diff on current progress: https://github.com/stonier/ueye_cam/pull/15. Handling the dynamic parameterisation is causing the largest delta.

So, getting the frame grabber loop working and dynamic parameter updates will I think get it to the point of being operational. At least sufficiently enough for others to start being able to contribute without significantly bombing each other's updates. I'll chime back in then :)

stonier commented 3 years ago

ueye_dynamic_parameters

Dynamic parameters are in!

I've made some fairly non-negligible modifications on the way - some of which address consistency issues with respect to parameterisation, others to help decouple driver and wrapper better so modifications now and in the future will be easier (shouldn't be any regressions, but if there are a couple, hopefully the benefits will outweigh the odd bugfix that is needed).

FYI: I've a list of issues at: https://github.com/stonier/ueye_cam/issues

stonier commented 3 years ago

Questions:

stonier commented 3 years ago

Export IDS Camera Configuration

Added Driver::saveCamConfig(), can be triggered via a dynamic parameter.

ueye_export_ids_camera_configuration

jmackay2 commented 3 years ago

I took a quick look at this but haven't dug in too much. I'll try to get around to it this weekend. I think it makes sense to target Foxy for this, since it is the current stable release and older versions have reached EOL. I noticed a few comments on updates needed for Foxy that we may want to go ahead and put it.

Thanks for all of your hard work on this. You've put in a lot of changes, so it may take me a little bit to look over everything.

stonier commented 3 years ago

Aye, managed to prioritise a 20.04 upgrade on our remote machine with the attached cameras so will be switching this code over to Foxy then.

Inbetwixt this and that, trying to work out why this doesn't just work out of the box as a component and bugfixing that.

stonier commented 3 years ago

FYI @ChrisFajardo-TRI - this is the upstream ROS2 port discussion thread.

stonier commented 3 years ago

Update: ueye_cam as a ros2 component working (https://github.com/stonier/ueye_cam/pull/31)!

stonier commented 3 years ago

Update: Upgraded to Foxy. There are three branches there now:

ros2 is currently synchronised with ros2_foxy. Am kickstarting an experiment with a binary release on the build farm - https://github.com/ros/rosdistro/pull/30030. We can update that when we move the ros2 branch into this repo.

anqixu commented 3 years ago

@stonier Can you please have a look at buildfarm issues? I quickly scanned through the logs, it seems that the minimal IDS driver is being downloaded, but then perhaps the installation directory is bad hence causing error?

https://build.ros2.org/job/Fbin_uF64__ueye_cam__ubuntu_focal_amd64__binary/1/console https://build.ros2.org/job/Fbin_ubv8_uFv8__ueye_cam__ubuntu_focal_arm64__binary/1/console

stonier commented 3 years ago

Oh, indeed. Should have expected something like this. CMake and bloom are getting more rigorous. I just had to create an exception to handle the header files appropriately going from CMake 3.10 to 3.16 (bionic -> focal).

I'll have to dig into this one. It's building fine, installing fine (actually doesn't install the IDS headers and libraries for obvious reasons, leaves them in the build directory), but trips up at deb'ify time.

Going to take a little while though - need to relearn how to deb'ify locally and see if I can reproduce I think. I might unwind it from rosdistro in the meantime. Any chance you can spin up a ros2 branch off master here for me so I can PR my forked branch to that (or add me to the repo - willing to help, but can't take the lead as I'll be mostly transferring this project to another developer soon)? I like the idea of keeping it here - this is where folks come for ueye_cam magic.

jmackay2 commented 3 years ago

@stonier I was testing this today with the dynamic reconfigure options, and it would sometimes crash if a parameter was turned on and then back off. This seemed to be happening with the flip_horizontal.

At this point I think we should go ahead and merge in your work and we can make any issues here and build off of it.

jmackay2 commented 3 years ago

@stonier I just made a new foxy branch off of master. Feel free to make a PR there.

stonier commented 3 years ago

@stonier I was testing this today with the dynamic reconfigure options, and it would sometimes crash if a parameter was turned on and then back off. This seemed to be happening with the flip_horizontal.

I'll try to reproduce tomorrow. Didn't actually test that one.

stonier commented 3 years ago
jmackay2 commented 2 years ago

With the foxy branch in, I think we can go ahead and close this. Specific ROS2 issues/upgrades can be created in separate tickets.

stonier commented 2 years ago

Excellent. Created a couple of issues covering the critical problems.