Trace: Change tr_debug color from gray to bright blue #14576
Conversation
ciarmcom
added
the
release-type: patch
|
@marcuschangarm, thank you for your changes. |
|
Does this still look alright for folks who use white or manila background terminals? |
|
Hard to say how different terminals interpret the inversion. On mine, info's default color and debug's gray are hard to distinguish. Bright blue is still readable on white and it is less bright than info, as intended. Cyan on white is really hard to read though:
|
|
For comparison, here is normal blue on black and white:
|
|
CI started |
|
This is just my opinion that this is not a right direction. Debugs should be non intrusive and you should be able to look away from the terminal and doing other stuff and immediately when you see INFO traces you check what happened and notice that important event happened. then when the errors and warnings come you also immediately notice that your actions is required This comes from our use case where we are doing interoperability events with other vendors where we organise test case between multiple other vendors and have multiple devices and Wireshark captures ongoing so multiple inputs that we need to follow. Errors and warnings are triggers for us to see that usually something is wrong with the test and it is going to fail and we need to start searching the problems using the debugs So this should be either be configurable (With all colors changed to accomodate different setup) and by default the non intrusiveness should be maintained Also runtime configuration would be ok to change the entire colour theme |
| @@ -52,7 +52,7 @@ | |||
| #define VT100_COLOR_ERROR "\x1b[31m" | |||
| #define VT100_COLOR_WARN "\x1b[33m" | |||
| #define VT100_COLOR_INFO "\x1b[39m" | |||
| #define VT100_COLOR_DEBUG "\x1b[90m" | |||
| #define VT100_COLOR_DEBUG "\x1b[94m" | |||
There was a problem hiding this comment.
This may end up into bikeshedding, but did you test with
#define VT100_COLOR_INFO "\x1b[97m"
#define VT100_COLOR_DEBUG "\x1b[37m"
On my eye+terminal combo the end result was readable and it kept text in same color, just different brightness.
No matter what you choose here, someone will not like it. What if one just made these configurable and let one to choose correct colors at build time from their app config?
There was a problem hiding this comment.
Makes sense in a trace-only world, but that part of the concept is that trace can be interleaved with standard non-trace application output (and maybe even command-line input). So that would mean "info trace" would be brighter than non-trace application output.
I guess you could set "bright white" after each trace line, rather than "default"?
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
I tend to agree with that, because there shouldn't normally be anything you care about in debug. I've said for a while the default output level shouldn't even include debug, and then when you do enable it, you often don't really want it to totally drown out the non-debug-level stuff. Having it non-intrusive by default helps you just leave it enabled - you don't have to shut it off to see the main info clearly. I vote for having a config option with a more "loud" output, including clearly visible debug, and possibly including the use of "bright white" that Tero suggested. (But make it just a simple "theme" total setting - per-colour setting would just be annoyingly fiddly). In effect we do have 2 themes already - colour and no-colour. Maybe that can just be extended to a multiway option? |
|
I actually agree with some of the counter points here. Another problem I'm trying to solve here though, is that too often I get sent screenshots from customers with their trace output so having defaults that are readable, without any modifications, would be extremely helpful and save time from asking them for better traces. |
@marcuschangarm to progress, shall we define these via config and keep the default values there? |
|
@0xc0170 keeping the current default values is not going to help me since I have no control over our customer's build and configurations. The whole point is to make it easier to help customers when they send us screen shots or when we have remote debugging sessions. |
|
I wouldn't be adverse to having the default option be a new, louder one, as long as it was easy to switch back to quiet debug for Mika's use case. |
I meant it this way as well, having the default values as they are here |
|
@kjbracey-arm @0xc0170 In that case, how about I
? |
|
I don't think anyone really wants to mess with options for individual colours. No-one's that picky, and if they are they can do local edits. :) Plus it would require people to look up VT100 color codes. My suggestion would be just to have a numbered color theme. Theme 0 = original, color theme 1 = this. You you don't technically even need an ifdef because That would let the C file retain original default behaviour, which I think is the right thing to do as it is used in some non-Mbed OS contexts. But for Mbed OS we can have the JSON file default the Mbed OS option for theme to 1. Help text can list the codes - "0 = original (dark debug); 1 = more prominent blue debug". Random thing that just occurred to me - why is "INFO" cyan in your screenshots? I can't see that in the source. Is that your terminal doing some parsing? |
It is for these non-Mbed customers I would like to have the #ifndefs. Having the default be the new bright color would also benefit these customer engagements.
Yes, for some reason MobaTerm highlights success, fail, error, info in different colors. |
|
Okay, I don't have any particular issue with the default being different for non-Mbed OS. Is this the master repo for such use now? Who'd have say on that? |
|
Pelion uses this repo for non-mbed os builds: https://github.com/PelionIoT/mbed-trace but we probably don't want it to diverge from Mbed OS too much. |
|
What shall we do to fix the default color in Mbed OS? Introduce a new color theme as suggested and set as default for Mbed OS? |
mergify
bot
dismissed stale reviews from Patater and 0xc0170
Pull request has been modified.
platform/mbed-trace/mbed_lib.json
Outdated
| @@ -19,6 +19,12 @@ | |||
| "value": "malloc", | |||
| "macro_name": "MEM_ALLOC" | |||
| }, | |||
| "color-theme": { | |||
| "help": "Set color theme. NULL for bright, 1 for dark.", | |||
There was a problem hiding this comment.
Why the choice of "null" and "1"? Why not "0" and "1"?
- "help": "Set color theme. NULL for bright, 1 for dark.",
+ "help": "Set color theme. 0 for readable, 1 for unobtrusive.",There was a problem hiding this comment.
I was just copying it from a different place. 0/1 sounds good as well.
The gray-on-black color code used for debug level print-out in mbed_trace is hard to read. Bright-blue-on-black increases the brightness of the text without over shadowing the info level default (white-on-black), thus making it easier to read while maintaining the original intention. For original color set MBED_TRACE_COLOR_THEME to 1.
|
CI started |
Jenkins CI Test : ❌ FAILEDBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
CI restarted |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
removed
release-type: patch
Summary of changes
The gray-on-black color code used for debug level print-out in mbed_trace is hard to read.
Bright-blue-on-black increases the brightness of the text without over shadowing the info level default (white-on-black), thus making it easier to read while maintaining the original intention.
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers