[RFC,1/2] env: Drop error messages when loading environment

Message ID 20180717220912.11358-1-semen.protsenko@linaro.org
State New
Headers show
Series
  • [RFC,1/2] env: Drop error messages when loading environment
Related show

Commit Message

Sam Protsenko July 17, 2018, 10:09 p.m.
This is just a draft to discuss ideas related to "Make U-Boot log great
again" thread.

With this patch we will have something like this:

    Loading Environment from FAT... Failed (-5)
    Loading Environment from MMC... OK

instead of this:

    Loading Environment from FAT... MMC: no card present
    ** Bad device mmc 0 **
    Failed (-5)
    Loading Environment from MMC... OK

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 env/env.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Wolfgang Denk July 18, 2018, 6:19 a.m. | #1
Dear Sam,

In message <20180717220912.11358-1-semen.protsenko@linaro.org> you wrote:
> This is just a draft to discuss ideas related to "Make U-Boot log great
> again" thread.
> 
> With this patch we will have something like this:
> 
>     Loading Environment from FAT... Failed (-5)
>     Loading Environment from MMC... OK
> 
> instead of this:
> 
>     Loading Environment from FAT... MMC: no card present
>     ** Bad device mmc 0 **
>     Failed (-5)
>     Loading Environment from MMC... OK

Why do you think an error message "Failed (-5)" looks great?

From a user's point of view, the "MMC: no card present"
is _much_ better (but of course it could still be improved).

Printing cryptic error codes has never been a good idea and is
definitly not a "great" idea.

Best regards,

Wolfgang Denk
Sam Protsenko July 18, 2018, 12:50 p.m. | #2
On Wed, Jul 18, 2018 at 9:19 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Sam,
>
> In message <20180717220912.11358-1-semen.protsenko@linaro.org> you wrote:
>> This is just a draft to discuss ideas related to "Make U-Boot log great
>> again" thread.
>>
>> With this patch we will have something like this:
>>
>>     Loading Environment from FAT... Failed (-5)
>>     Loading Environment from MMC... OK
>>
>> instead of this:
>>
>>     Loading Environment from FAT... MMC: no card present
>>     ** Bad device mmc 0 **
>>     Failed (-5)
>>     Loading Environment from MMC... OK
>
> Why do you think an error message "Failed (-5)" looks great?
>

As I mentioned in commit message, this RFC series is merely for
discussing ideas. I don't think it's "great" (for the same reasons you
mentioned), but this is one of things we *could* do technically, to
make boot log straight. What I mean "technically" doable: for example
we would like to see log like this:

    Loading Environment from FAT... Failed (-5):
       -> MMC: no card present
       -> ** Bad device mmc 0 **
    Loading Environment from MMC... OK

But to do so, we would probably need to do one of these:
  1. Rework the code for all boot sources (like drivers/mmc/mmc.c), so
that that code doesn't print warnings to console, but instead filling
some error buffer, so we can print that buffer later from env.c. I'm
not sure it's a sane idea
  2. Issuing some escape codes for moving cursor up and down, to
modify already printed message, also has a lot of implications and I
don't thing it's sane as well...
  3. Messing with GD_FLG_RECORD and GD_FLG_SILENT also doesn't seem to
be right here, as much of platforms don't enable CONFIG_CONSOLE_RECORD
and CONFIG_SILENT_CONSOLE

So *technically*, we can only do what I did in two RFC patches I sent.
The only other way I see, is to make boot log look like this:

    --> Loading Environment from FAT... MMC: no card present
    ** Bad device mmc 0 **
    Failed (-5):
    --> Loading Environment from MMC... OK

Not sure if I like this way, but it's doable.

If you have any preferences about what I said, or if you have any
other ideas on how to approach this, please share. That's the whole
reason why I sent this RFC series :)

> From a user's point of view, the "MMC: no card present"
> is _much_ better (but of course it could still be improved).
>
> Printing cryptic error codes has never been a good idea and is
> definitly not a "great" idea.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Fools ignore complexity. Pragmatists suffer it. Some  can  avoid  it.
> Geniuses remove it.
>      - Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982
Tom Rini July 18, 2018, 12:51 p.m. | #3
On Wed, Jul 18, 2018 at 08:19:50AM +0200, Wolfgang Denk wrote:
> Dear Sam,

> 

> In message <20180717220912.11358-1-semen.protsenko@linaro.org> you wrote:

> > This is just a draft to discuss ideas related to "Make U-Boot log great

> > again" thread.

> > 

> > With this patch we will have something like this:

> > 

> >     Loading Environment from FAT... Failed (-5)

> >     Loading Environment from MMC... OK

> > 

> > instead of this:

> > 

> >     Loading Environment from FAT... MMC: no card present

> >     ** Bad device mmc 0 **

> >     Failed (-5)

> >     Loading Environment from MMC... OK

> 

> Why do you think an error message "Failed (-5)" looks great?

> 

> From a user's point of view, the "MMC: no card present"

> is _much_ better (but of course it could still be improved).

> 

> Printing cryptic error codes has never been a good idea and is

> definitly not a "great" idea.


Agreed, I don't think we want to go down the path of just suppressing
some of these error messages, that's not helpful.

-- 
Tom

Patch

diff --git a/env/env.c b/env/env.c
index 5c0842ac07..85598fa5d4 100644
--- a/env/env.c
+++ b/env/env.c
@@ -187,6 +187,7 @@  int env_load(void)
 
 	for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {
 		int ret;
+		unsigned long have_console = gd->have_console;
 
 		if (!drv->load)
 			continue;
@@ -195,7 +196,11 @@  int env_load(void)
 			continue;
 
 		printf("Loading Environment from %s... ", drv->name);
+
+		/* Suppress console output for drv->load() */
+		gd->have_console = 0;
 		ret = drv->load();
+		gd->have_console = have_console;
 		if (ret)
 			printf("Failed (%d)\n", ret);
 		else