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.

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

Support for trail filters #50

Merged
merged 28 commits into from
Aug 13, 2024
Merged

Support for trail filters #50

merged 28 commits into from
Aug 13, 2024

Conversation

AndreasAZiegler
Copy link
Contributor

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.

AndreasAZiegler and others added 19 commits November 9, 2022 16:56
@AndreasAZiegler
Added bias_fo for Gen4 sensor called bias_fo_n.
46fdc7a
@AndreasAZiegler
Added serial and trigger_in_mode parameters.
eb12479
@AndreasAZiegler
Warn that bias_pr is not supported for Gen4.1 and IMX636.
aaa899f
@AndreasAZiegler
Fixed trigger pins config in nodelet mode.
7390be8
@AndreasAZiegler
Added launch files for EVK1 and EVK2.
2225696
@AndreasAZiegler
Our bias settings:
5966670 
gen4_CD_standard are the standard biases for EVK1
gen41 are tuned biases for the EVK2
@AndreasAZiegler
Allow to set the name of the node.
bf30a54
@AndreasAZiegler
Added sync_mode and name parameter.
01b37b2
@AndreasAZiegler
Added parameters to better control the node.
2dc5a4d
@AndreasAZiegler
Added bias_fo_n
ac91627
@AndreasAZiegler
Fixed typo.
4833918
@AndreasAZiegler
Also start primary node since otherwise hardware sync signal is missing.
2a37cc7
@AndreasAZiegler
Merge remote-tracking branch 'az/master'
c779836
@AndreasAZiegler
Merge branch 'master' of github.com:AndreasAZiegler/metavision_ros_dr…
646cc51 
…iver
@AndreasAZiegler
Added functionality th activate trail filters.
88d5617
@AndreasAZiegler
Reverted some old, unrelated changes.
84750ba
@AndreasAZiegler
Trail filter needs to be activated after the camera is configured.
1e87077
@AndreasAZiegler
Some refactoring.
8d44769
@AndreasAZiegler
Sync with main repo.
4035b74
@berndpfrommer
Copy link
Collaborator

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 berndpfrommer self-assigned this Aug 12, 2024
berndpfrommer
berndpfrommer requested changes Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@berndpfrommer berndpfrommer left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR, please see review comments.

include/metavision_driver/metavision_wrapper.h Outdated Show resolved Hide resolved
include/metavision_driver/metavision_wrapper.h Outdated Show resolved Hide resolved
src/driver_ros2.cpp Outdated Show resolved Hide resolved
src/metavision_wrapper.cpp Outdated Show resolved Hide resolved
src/metavision_wrapper.cpp Show resolved Hide resolved
@berndpfrommer
Copy link
Collaborator

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

@AndreasAZiegler
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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

@berndpfrommer berndpfrommer merged commit f7a4fb6 into ros-event-camera:master Aug 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berndpfrommer berndpfrommer berndpfrommer approved these changes

Assignees

@berndpfrommer berndpfrommer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AndreasAZiegler @berndpfrommer