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

Change color stream default to 30fps #164

Merged
merged 1 commit into from
Dec 15, 2016

Conversation

mdhorn
Copy link

@mdhorn mdhorn commented Dec 15, 2016

Improve reliability of the camera by reducing the color stream
to 30fps by default. This will be the highest frame rate which will
be validate with VGA or higher resolutions for use with ROS.

Default files will no longer use the librealsense preset modes
as they will be depreciated in the future.

@mdhorn mdhorn force-pushed the default-color-30fps branch from 11d19fd to e842cf0 Compare December 15, 2016 18:11
<!-- These 'arg' tags are just place-holders for passing values from test files.
The recommended way is to pass the values directly into the 'param' tags. -->
<arg name="enable_depth" default="true" />
<arg name="enable_ir" default="true" />

Choose a reason for hiding this comment

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

enable_ir and enable_ir2 defaults should be 'false'

@@ -4,7 +4,34 @@
<arg name="camera_type" default="F200" /> <!-- Type of camera -->
<arg name="serial_no" default="" />
<arg name="usb_port_id" default="" /> <!-- USB "Bus#-Port#" -->
<arg name="manager" value="$(arg camera)_nodelet_manager" />
<arg name="manager" value="nodelet_manager" />

Choose a reason for hiding this comment

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

I realize you didn't change this, but should this be default= instead of value=? If it is default, it can be overridden by the command line if it is value, it is essentially hard-coded to "nodelet_manager". See this: http://wiki.ros.org/roslaunch/XML/arg. Same would apply for all the camera types.

Copy link
Author

Choose a reason for hiding this comment

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

@reaganlo I thought we had made this a value instead of default to prevent the user from changing the name of the nodelet manager?

Choose a reason for hiding this comment

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

Yes, we have defaults only for those fields that are likely to be changed by users.

Choose a reason for hiding this comment

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

@mdhorn On second thoughts I think lets keep it consistent and have everything as "defaults". I feel there is no harm in doing so.

Copy link
Author

Choose a reason for hiding this comment

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

@reaganlo I'm reformatting the contents of the default files. I've tested, and I can switch to manual mode and ONLY set manual mode and the color FPS -- much simpler default file.
I'll be resubmitting this PR shortly.

Copy link

@reaganlo reaganlo left a comment

Choose a reason for hiding this comment

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

With these changes, do we even need to have the "modify_params" launch files for all the cameras? Because the "default" launch files itself shows how params can be set in our cameras.

Copy link

@reaganlo reaganlo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@mkhansenbot mkhansenbot left a comment

Choose a reason for hiding this comment

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

Approved 👍

Improve reliability of the camera by reducing the color stream
to 30fps by default. This will be the highest frame rate which will
be validate with VGA or higher resolutions for use with ROS.

Default files will no longer use the librealsense preset modes
as they will be depreciated in the future.
@mdhorn mdhorn force-pushed the default-color-30fps branch from 6928e2b to fdc341a Compare December 15, 2016 23:16
@mdhorn mdhorn merged commit 687188b into IntelRealSense:indigo-devel Dec 15, 2016
@mdhorn mdhorn deleted the default-color-30fps branch December 15, 2016 23:27
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.

3 participants