Add rosconsole echo by efernandez · Pull Request #1324 · ros/ros_comm

efernandez · 2018-02-11T18:48:28Z

This PR adds a new command to rosconsole that allows to print the logger messages from the /rosout or /rosout_agg topic using only the CLI.

This new command is echo, so we can simply call rosconsole echo to see all the logger messages. The command takes multiple optional arguments that modify its behaviour, as shown below:

$ rosconsole echo -h
usage: rosconsole echo [-h] [-1] [--topic TOPIC] [--nocolor] [-d]
                       [FILTER] [LEVEL]

Print logger messages

positional arguments:
  FILTER         regular expression to filter the logger name (default: .*)
  LEVEL          minimum logger level to print (default: warn)

optional arguments:
  -h, --help     show this help message and exit
  -1, --once     prints one logger message and exits
  --topic TOPIC  topic to read the logger messages from (default: /rosout)
  --nocolor      output without color
  -d, --detail   print full logger details

This video shows how it works using two test nodes from https://github.com/efernandez/rosconsole_echo_test:

The two positional arguments supports auto-complete hints thanks to ros/ros#168.

This is slightly inspired on @guillaumeautran 's rosdiagnostic tool. I tried to make it closer to how the other rosconsole commands are though, and also auto-complete friendly.

efernandez · 2018-02-11T18:51:12Z

FYI @mikepurvis @dirk-thomas

dirk-thomas

The CI failure is unrelated to this patch and has been addressed by #1325. Please rebase this branch to the latest commit of the targeted branch so that CI can pass.

dirk-thomas · 2018-02-13T00:10:18Z

clients/rospy/src/rospy/rosconsole.py

+from datetime import datetime
+from dateutil.tz import tzlocal
+
+from argparse import ArgumentParser


It seems it would make sense to use argparse for the whole script. But I wouldn't couple that with the feature you are adding here. It is certainly good to use it for the new echo verb.

Do you mean I should make it local to the function where it's being used?

I honestly don't think it makes a big difference, but I can do that if that's what you're requesting here.

Sorry, for not being clearer. I wasn't referring to the location of the import statement. Instead I was trying to point out that the first command line argument of rosconsole is inspected "manually" by looking at sys.argv and then argparse is only used for the new echo verb. It would be nice if argparse would be used for all arguments including the decision/selection based on the verb.

But that is clearly outside the scope of this PR.

Gotcha! Yes, that makes a lot of sense, but definitely not a change for this PR.

dirk-thomas · 2018-02-13T00:10:50Z

clients/rospy/src/rospy/rosconsole.py

+
+    parser.add_argument('--nocolor', action='store_true', help='output without color')
+
+    parser.add_argument('-d', '--detail', action='store_true', help='print full logger details')


Often such an option is called -v / --verbose.

dirk-thomas · 2018-02-13T00:12:37Z

clients/rospy/src/rospy/rosconsole.py

+
+    rospy.spin()
+
+    del rosconsole


Deleting the local variable doesn't do anything. Since the script is exited after this function call anywhere there is nothing which needs to be done. As an alternative - if the goal is to also be able to call this function without exiting right after it - the RosConsoleEcho class could have a deregister method which stops the subscription.

dirk-thomas · 2018-02-13T00:14:46Z

clients/rospy/src/rospy/rosconsole.py

+
+    parser.add_argument('level', metavar='LEVEL', type=str, nargs='?', default='warn',
+                        choices=RosConsoleEcho.STRING_LEVEL.keys(),
+                        help='minimum logger level to print (default: %(default)s)')


Since both positional arguments filter and level are optional it is not very clear how to pass e.g. only a level. Maybe consider to change either of them into -- option.

The main reason I made them positional arguments is because I didn't know how to provide auto-complete hints with rosbash when I have something like -l (or --level).

I initially had --filter and --level. If I only have --level, do you if it's possible to have auto-complete hints in ros/ros#168?

I think it is possible. Manually implemented completion is usually a 🤐 In newer code I usually rely on argcomplete but that would be a drastic change.

I will leave it up to you if you want to change the positional arguments to options.

I've had a quick look at bash completion and it seems it's definitely possible. I don't know how easy and clean it'd be though.

Since I really like the idea of having --level, so we can change the level only, I'd give it a try.
I'll come back to you soon, after updating both PRs.

