mbox series

[v4l-utils,0/2] v4l2-compliance colors

Message ID 20190408084558.2979-1-p.zabel@pengutronix.de (mailing list archive)
Headers show
Series v4l2-compliance colors | expand

Message

Philipp Zabel April 8, 2019, 8:45 a.m. UTC
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.

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(-)

Comments

Hans Verkuil April 8, 2019, 9:05 a.m. UTC | #1
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(-)
>
Mauro Carvalho Chehab April 8, 2019, 10:28 a.m. UTC | #2
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
Hans Verkuil April 8, 2019, 10:44 a.m. UTC | #3
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
>
Philipp Zabel April 8, 2019, 2:41 p.m. UTC | #4
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
Hans Verkuil April 8, 2019, 2:46 p.m. UTC | #5
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
Mauro Carvalho Chehab April 8, 2019, 4:10 p.m. UTC | #6
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
Hans Verkuil April 8, 2019, 4:21 p.m. UTC | #7
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
>
Mauro Carvalho Chehab April 8, 2019, 4:50 p.m. UTC | #8
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