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

Refactor usb cam library with no ros deps #209

Conversation

flynneva
Copy link
Collaborator

  • Refactor usb_cam library to be bit more understandable
  • Add some basic unit tests
  • Test to see if we can remove all ROS deps from usb_cam lib and only use ROS in the node wrappers

@flynneva
Copy link
Collaborator Author

flynneva commented Nov 23, 2022

@twdragon I refactored the code just a bit to be a bit more readable and added the beginning of some unit tests that we can add onto later.

The usb_cam library is already pretty well separated actually from the ROS code and from what I can tell only uses ROS-specific code for the timestamp and the logger. These should be pretty easy to get rid of I think but am interested in your feedback here on the best way to do that.

Should we try and template the code? So we could specify a ROS-clock type to use in usb_cam? Or should we try and return the time in some other way (e.g. std::chrono::time_point) and provide converter functions for each ROS-specific version?

The second option might be easier to implement, and also give us the benefit of being able to use the usb_cam library without ROS at all which should make integration tests a bit easier.

@twdragon
Copy link
Collaborator

@flynneva great work! Thank you a lot! Please give me some time to examine the code and adapt it, then it would be also a great unified release!

@flynneva flynneva force-pushed the 203-refactor-usb-cam-library-with-no-ros-deps branch from 4e967da to 7580186 Compare November 24, 2022 00:19
@flynneva flynneva force-pushed the 203-refactor-usb-cam-library-with-no-ros-deps branch from 7580186 to 1d7fda4 Compare November 24, 2022 00:33
@flynneva
Copy link
Collaborator Author

flynneva commented Nov 24, 2022

@twdragon I added some more tests and I think the usb_cam.hpp and usb_cam.cpp now have no ROS deps...so this means we could reuse this library for both ROS 1 and ROS 2 if needed / not have to change it if ROS 2 changes some API in the future.

To make this happen I had to remove the use of ROS 2 rclcpp::Clock and the msgs needed to store the timestamps of each frame. I checked the new implementation and it seems to work...but let me know what you think / if you see anything that could be improved.

One thing I still am unsure about is how to "simulate" a /dev/ttyUSB0 camera for integration tests. I'll look into this over the next week to see if there are already testing strategies out there that we could use. UPDATE: this emulation driver looks promising

@twdragon
Copy link
Collaborator

@flynneva great! I will start in some days, thank you for this contribution! I hope to make things work.

twdragon
twdragon previously approved these changes Nov 28, 2022
Copy link
Collaborator

@twdragon twdragon left a comment

Choose a reason for hiding this comment

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

I see it makes the code much mere readable. Thank you, and I will do my best to adapt it

@flynneva flynneva force-pushed the 203-refactor-usb-cam-library-with-no-ros-deps branch 2 times, most recently from acabf65 to aaf3f13 Compare December 2, 2022 04:03
@flynneva
Copy link
Collaborator Author

flynneva commented Dec 2, 2022

@twdragon I squeezed one more change into this PR by adding a simple integration test to round out the first layer of tests we add here. This test can be improved later on once we are able to simulate v4l2 devices on the CI runners.

Could you look it over once more and approve?

@flynneva flynneva force-pushed the 203-refactor-usb-cam-library-with-no-ros-deps branch 2 times, most recently from 232d4f3 to 9f15e49 Compare December 2, 2022 04:18
@flynneva flynneva force-pushed the 203-refactor-usb-cam-library-with-no-ros-deps branch from 9f15e49 to ad88e29 Compare December 3, 2022 03:44
@flynneva flynneva force-pushed the 203-refactor-usb-cam-library-with-no-ros-deps branch 2 times, most recently from d2cec78 to c024178 Compare December 3, 2022 04:42
@flynneva flynneva force-pushed the 203-refactor-usb-cam-library-with-no-ros-deps branch from c024178 to 9b8b413 Compare December 3, 2022 04:43
Copy link
Collaborator

@twdragon twdragon left a comment

Choose a reason for hiding this comment

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

@flynneva great work again! I see an excellent opportunity to recover portability for ROS 1 node too and will have a look very soon! Now I am ready to approve this PR

src/usb_cam.cpp Show resolved Hide resolved
@flynneva flynneva merged commit ae9b985 into ros-drivers:ros2 Dec 5, 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.

2 participants