Changed to -l (--level) and update the rosbash auto-complete function to support it in ros/ros#168

dirk-thomas · 2018-02-13T00:16:30Z

clients/rospy/src/rospy/rosconsole.py

+                                 self.LEVEL_STRING_COLOR
+
+        self._filter = re.compile(options.filter)
+        self._level = self.STRING_LEVEL[options.level.lower()]


You could avoid the definition of STRING_LEVEL with the following code:

self._level = getattr(Log, options.level.upper())

Cool! Done.

dirk-thomas · 2018-02-13T00:26:04Z

clients/rospy/src/rospy/rosconsole.py

+
+    def __init__(self, options):
+        self._level_string_map = self.LEVEL_STRING_NO_COLOR if options.nocolor else \
+                                 self.LEVEL_STRING_COLOR


In order to reduce the two variables LEVEL_STRING_NO_COLOR and LEVEL_STRING_COLOR you could use the following more compact logic (just written in the browser):

LEVEL_COLOR = { 'DEBUG': 92, 'INFO': 97, 'WARN': 93, 'ERROR': 91, 'FATAL': 95, } self._level_string_map = {getattr(Log, level): level.ljust(5) for level in LEVEL_COLOR.keys()} if not options.nocolor: [ self._level_string_map[getattr(Log, level)] = '\033[' + str(color) + 'm' + self._level_string_map[getattr(Log, level)] + '\033[0m' for level, color i LEVEL_COLOR.items() ]

In the (unlikely) event of another level being added in the future the lines to update would be much lower.

Makes sense.

I've done a different implementation though. Let me know if it looks good to you.

And some minor fixes related to the order of the levels when printed by argparse when the user introduces a wrong level.

efernandez · 2018-02-16T14:59:41Z

clients/rospy/src/rospy/rosconsole.py

+        rospy.Subscriber(options.topic, Log, callback)
+
+    def _stringify(self, level):
+        string = level.ljust(5)


@dirk-thomas I feel I should change this to string = level.ljust(max([len(level) for level in LEVEL_COLOR.keys()]))

Sounds good?

Done.

Also rebased onto lunar-devel, although the tests were already passing before that.

efernandez · 2018-02-19T19:05:23Z

Let me know if there is something else to do, or otherwise I'll squash all commits into one before you merge this.

dirk-thomas · 2018-02-26T21:27:06Z

Thank you!

This reverts commit 94aaaec.

efernandez mentioned this pull request Feb 11, 2018

Add rosconsole echo ros/ros#168

Closed

dirk-thomas added the enhancement label Feb 12, 2018

dirk-thomas reviewed Feb 13, 2018

View reviewed changes

dirk-thomas added requires-changes more-information-needed and removed requires-changes labels Feb 13, 2018

efernandez commented Feb 16, 2018

View reviewed changes

efernandez added 7 commits February 19, 2018 14:03

Add rosconsole echo

2121b27

Change detail argument to verbose

e30cf18

Remove explicit del call

38d7d58

Use getattr instead of custom STRING_LEVEL

941e572

Simplify level_string_map init

cbf04de

Change level positional arg to flag

47e303b

Pre-compute level max length

8a0d767

efernandez force-pushed the add_rosconsole_echo branch from fba7c5a to 8a0d767 Compare February 19, 2018 19:03

dirk-thomas removed the more-information-needed label Feb 26, 2018

dirk-thomas merged commit 94aaaec into ros:lunar-devel Feb 26, 2018

dirk-thomas added a commit that referenced this pull request Apr 3, 2018

Revert "Add rosconsole echo (#1324)"

2d0a944

This reverts commit 94aaaec.

dirk-thomas mentioned this pull request Aug 20, 2018

changes between 1.12.13 and 1.13.7 for backporting #1490

Merged


		parser.add_argument('--nocolor', action='store_true', help='output without color')

		parser.add_argument('-d', '--detail', action='store_true', help='print full logger details')

Conversation

efernandez commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efernandez commented Feb 11, 2018

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efernandez Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efernandez commented Feb 19, 2018

Uh oh!

dirk-thomas commented Feb 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

efernandez commented Feb 11, 2018 •

edited

Loading

efernandez Feb 14, 2018 •

edited

Loading