| Message ID | 20190408084558.2979-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | v4l2-compliance colors | expand |
Hi Philipp, On 4/8/19 10:45 AM, Philipp Zabel wrote: > Hi, > > not sure if anybody finds this as useful as I do to spot compliance > failures and warnings in a sea of OKs more easily, but this patch adds > some color escape codes to the output of v4l2-compliance. It marks "OK" > green, "warn" bold, and "fail" / "FAIL" bright red if the output is a > terminal. I would have preferred to mark warnings yellow, but that > doesn't work well on black-on-white terminals. Hmm, I hate colors myself :-) So I would prefer if an option is added to explicitly enable colors. And the check for stdout can be replaced by a check for this new option. Also, the same option and behavior should be added to cec-compliance as well. I propose the option -C, --show-colors. And don't forget to update the man pages for both utils! Regards, Hans > > regards > Philipp > > Philipp Zabel (2): > v4l2-compliance: use warn() in warn_once() > v4l2-compliance: add colors > > utils/v4l2-compliance/v4l2-compliance.cpp | 11 ++++++++--- > utils/v4l2-compliance/v4l2-compliance.h | 22 +++++++++++++--------- > 2 files changed, 21 insertions(+), 12 deletions(-) >
Em Mon, 8 Apr 2019 11:05:20 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > Hi Philipp, > > On 4/8/19 10:45 AM, Philipp Zabel wrote: > > Hi, > > > > not sure if anybody finds this as useful as I do to spot compliance > > failures and warnings in a sea of OKs more easily, but this patch adds > > some color escape codes to the output of v4l2-compliance. It marks "OK" > > green, "warn" bold, and "fail" / "FAIL" bright red if the output is a > > terminal. I would have preferred to mark warnings yellow, but that > > doesn't work well on black-on-white terminals. > > Hmm, I hate colors myself :-) > > So I would prefer if an option is added to explicitly enable colors. And the > check for stdout can be replaced by a check for this new option. > > Also, the same option and behavior should be added to cec-compliance as well. > > I propose the option -C, --show-colors. Just my two cents here: I guess most people love colors for warnings (I do), and this has becoming more popular on gcc - and it is already a default for dvb tools, with is part of v4l2-utils. So, IMHO, it would make more sense to have colors enabled by default, and provide, instead, either an option to disable or to have an env var that would control it. > > And don't forget to update the man pages for both utils! > > Regards, > > Hans > > > > > regards > > Philipp > > > > Philipp Zabel (2): > > v4l2-compliance: use warn() in warn_once() > > v4l2-compliance: add colors > > > > utils/v4l2-compliance/v4l2-compliance.cpp | 11 ++++++++--- > > utils/v4l2-compliance/v4l2-compliance.h | 22 +++++++++++++--------- > > 2 files changed, 21 insertions(+), 12 deletions(-) > > > Thanks, Mauro
On 4/8/19 12:28 PM, Mauro Carvalho Chehab wrote: > Em Mon, 8 Apr 2019 11:05:20 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> Hi Philipp, >> >> On 4/8/19 10:45 AM, Philipp Zabel wrote: >>> Hi, >>> >>> not sure if anybody finds this as useful as I do to spot compliance >>> failures and warnings in a sea of OKs more easily, but this patch adds >>> some color escape codes to the output of v4l2-compliance. It marks "OK" >>> green, "warn" bold, and "fail" / "FAIL" bright red if the output is a >>> terminal. I would have preferred to mark warnings yellow, but that >>> doesn't work well on black-on-white terminals. >> >> Hmm, I hate colors myself :-) >> >> So I would prefer if an option is added to explicitly enable colors. And the >> check for stdout can be replaced by a check for this new option. >> >> Also, the same option and behavior should be added to cec-compliance as well. >> >> I propose the option -C, --show-colors. > > Just my two cents here: I guess most people love colors for warnings > (I do), and this has becoming more popular on gcc - and it is already > a default for dvb tools, with is part of v4l2-utils. > > So, IMHO, it would make more sense to have colors enabled by default, > and provide, instead, either an option to disable or to have an env > var that would control it. If we do that then it needs to be the same for all utils. I could live with a env variable. I just tried to run dvb-fe-tool (no arguments), and I get a warning in a brown/orange color, but after that my cursor turns the same color. Does it properly go back to black? Regards, Hans > >> >> And don't forget to update the man pages for both utils! >> >> Regards, >> >> Hans >> >>> >>> regards >>> Philipp >>> >>> Philipp Zabel (2): >>> v4l2-compliance: use warn() in warn_once() >>> v4l2-compliance: add colors >>> >>> utils/v4l2-compliance/v4l2-compliance.cpp | 11 ++++++++--- >>> utils/v4l2-compliance/v4l2-compliance.h | 22 +++++++++++++--------- >>> 2 files changed, 21 insertions(+), 12 deletions(-) >>> >> > > > > Thanks, > Mauro >
On Mon, 2019-04-08 at 12:44 +0200, Hans Verkuil wrote (paraphrased): > On 4/8/19 12:28 PM, Mauro Carvalho Chehab wrote (paraphrased): [...] > > > [I hate colors] > > [I love colors] > I could live with [compromise] Sorry for opening that can of worms :-) How about doing something similar to 'ls': - an environment variable V4L_COLORS which, if not set, defaults to V4L_COLORS="ok=32:warn=1:fail=1;31" - an option --color=always|never|auto, which if not set, defaults to auto (check isatty()) - to disable colors by default, set V4L_COLORS to "ok=0:warn=0:fail=0" This would allow to pipe colors into non-terminal stdout like less -R if desired, or quickly disable colors with --color=never. The pattern could be extended to include dvbv5 log levels or fe tool quality levels. Or alternatively, V4L_COLOR=always|never|auto for easier configuration, but with a fixed color scheme? regards Philipp
On 4/8/19 4:41 PM, Philipp Zabel wrote: > On Mon, 2019-04-08 at 12:44 +0200, Hans Verkuil wrote (paraphrased): >> On 4/8/19 12:28 PM, Mauro Carvalho Chehab wrote (paraphrased): > [...] >>>> [I hate colors] >>> [I love colors] >> I could live with [compromise] > > Sorry for opening that can of worms :-) > > How about doing something similar to 'ls': > > - an environment variable V4L_COLORS which, if not set, defaults to > V4L_COLORS="ok=32:warn=1:fail=1;31" > - an option --color=always|never|auto, which if not set, defaults to > auto (check isatty()) > - to disable colors by default, set V4L_COLORS to "ok=0:warn=0:fail=0" > > This would allow to pipe colors into non-terminal stdout like less -R if > desired, or quickly disable colors with --color=never. > The pattern could be extended to include dvbv5 log levels or fe tool > quality levels. > > Or alternatively, V4L_COLOR=always|never|auto for easier configuration, > but with a fixed color scheme? I think that is easiest. As long as the color scheme works for both black-on-white and white-on-black it should be fine. Regards, Hans
Em Mon, 8 Apr 2019 12:44:18 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 4/8/19 12:28 PM, Mauro Carvalho Chehab wrote: > > Em Mon, 8 Apr 2019 11:05:20 +0200 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> Hi Philipp, > >> > >> On 4/8/19 10:45 AM, Philipp Zabel wrote: > >>> Hi, > >>> > >>> not sure if anybody finds this as useful as I do to spot compliance > >>> failures and warnings in a sea of OKs more easily, but this patch adds > >>> some color escape codes to the output of v4l2-compliance. It marks "OK" > >>> green, "warn" bold, and "fail" / "FAIL" bright red if the output is a > >>> terminal. I would have preferred to mark warnings yellow, but that > >>> doesn't work well on black-on-white terminals. > >> > >> Hmm, I hate colors myself :-) > >> > >> So I would prefer if an option is added to explicitly enable colors. And the > >> check for stdout can be replaced by a check for this new option. > >> > >> Also, the same option and behavior should be added to cec-compliance as well. > >> > >> I propose the option -C, --show-colors. > > > > Just my two cents here: I guess most people love colors for warnings > > (I do), and this has becoming more popular on gcc - and it is already > > a default for dvb tools, with is part of v4l2-utils. > > > > So, IMHO, it would make more sense to have colors enabled by default, > > and provide, instead, either an option to disable or to have an env > > var that would control it. > > If we do that then it needs to be the same for all utils. I could live > with a env variable. Fully agreed on that. We should handle it the same way on all apps. > I just tried to run dvb-fe-tool (no arguments), and I get a warning > in a brown/orange color, but after that my cursor turns the same color. > Does it properly go back to black? Hmm... good point. It should, but this is something that I usually don't really test here, as my prompt is colored: PS1='\[\033[01;32m\]\u@\h\[\033[01;34m\] \w \$\[\033[00m\] ' so if it doesn't reset the terminal, I wouldn't notice. Just did a quick test here. Colors are being reset with Mate terminal: <colored prompt> $ PS1="\w \$ " ~ $ dvb-fe-tool Device Montage Technology M88DS3103 (/dev/dvb/adapter0/frontend0) capabilities: CAN_2G_MODULATION CAN_FEC_1_2 CAN_FEC_2_3 CAN_FEC_3_4 CAN_FEC_4_5 CAN_FEC_5_6 CAN_FEC_6_7 CAN_FEC_7_8 CAN_FEC_8_9 CAN_FEC_AUTO CAN_INVERSION_AUTO CAN_QPSK CAN_RECOVER DVB API Version 5.11, Current v5 delivery system: DVBS Supported delivery systems: [DVBS] DVBS2 Frequency range for the current standard: From: 950 MHz To: 2.15 GHz Tolerance: 5.00 MHz Symbol rate ranges for the current standard: From: 1.00 MBauds To: 45.0 MBauds SEC: set voltage to OFF ERROR FE_SET_VOLTAGE: Operation not permitted # printed in RED ~ $ dvb-fe-tool -a 2 WARNING device dvb2.frontend0 not found # printed in YELLOW ~ $ With both the above cases, the prompt return to black and white. Perhaps the terminal you're using are not properly handling the color reset command. I saw in the past some broken terminal emulations where resetting the colors only work if it also prints something after the color reset command with a "\n" at the end. Thanks, Mauro
On 4/8/19 6:10 PM, Mauro Carvalho Chehab wrote: > Em Mon, 8 Apr 2019 12:44:18 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> On 4/8/19 12:28 PM, Mauro Carvalho Chehab wrote: >>> Em Mon, 8 Apr 2019 11:05:20 +0200 >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >>> >>>> Hi Philipp, >>>> >>>> On 4/8/19 10:45 AM, Philipp Zabel wrote: >>>>> Hi, >>>>> >>>>> not sure if anybody finds this as useful as I do to spot compliance >>>>> failures and warnings in a sea of OKs more easily, but this patch adds >>>>> some color escape codes to the output of v4l2-compliance. It marks "OK" >>>>> green, "warn" bold, and "fail" / "FAIL" bright red if the output is a >>>>> terminal. I would have preferred to mark warnings yellow, but that >>>>> doesn't work well on black-on-white terminals. >>>> >>>> Hmm, I hate colors myself :-) >>>> >>>> So I would prefer if an option is added to explicitly enable colors. And the >>>> check for stdout can be replaced by a check for this new option. >>>> >>>> Also, the same option and behavior should be added to cec-compliance as well. >>>> >>>> I propose the option -C, --show-colors. >>> >>> Just my two cents here: I guess most people love colors for warnings >>> (I do), and this has becoming more popular on gcc - and it is already >>> a default for dvb tools, with is part of v4l2-utils. >>> >>> So, IMHO, it would make more sense to have colors enabled by default, >>> and provide, instead, either an option to disable or to have an env >>> var that would control it. >> >> If we do that then it needs to be the same for all utils. I could live >> with a env variable. > > Fully agreed on that. We should handle it the same way on all apps. > >> I just tried to run dvb-fe-tool (no arguments), and I get a warning >> in a brown/orange color, but after that my cursor turns the same color. >> Does it properly go back to black? Ah, it appears to be a side-effect of not-quite-right PS1 prompt sequence that I had in my .bashrc. So dvb-fe-tool seems to do the right thing after all. Regards, Hans > > Hmm... good point. It should, but this is something that I usually don't > really test here, as my prompt is colored: > > PS1='\[\033[01;32m\]\u@\h\[\033[01;34m\] \w \$\[\033[00m\] ' > > so if it doesn't reset the terminal, I wouldn't notice. > > Just did a quick test here. Colors are being reset with Mate terminal: > > <colored prompt> $ PS1="\w \$ " > ~ $ dvb-fe-tool > Device Montage Technology M88DS3103 (/dev/dvb/adapter0/frontend0) capabilities: > CAN_2G_MODULATION > CAN_FEC_1_2 > CAN_FEC_2_3 > CAN_FEC_3_4 > CAN_FEC_4_5 > CAN_FEC_5_6 > CAN_FEC_6_7 > CAN_FEC_7_8 > CAN_FEC_8_9 > CAN_FEC_AUTO > CAN_INVERSION_AUTO > CAN_QPSK > CAN_RECOVER > DVB API Version 5.11, Current v5 delivery system: DVBS > Supported delivery systems: > [DVBS] > DVBS2 > Frequency range for the current standard: > From: 950 MHz > To: 2.15 GHz > Tolerance: 5.00 MHz > Symbol rate ranges for the current standard: > From: 1.00 MBauds > To: 45.0 MBauds > SEC: set voltage to OFF > ERROR FE_SET_VOLTAGE: Operation not permitted # printed in RED > > ~ $ dvb-fe-tool -a 2 > WARNING device dvb2.frontend0 not found # printed in YELLOW > ~ $ > > With both the above cases, the prompt return to black and white. > > Perhaps the terminal you're using are not properly handling the > color reset command. I saw in the past some broken terminal emulations > where resetting the colors only work if it also prints something after > the color reset command with a "\n" at the end. > > Thanks, > Mauro >
Em Mon, 8 Apr 2019 16:46:02 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 4/8/19 4:41 PM, Philipp Zabel wrote: > > On Mon, 2019-04-08 at 12:44 +0200, Hans Verkuil wrote (paraphrased): > >> On 4/8/19 12:28 PM, Mauro Carvalho Chehab wrote (paraphrased): > > [...] > >>>> [I hate colors] > >>> [I love colors] > >> I could live with [compromise] > > > > Sorry for opening that can of worms :-) > > > > How about doing something similar to 'ls': > > > > - an environment variable V4L_COLORS which, if not set, defaults to > > V4L_COLORS="ok=32:warn=1:fail=1;31" > > - an option --color=always|never|auto, which if not set, defaults to > > auto (check isatty()) > > - to disable colors by default, set V4L_COLORS to "ok=0:warn=0:fail=0" > > > > This would allow to pipe colors into non-terminal stdout like less -R if > > desired, or quickly disable colors with --color=never. > > The pattern could be extended to include dvbv5 log levels or fe tool > > quality levels. > > > > Or alternatively, V4L_COLOR=always|never|auto for easier configuration, > > but with a fixed color scheme? > > I think that is easiest. As long as the color scheme works for both black-on-white > and white-on-black it should be fine. Instead of V4L2_COLOR, perhaps MEDIA_APPS_COLLOR would be a better name, as the same env var could be used also for DVB tools. > > Regards, > > Hans Thanks, Mauro