[PATCH] menu: Re-enable the ANSI codes

Sat Jun 17 12:48:51 CEST 2023

Hi Pali,

On Thu, 15 Jun 2023 at 17:56, Pali Rohár <pali at kernel.org> wrote:
>
> On Thursday 15 June 2023 13:34:09 Simon Glass wrote:
> > Hi Pali,
> >
> > On Mon, 12 Jun 2023 at 22:33, Pali Rohár <pali at kernel.org> wrote:
> > >
> > > On Monday 12 June 2023 22:17:48 Simon Glass wrote:
> > > > Hi Pali,
> > > >
> > > > On Mon, 12 Jun 2023 at 21:22, Pali Rohár <pali at kernel.org> wrote:
> > > > >
> > > > > On Monday 12 June 2023 21:14:32 Simon Glass wrote:
> > > > > > The intent here was to allow ANSI codes to be disabled, since it was
> > > > > > proving impoosible to test operation of the menu code when it kept moving
> > > > > > the cursor. Unfortunately this ended up in the patch.
> > > > > >
> > > > > > Correct this by enabling ANSI again.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > Reported-by: Pali Rohár <pali at kernel.org>
> > > > > > Reported-by: Mark Kettenis <mark.kettenis at xs4all.nl>
> > > > > > Reported-by: Frank Wunderlich <frank-w at public-files.de>
> > > > > > Fixes: 32bab0eae51b ("menu: Make use of CLI character processing")
> > > > > > ---
> > > > > >
> > > > > >  common/menu.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/common/menu.c b/common/menu.c
> > > > > > index 94514177e4e9..b55cf7b99967 100644
> > > > > > --- a/common/menu.c
> > > > > > +++ b/common/menu.c
> > > > > > @@ -15,7 +15,7 @@
> > > > > >
> > > > > >  #include "menu.h"
> > > > > >
> > > > > > -#define ansi 0
> > > > > > +#define ansi 1
> > > > > >
> > > > > >  /*
> > > > > >   * Internally, each item in a menu is represented by a struct menu_item.
> > > > > > --
> > > > > > 2.41.0.162.gfafddb0af9-goog
> > > > > >
> > > > >
> > > > > Hello, I have tested this change but bootmenu still does not work. There
> > > > > is still same issue which I reported month ago. When I press DOWN key
> > > > > then bootmenu immediately quit instead of moving into the next entry.
> > > >
> > > > Thanks for testing this.
> > > >
> > > > Is there a way for me to test this with sandbox? Or does your Nokia
> > > > test check it?
> > >
> > > I guess that bootmenu command could work in sandbox. But I have not
> > > tried it.
> > >
> > > Nokia CI test does not try any terminal, keyboard or VGA interaction, so
> > > broken rendering or broken keyboard input is not caught by CI.
> > >
> > > But it is possible to test it manually. See U-Boot documentation how to
> > > run Nokia u-boot image in qemu. Bootmenu is automatically started.
> > > https://u-boot.readthedocs.io/en/latest/board/nokia/rx51.html#run-in-qemu
> >
> > I tried to follow this but got stuck here:
> >
> > ./configure --enable-system --target-list=arm-softmmu --disable-werror
> >
> > ERROR: Cannot use 'python', Python 2.4 or later is required.
> >        Note that Python 3 or later is not yet supported.
> >        Use --python=/path/to/python to specify a supported Python.
> >
> > Python 2 has been deprecated for years and I think it was removed recently.
>
> Ach :-( Is not there some configure option to disable python?

I don't know, but the qemu you are using seems to be 10 years old?

>
> In U-Boot CI is qemu compiling without issues (but variant without GUI).

You mean that it still uses python 2? But for how much longer?

I don't think it is reasonable to base a test on an old version of qemu.

>
> > >
> > > Bootmenu is available on both VGA display and terminal output. Problem
> > > with DOWN key is on the terminal.
> > >
> > > >
> > > > I'm sure you have this commit:?
> > > >
> > > > 17b45e684af9 ("cli: Correct several bugs in cli_getch()")
> > >
> > > Yes, as it is in the master branch already.
> >
> > I tried the following with sandbox:
> >
> > setenv bootmenu_0 "my item=echo item 0"
> > setenv bootmenu_1 "second item=echo item 1"
> > setenv bootmenu_delay 10
> > bootmenu
> >
> > I see that with the ANSI patch the 'press key' prompt now appears in
> > the right place.
> >
> > When I press down key repeatedly, it goes down until the last item and
> > then refuses to go any further.
> >
> > So I can't repeat this problem.
>
> Hm... so it looks like some ARM or UART specific thing... or something
> which is not triggered by sandbox.
>
> Just some suggestions, could you try to add some more entries? Or trying
> to wait e.g. 30s before pressing another key (qemu is slow so there can
> be some delay issue?)? Or could you try to run bootmenu on some real
> hardware via UART?

I did get it going by hacking my machine.

I'll send a series with some debugging, etc. I am not quite sure what
the problem is, but it seems to be timing-related, or perhaps a bug in
the keyboard code (I found one bug already).

>
> > I tried looking at the Nokia code. The keyboard driver should be in
> > drivers/input, but I eventually found it in the board directory.
> >
> > Are you sure that rx51_kp_tstc() behaves correctly? Can you add
> > debugging to see what codes it is emitting?
> >
> > Also you don't need to generate escape sequences in rx51_kp_fill().
> > You can just use Ctrl-N, Ctrl-P, Ctrl-B and Ctrl-F (see cli_ch_esc()).
> > That should simplify things a bit.
> >
> > If you can find a way to repeat this problem and have verified that
> > the Nokia code is correct, I can take another look.
>
> Problem with DOWN key is on the terminal, not on the screen or keyboard.
> So all those rx51_kp_tstc() and rx51_kp_fill() should not be the cause
> (unless some input/output multiplexing in u-boot takes a role).
>
> Terminal to qemu is connected via qemu's UART emulation, so omap2 uart
> driver in this case is the one which connects u-boot to the qemu's UART
> emulation.
>
> > Regards,
> > Simon
> >
> > [1] https://patchwork.ozlabs.org/project/uboot/patch/20230612201434.861700-1-sjg@chromium.org/

Regards,
SImon