-
Notifications
You must be signed in to change notification settings - Fork 21
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
Added support for fixedrate mode #5
Conversation
///@todo add the fixed rate implementation | ||
desired_freq_ = 0; | ||
trigger_mode_ = prosilica::FixedRate; | ||
desired_freq_ = config.trig_rate; |
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.
It it not entirely obvious that the user should set the parameter described as "External triggering rate" when in fixed-rate mode.
What are your thoughts on adding another parameter which is explicitly the frame rate for fixed rate mode?
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.
I feel like a second parameter might be more confusing, especially when not using fixed rate mode. The description of trigger rate could be adjusted to make clear the meaning of this value in fixed rate mode
Is there a reason why this was never implemented previously? Most of the code previously existed to make this function, but it was commented out. |
I'm not sure why this wasn't implemented. I inherited the driver along with the rest of the PR2 stacks. It looks like the driver has moved repositories a few times, and the original change logs have been lost to time... I'm going to spend some time with the dynamic_reconfigure manual tonight and see if I can merge this and clean up the configuration a bit. |
It looks like we could use the dynamic_reconfigure groups API to hide some of the options that aren't related to the current streaming mode: http://wiki.ros.org/dynamic%20reconfigure/Reviews/7/1/11%20Groups%20API%20review%20API%20Review |
I'm going to merge this and see if I can clean up the parameters. |
Added support for fixedrate mode
I've refactored the configuration in c232a54 but I don't have a camera to test with. Can you give it a try for me? |
Sorry it took so long for me to get around to this.
|
Was tested with a Manta G-095C