search

ros-event-camera / metavision_driver

driver for event based cameras using the MetaVision SDK (Prophesee and CenturyArk)
Apache License 2.0
42 stars 12 forks source link

Support for trail filters #50

Closed AndreasAZiegler closed 3 months ago

AndreasAZiegler commented 3 months ago

This PR adds support for the trail filters described here. Successfully tested on an EVKv4 from Prophesse.

Note: So far I only implemented it for ROS1.

berndpfrommer commented 3 months ago

Hi Andreas, Thanks for the PR! First let me apologize for leaving an earlier PR sitting that I just merged. I had simply forgotten to merge it in, it was ready to go. Apparently it had some conflicts, but it should be easy to resolve. Second, I have some stylistic changes that I'd like to see before we merge this. Sorry for being peculiar, but I think the code can be written shorter with minimum performance penalty. A smaller code base is easier to maintain, and has less potential for bugs. Please see feedback in the comments. Just to clarify: this is a valid and good PR, and I want it going into the driver, it just needs some polishing.

berndpfrommer commented 3 months ago

Oh, and you wrote you only did ROS1, but the PR has only ROS2. That is fine. ROS1 is basically end-of-life.

AndreasAZiegler commented 3 months ago

@berndpfrommer Thanks a lot for the valuable feedback, I'm always happy to learn and improve. It has been a while for me since the last PR with a "senior" developer as reviewer. I believe I addressed all of your comments. Please let me know if I missed some. I hope the test pass as well.

berndpfrommer commented 3 months ago

Tests fail due to formatting errors. You can check in your workspace before committing by:

colcon test --packages-select metavision_driver
colcon test-result --verbose

Also please see additional comments: bomb out if invalid filter type is specified. You want to get the user's attention when they are feeding in nonsense parameters. Another few nitpicks as well, see new comments.

AndreasAZiegler commented 3 months ago

Is there something I can help to get the test passing? Looks like there some issues in logging.h?