-
Notifications
You must be signed in to change notification settings - Fork 102
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
Allow parsing of escape sequence in log format #443
Conversation
@clalancette mind looking at how these escape characters would be handled on Windows? 🧇 |
So it turns out that these sequences do indeed work on Windows, but with a caveat. The caveat is that you need to enable the processing of these sequences first. In particular, you need to call a sequence similar to the following:
(obviously I've omitted error-checking and the like). So far, so good; we can easily make a function to do that. I think we should probably call this only when we see a '\x1b' sequence, and otherwise leave it disabled. The hard part is disabling that virtual terminal processing during shutdown. I guess maybe we can track what we've done above, and then disable it during So with those changes, I'd be happy to take this as a new feature. |
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 think we need to have test cases in addition to #443 (comment)
It took me a while to get a Windows System with ROS2 but I implemented the proposed solution and this PR should now also work on Windows systems. |
I tried implementing a test in my previous commit. However, the test always keeps failing, although the feature itself works fine. I spend quiet some time trying to get the test to run, but I was able to do so. I don't have any other ideas where to look for. @fujitatomoya could you maybe take a look at the test and see if you can identify what is going wrong? |
Signed-off-by: Marc Bestmann <[email protected]>
31aca30
to
ca06d1f
Compare
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.
besides, i think this need to rebase.
Signed-off-by: Marc Bestmann <[email protected]>
Signed-off-by: Marc Bestmann <[email protected]>
Signed-off-by: Marc Bestmann <[email protected]>
Signed-off-by: Marc Bestmann <[email protected]>
Signed-off-by: Marc Bestmann <[email protected]>
e0eba69
to
e92e58f
Compare
I fixed the test and also manually tested the final version both on Linux and Windows. If there is anything else that should be changed please tell me. |
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.
lgtm with green CI
@SammyRamone unfortunately windows still failing, https://ci.ros2.org/job/ci_windows/20949/testReport/projectroot/Release/test_logging_output_format/. could you check that? |
I had a look at it and I am not sure why this test works fine on Linux but not Windows. I will further investigate, but I might need some time since I have a lot of other things to do right now and this seems like it will take some time. It might be related to the usage of the raw string but I need to do some testing. |
Signed-off-by: Marc Bestmann <[email protected]>
a7fd4e5
to
e0196e6
Compare
@fujitatomoya I changed the test to use a normal string instead of the raw string. The test now runs successfully in Linux and Windows. I spend some time researching why this makes a difference in Windows but not in Linux, but I have no idea. This environment is just passed on to the standard Python |
@SammyRamone thank you for taking time to work on this. i missed that one, but it aligns with other test cases. lgtm with green CI. |
This is an extendion of #426 which allowed the usage of control codes (i.e. \a, \b, \n, \r, \t) in the log format. This PR adds parsing of the escape character \x1b (also known as \033 or \e).
The motivation is that this allows using Select Graphic Rendition parameters.
Their usage allows setting of bold, faint, italic and/or underline to make the logging output of the screen better readable. This is especially interesting to highlight parts of the logging output (the actual message) while giving less focus to additional information (e.g. the function name, file name or line number).
Example:
![grafik](https://private-user-images.githubusercontent.com/11520148/290552264-9f96de31-4ded-4a31-8d28-c29e5e9d9091.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxNzkzMTEsIm5iZiI6MTczOTE3OTAxMSwicGF0aCI6Ii8xMTUyMDE0OC8yOTA1NTIyNjQtOWY5NmRlMzEtNGRlZC00YTMxLThkMjgtYzI5ZTVlOWQ5MDkxLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjEwVDA5MTY1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTkyMmJiOGRjNmIxMDdhNDNmNzMyYmQxNTY2YzEzZWNlMWVmZWEyMDYxMTgzNTBmM2QxN2NjNjAxMjAzN2MyMmUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.QONoxakEctRaxcWsqSVDo8rVuuYEmPk1G7Kxn-ettLQ)
RCUTILS_CONSOLE_OUTPUT_FORMAT="\x1b[4m{name}\x1b[24m \x1b[1m{message}\x1b[0m \x1b[2m{function_name} \x1b[3m{file_name}:{line_number} \x1b[0m"
Side node:
I chose the '\x1b' representation instead of the '\e' since this would result in the following compiler warning:
warning: non-ISO-standard escape sequence, '\e'
This makes the parsing a bit more complex since we have a 4 character input that is parsed to a 2 character output (while the other control codes are 2->1).
I just decided on this approach to ensure sticking to the ISO standard. However, it would also be fine for me to use '\e'.
I will also add an update to the documentation if the PR is accepted.