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

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

[Philipp Zabel]

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

-- 
2.20.1

[PATCH v4l-utils 1/2] v4l2-compliance: use warn() in warn_once()

[Philipp Zabel]

Use the warn() macro in warn_once() instead of duplicating its contents.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 utils/v4l2-compliance/v4l2-compliance.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h
index ac709e94a537..f7e1ea5d7124 100644
--- a/utils/v4l2-compliance/v4l2-compliance.h
+++ b/utils/v4l2-compliance/v4l2-compliance.h
@@ -206,12 +206,7 @@ private:
 									\
 		if (!show) {						\
 			show = true;					\
-			warnings++;					\
-			if (show_warnings)				\
-				printf("\t\twarn: %s(%d): " fmt,	\
-					__FILE__, __LINE__, ##args); 	\
-			if (exit_on_warn)				\
-				exit(1);				\
+			warn(fmt, ##args);				\
 		}							\
 	} while (0)
 
-- 
2.20.1

[PATCH v4l-utils 2/2] v4l2-compliance: add colors

[Philipp Zabel]

If the output is a terminal, use color codes to mark OK, warn, and
FAIL messages with green, bold, and bright red accents, respectively.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 utils/v4l2-compliance/v4l2-compliance.cpp | 11 ++++++++---
 utils/v4l2-compliance/v4l2-compliance.h   | 15 ++++++++++++---
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
index f7611d75cb1b..488e746d055d 100644
--- a/utils/v4l2-compliance/v4l2-compliance.cpp
+++ b/utils/v4l2-compliance/v4l2-compliance.cpp
@@ -89,6 +89,7 @@ static int grand_total, grand_ok, grand_warnings;
 bool show_info;
 bool no_progress;
 bool show_warnings = true;
+bool show_colors;
 bool exit_on_fail;
 bool exit_on_warn;
 bool is_vivid;
@@ -250,15 +251,17 @@ const char *ok(int res)
 	static char buf[100];
 
 	if (res == ENOTTY) {
-		strcpy(buf, "OK (Not Supported)");
+		strcpy(buf, show_colors ?
+		       COLOR_GREEN("OK") " (Not Supported)" :
+		       "OK (Not Supported)");
 		res = 0;
 	} else {
-		strcpy(buf, "OK");
+		strcpy(buf, show_colors ? COLOR_GREEN("OK") : "OK");
 	}
 	tests_total++;
 	if (res) {
 		app_result = res;
-		sprintf(buf, "FAIL");
+		sprintf(buf, show_colors ? COLOR_RED("FAIL") : "FAIL");
 	} else {
 		tests_ok++;
 	}
@@ -1630,6 +1633,8 @@ int main(int argc, char **argv)
 	bool direct = !options[OptUseWrapper];
 	int fd;
 
+	show_colors = isatty(STDOUT_FILENO);
+
 	if (type == MEDIA_TYPE_UNKNOWN)
 		type = mi_media_detect_type(device.c_str());
 	if (type == MEDIA_TYPE_CANT_STAT) {
diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h
index f7e1ea5d7124..7909c3822cd0 100644
--- a/utils/v4l2-compliance/v4l2-compliance.h
+++ b/utils/v4l2-compliance/v4l2-compliance.h
@@ -52,6 +52,7 @@
 extern bool show_info;
 extern bool show_warnings;
 extern bool no_progress;
+extern bool show_colors;
 extern bool exit_on_fail;
 extern bool exit_on_warn;
 extern bool is_vivid; // We're testing the vivid driver
@@ -185,17 +186,24 @@ private:
 	std::set<int> fhs;
 };
 
+#define COLOR_GREEN(s) "\033[32m" s "\033[0m"
+#define COLOR_RED(s) "\033[1;31m" s "\033[0m"
+#define COLOR_BOLD(s) "\033[1m" s "\033[0m"
+
 #define info(fmt, args...) 					\
 	do {							\
 		if (show_info)					\
- 			printf("\t\tinfo: " fmt, ##args);	\
+			printf("\t\tinfo: " fmt, ##args);	\
 	} while (0)
 
 #define warn(fmt, args...) 					\
 	do {							\
 		warnings++;					\
 		if (show_warnings)				\
- 			printf("\t\twarn: %s(%d): " fmt, __FILE__, __LINE__, ##args);	\
+			printf("\t\t%s: %s(%d): " fmt,		\
+			       show_colors ?			\
+			       COLOR_BOLD("warn") : "warn",	\
+			       __FILE__, __LINE__, ##args);	\
 		if (exit_on_warn)				\
 			exit(1);				\
 	} while (0)
@@ -218,7 +226,8 @@ private:
 
 #define fail(fmt, args...) 						\
 ({ 									\
- 	printf("\t\tfail: %s(%d): " fmt, __FILE__, __LINE__, ##args);	\
+	printf("\t\t%s: %s(%d): " fmt, show_colors ?			\
+	       COLOR_RED("fail") : "fail", __FILE__, __LINE__, ##args);	\
 	if (exit_on_fail)						\
 		exit(1);						\
 	1;								\
-- 
2.20.1

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Hans Verkuil]

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

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Mauro Carvalho Chehab]

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

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Hans Verkuil]

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
> 

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Philipp Zabel]

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

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Hans Verkuil]

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

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Mauro Carvalho Chehab]

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

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Mauro Carvalho Chehab]

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

Re: [PATCH v4l-utils 0/2] v4l2-compliance colors

[Hans Verkuil]

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
>