/ros_comm

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

Add rosconsole command to change logger levels from command line #576

Closed
wants to merge 7 commits into
base: indigo-devel
from

Conversation

Reviewers
No reviews
Assignees

@gerkey gerkey

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@cmansley @ros-pull-request-builder @dirk-thomas @gerkey
@cmansley
Contributor

cmansley commented Mar 5, 2015

There is only the rqt_logger_level command for changing the logger
level for node. If you are doing remote work without an X session, you
have to send service commands directly or change the logger
configuration. This command line tool allows you to query and set the
logger level.
@cmansley
Add rosconsole command to change logger levels from command line 
There is only the rqt_logger_level command for changing the logger
level for node. If you are doing remote work without an X session, you
have to send service commands directly or change the logger
configuration. This command line tool allows you to query and set the
logger level.
74b590a
@ros-pull-request-builder

This comment has been minimized.

Show comment
Hide comment
@ros-pull-request-builder

ros-pull-request-builder Mar 5, 2015

Member

Can one of the admins verify this patch?
Member

ros-pull-request-builder commented Mar 5, 2015

Can one of the admins verify this patch?
tools/rosconsole/CMakeLists.txt
@@ -121,3 +121,8 @@ if(CATKIN_ENABLE_TESTING)
target_link_libraries(${PROJECT_NAME}-thread_test ${PROJECT_NAME})
endif()
endif()
catkin_python_setup()

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

The package needs a setup.py file for this to work. You might have missed to add it to the repo.
Copy URL
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

The package needs a setup.py file for this to work. You might have missed to add it to the repo.
Show outdated Hide outdated tools/rosconsole/CMakeLists.txt
catkin_python_setup()
catkin_install_python(PROGRAMS scripts/rosconsole
DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION})

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

It probably makes sense to install this into the global bin folder rather then the package-specific libexec folder to be able to run it without rosrun. Instead of using catkin_install_python you can list it as a script in the setup.py file.
Copy URL
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

It probably makes sense to install this into the global bin folder rather then the package-specific libexec folder to be able to run it without rosrun. Instead of using catkin_install_python you can list it as a script in the setup.py file.
Show outdated Hide outdated tools/rosconsole/src/rosconsole/__init__.py
#!/usr/bin/env python
# make sure we aren't using floor division
from __future__ import division, print_function

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

It looks like you need neither of these two.
Copy URL
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

It looks like you need neither of these two.
Show outdated Hide outdated tools/rosconsole/src/rosconsole/__init__.py
if argv is None:
argv=sys.argv
# filter out remapping arguments in case we are being invoked via roslaunch
argv = rospy.myargv(argv)

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

Since all nodes will "race" when being invoked through roslaunch it might not be a good idea to use this script to try to change the logger level since the targeted node might not have started fast enough.
Copy URL
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

Since all nodes will "race" when being invoked through roslaunch it might not be a good idea to use this script to try to change the logger level since the targeted node might not have started fast enough.

This comment has been minimized.

Show comment
Hide comment
@cmansley

cmansley Mar 12, 2015

Contributor

Do you have a suggestion to fix this in the tool? Because it would be a shame to lose out on the functionality of the tool just because of the possibility of abuse.
Copy URL
@cmansley

cmansley Mar 12, 2015

Contributor

Do you have a suggestion to fix this in the tool? Because it would be a shame to lose out on the functionality of the tool just because of the possibility of abuse.

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

You can keep this line in the patch. I just wanted to state that using it in a launch file is imo not recommended. Therefore the comment is a bit misleading.

roslaunch is not designed to handle this and will very likely not be modified to do so. We are aware of that limitation and ROS 2 will for sure address those but that won't help in ROS 1.
Copy URL
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

You can keep this line in the patch. I just wanted to state that using it in a launch file is imo not recommended. Therefore the comment is a bit misleading.

roslaunch is not designed to handle this and will very likely not be modified to do so. We are aware of that limitation and ROS 2 will for sure address those but that won't help in ROS 1.

This comment has been minimized.

Show comment
Hide comment
@cmansley

cmansley Mar 12, 2015

Contributor

Another copy and paste artifact. I can remove the comment.
Copy URL
@cmansley

cmansley Mar 12, 2015

Contributor

Another copy and paste artifact. I can remove the comment.
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

This is a great new feature.

I made a couple of small comments inline. The "biggest" suggestion would be to use argparse instead of custom argv handling. This will likely also automatically improve the usage information for users.
Member

dirk-thomas commented Mar 12, 2015

This is a great new feature.

I made a couple of small comments inline. The "biggest" suggestion would be to use argparse instead of custom argv handling. This will likely also automatically improve the usage information for users.
@cmansley

This comment has been minimized.

Show comment
Hide comment
@cmansley

cmansley Mar 12, 2015

Contributor

I fixed some of the copy and paste errors and I added the setup.py file. Thanks for the feedback.

With regards to argparse, I simply copied what exists in the other default ros command line tools. And at least on older systems, argparse is not part of the default python install.
Contributor

cmansley commented Mar 12, 2015

I fixed some of the copy and paste errors and I added the setup.py file. Thanks for the feedback.

With regards to argparse, I simply copied what exists in the other default ros command line tools. And at least on older systems, argparse is not part of the default python install.
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 12, 2015

Member

Actually nowadays it is part of it. Since this patch targets Indigo which implies Ubuntu Trusty which comes with Python 2.7 which includes argparse by default.

Anyway you don't have to use argparse - it was only meant as a suggestion. A lot of existing tools still use optparse.
Member

dirk-thomas commented Mar 12, 2015

Actually nowadays it is part of it. Since this patch targets Indigo which implies Ubuntu Trusty which comes with Python 2.7 which includes argparse by default.

Anyway you don't have to use argparse - it was only meant as a suggestion. A lot of existing tools still use optparse.
tools/rosconsole/setup.py
packages=['rosconsole'],
package_dir={'': 'src'},
scripts=['scripts/rosconsole'],
requires=['genmsg', 'genpy', 'roslib', 'rospkg']

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 16, 2015

Member

Can you please explain why you used these four dependencies?
Copy URL
@dirk-thomas

dirk-thomas Mar 16, 2015

Member

Can you please explain why you used these four dependencies?

This comment has been minimized.

Show comment
Hide comment
@cmansley

cmansley Mar 16, 2015

Contributor

I copied from all other default tools.
Copy URL
@cmansley

cmansley Mar 16, 2015

Contributor

I copied from all other default tools.
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Mar 16, 2015

Member

Please make new Python code comply with PEP8.
Member

dirk-thomas commented Mar 16, 2015

Please make new Python code comply with PEP8.
@cmansley
pep8 the python scripts 
Note: pep8 does allow an "increase the nominal line length from 80 to
100 characters"
f6ce896

@gerkey gerkey self-assigned this Apr 2, 2015

@gerkey gerkey referenced this pull request Apr 2, 2015

Closed

Add rosconsole command to change logger levels from command line #594

@gerkey

This comment has been minimized.

Show comment
Hide comment
@gerkey

gerkey Apr 2, 2015

Contributor

Thanks for the contribution. I made a few small fixes and created #594 to replace this PR.
Contributor

gerkey commented Apr 2, 2015

Thanks for the contribution. I made a few small fixes and created #594 to replace this PR.

@gerkey gerkey closed this Apr 2, 2015

@cmansley cmansley referenced this pull request Jun 23, 2015

Merged

Fix bug with missing current logger levels #631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment