-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[computer vision] Opticflow multiple cameras #2618
[computer vision] Opticflow multiple cameras #2618
Conversation
…s' into opticflow_multiple_cameras
Thank you for this important update. |
Perhaps me calling it camera id is not the clearest name. In order to run the cv functions and know which opticflow struct to use, where to write the result etc. some form of identification needs to be passed to the Originally I didn't want to hardcode an id for front/bottom camera (to support an arbitrary number of devices), so technically when This index/id being so free to choose could cause some confusion on how it's supposed to be used, so if linking the camera name to it is a better option I can change it. It's not the cleanest solution at the moment, as there are no checks/safeguards on this extra function parameter, so I am very much open to suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra information.
Hi Matteo, I'm currently going through your pull request. Why did you choose to set the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments, but looks good to me
I thought it would be better to avoid hardcoding Another option that I thought of was to create a static variable in cv.c that would start at 0 and increment every time a new device is added, so no extra arguments would need to be passed to |
Agreed, let's stick with your solution then :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only two minor comments, pprzlink, and then it's ready to be merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Could someone (@gautierhattenberger ?) take a short look at the pprzlink changes before merging? Cheers
Proposal for #1772
The files currently compile, I plan to test the opticflow module on a Bebop with both cameras before finalizing the pull request.The modified
cv_opticflow
module is able to run the computer vision functions on both front and bottom camera simultaneously. I tested it on a Bebop 1, I added the required defines to thebebop_opticflow_indoor.xml
airframe to showcase the module usage, but perhaps making a whole new airframe might be better.I have not found a clever solution on how to avoid writing a lot of duplicate code for the defines in
cv_opticflow.xml
(and as a consequence in the initialization of the multipleopticflow_t
structs). Originally I intended for the module to be able to run on an arbitrary number of cameras, but due to how Paparazzi handles module defines I had to hardcode it to front and bottom cameras only in a lot of places.There are a lot of changes that have been made already, the next step would be to change the opticflow ABI message, since (from my understanding) it's not currently possible to distinguish if the message contains results from the front or the bottom camera.