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

Adding a warning for VGA resolution for depth camera #93

Merged
merged 2 commits into from
Nov 21, 2017

Conversation

nlyubova
Copy link
Member

@mikaelarguedas finally, the warning is available :)

@@ -99,6 +99,7 @@ const sensor_msgs::CameraInfo& getCameraInfo( int camera_source, int resolution
if ( resolution == AL::kVGA )
{
static const sensor_msgs::CameraInfo cam_info_msg = createCameraInfoDEPTHVGA();
ROS_WARN("VGA resolution is not supported for the depth camera, please use QVGA");
Copy link
Member

Choose a reason for hiding this comment

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

As this is logging it should include the the ros header with the logging macros.
Should we rephrase to say QVGA or lower ? (is QQVGA a valid resolution for the depth camera on pepper ?)

Copy link
Member Author

@nlyubova nlyubova Nov 21, 2017

Choose a reason for hiding this comment

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

True that there are no ROS_WARN in Naoqi Driver, it seems that they have preferred to use std::cout.

What kind of macros do you mean? #include <ros/console.h> ?

Agree for "QVGA or lower"

Copy link
Member

Choose a reason for hiding this comment

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

What kind of macros do you mean? #include <ros/console.h> ?

yep 👍

Agree for "QVGA or lower"

👍

@nlyubova
Copy link
Member Author

nlyubova commented Nov 21, 2017

thanks! fixed in a new commit

@@ -27,6 +27,7 @@
* ROS includes
*/
#include <cv_bridge/cv_bridge.h>
#include <ros/console.h>
Copy link
Member

Choose a reason for hiding this comment

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

I think you will have to add rosconsole to the package.xml as a dependency

@mikaelarguedas
Copy link
Member

thanks! fixed in a new commit

👍 One more comment about dependency declaration otherwise I think this is ready to be merged!

@nlyubova nlyubova force-pushed the fix_VGA_warning branch 2 times, most recently from 5ec00ee to 9489698 Compare November 21, 2017 16:58
@nlyubova
Copy link
Member Author

done, in the same commit with fix.

package.xml Outdated
@@ -28,6 +28,7 @@
<build_depend>tf2_geometry_msgs</build_depend>
<build_depend>tf2_msgs</build_depend>
<build_depend>tf2_ros</build_depend>
<build_depend>rosconsole</build_depend>
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the quick fix, one tiny comment: can you place it in alphabetical order in the list ? (it looks like all the other deps are ordered alphabetically) Same below for the run_depend

@nlyubova
Copy link
Member Author

just moved it according to the order

@nlyubova
Copy link
Member Author

squash ?

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @nlyubova

@mikaelarguedas
Copy link
Member

squash ?

Yep, you can use the "Squash and Merge" button from the github UI (arrow next to "Merge pull request")

@nlyubova nlyubova merged commit 56d9cee into master Nov 21, 2017
@nlyubova nlyubova deleted the fix_VGA_warning branch November 21, 2017 17:20
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.

2 participants