Re: [PATCH v3 08/25] menu: Make use of CLI character processing

[Simon Glass <sjg@chromium.org>]

Hi,

On Sun, 23 Apr 2023 at 19:49, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Apr 18, 2023 at 07:49:34PM -0600, Simon Glass wrote:
> > Hi Daniel,
> >
> > On Tue, 11 Apr 2023 at 14:19, Daniel Golle <daniel@makrotopia.org> wrote:
> > >
> > > On Fri, Jan 06, 2023 at 08:52:26AM -0600, Simon Glass wrote:
> > > > Avoid duplicating some of the escape-sequence processing here and use the
> > > > CLI function instead.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > [...]
> > > > diff --git a/common/menu.c b/common/menu.c
> > > > index 7db98942a61..45f36ae3ede 100644
> > > > --- a/common/menu.c
> > > > +++ b/common/menu.c
> > > > @@ -15,6 +15,8 @@
> > > >
> > > >  #include "menu.h"
> > > >
> > > > +#define ansi 0
> > >
> > > Now that I'm using U-Boot 2023.04 I noticed that this introduces a (cosmetic)
> > > regression: The autoboot countdown in bootmenu is now longer re-written, but
> > > instead the last menu item line is continously appended like:
> > >
> > > U-Boot consoleHit any key to stop autoboot: 3 Hit any key to stop autoboot: 2
> > >
> > > To regain the previous behavior one should change the line to
> > > #define ansi 1
> >
> > Yes, perhaps you could send a patch?
>
> As a new CONFIG symbol, default y for the old behavior please.
>
> > > To me this looks a bit unfinished, as if there was some sort of
> > > probing of the terminal capabilities supposed to happen but then
> > > this has never been implemeneted...
> >
> > I have expressed the review that ANSI should be something we can
> > enable or disable, rather than always having it enabled. For tests in
> > particular, ANSI is a pain since it messes up the logs with escape
> > sequences and it is hard to see what is happening.
> >
> > IMO we should have an ANSI library which you call to emit sequences.
> > Or perhaps we could try to be clever and build them into printf() as
> > special strings, so long as we can avoid compiler warnings.
>
> Well, I think for starting we can just make the behavior configurable at
> build time. I'm not sure we need to do run-time (not clear if you're
> suggesting that or not), and I don't know if code changes to deal with
> ANSI-or-not would result in some unreasonable code growth here.

My main goal is to avoid this stuff in test logs, so mostly sandbox,
and having it configurable at runtime (for sandbox) would be ideal.

Making it build-time configuration is fine as a starting point.

Regards,
Simon