8000 ROS2 Port · Issue #108 · anqixu/ueye_cam · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ROS2 Port #108

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.

Sign up for GitHub

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

Jump to bottom
Closed
stonier opened this issue May 10, 2021 · 21 comments · Fixed by #110
Closed

ROS2 Port #108

stonier opened this issue May 10, 2021 · 21 comments · Fixed by #110
Labels
enhancement ros2

Comments

@stonier
Copy link
Collaborator
stonier commented May 10, 2021

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
Copy link
Owner
anqixu commented May 10, 2021

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
Copy link
Collaborator

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
Copy link
Collaborator Author
stonier commented May 11, 2021

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
Copy link
Collaborator

@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:

  • Make it a Component (ROS2's version of the nodelet that we currently use)
  • Update all of the ROS parameters and dynamic reconfigure
  • Fix the isZero and is_zero checks
  • Clean up the old nodehandles
  • Update the launch files to ROS2 python
  • Add some type of logging output back into the ueye_cam_driver
  • Fix whatever bugs for publishers and messages that I fudged (particularly the service)
  • Update the github workflow to use ROS2 (probably foxy and rolling)
  • General cleanup (removing some of the old ROS1 commented out code)

@stonier
Copy link
Collaborator Author
stonier commented May 27, 2021 •
edited
Loading

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

  • Have got a connection established
  • Pubs, subs and services are fixed
  • Nodehandle and local nodehandles are getting ... handled
  • Ros Time / Clock handling fixed
  • Parameter defaults, overrides on startup working
  • A parameter event handler is in, but not doing anything as yet (~dynamic_reconfigure)
  • A single launcher working, with param configuration from yaml

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
Copy link
Collaborator Author
stonier commented Jun 6, 2021 •
edited
Loading

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).

  • Shifted camera parameter data structures into the driver (single source of truth). The ROS wrapper just exposes / manipulates these.
  • Moved batch setting of parameters from the ROS layer into the Driver layer
  • Consolidated batch setting in one place (these are used at launch and on reconfigure, previously these traced separate paths)
  • Using exceptions for error handling in the Driver, this lets the library emit error messages to the user of the library so that they can log it with their system of choice. This removes a ROS dependency for the driver, which should be pure c++ and avoids annoying users with uncontrolled debug/warning/error spam. Not completely transitioned yet.
  • Adopting a fail-hard-fail-fast + educate policy with respect to setting parameters. The previous implementation accommodated invalid parameterisations by re-trying with something more robust (e.g. mono8, or image_top=-1). I find the latter frequently leads to masking of surprising bugs. In addition, it makes it much harder to get consistency right when the driver modifies incoming requests under the hood of it's setXYZ methods - you have to remember to circle back with the updated value.

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

@stonier
Copy link
Collaborator Author
stonier commented Jun 6, 2021

Questions:

  • Is not having an intrinsics file considered fatal?
  • Good time to push this onto a ros branch here?

@stonier
Copy link
Collaborator Author
stonier commented Jun 7, 2021 •
edited
Loading

Export IDS Camera Configuration

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

ueye_export_ids_camera_configuration

@jmackay2
Copy link
Collaborator

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
Copy link
Collaborator Author
stonier commented Jun 18, 2021 •
edited
Loading

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
Copy link
Collaborator Author
stonier commented Jun 18, 2021

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

@stonier
Copy link
Collaborator Author
stonier commented Jun 19, 2021 •
edited
Loading

Update: ueye_cam as a ros2 component working (stonier#31)!

@stonier
Copy link
Collaborator Author
stonier commented Jun 25, 2021 •
edited
Loading

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 - ros/rosdistro#30030. We can update that when we move the ros2 branch into this repo.

@jacobperron jacobperron mentioned this issue Jun 25, 2021
Merged
@anqixu
Copy link
Owner
anqixu commented Jun 28, 2021

@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
Copy link
Collaborator Author
stonier commented Jun 29, 2021 •
edited
Loading

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@stonier
Copy link
Collaborator Author
stonier commented Jun 29, 2021

@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
Copy link
Collaborator Author
stonier commented Jun 29, 2021

@stonier stonier mentioned this issue Jun 29, 2021
Merged
@jmackay2 jmackay2 linked a pull request Jun 30, 2021 that will close this issue
Initial ROS2 Port #110
Merged
@jmackay2
Copy link
Collaborator

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

This was referenced Jul 15, 2021
Open
Open
@stonier
Copy link
Collaborator Author
stonier commented Jul 15, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement ros2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Initial ROS2 Port stonier/ueye_cam
3 participants
@stonier @anqixu @jmackay2
0