[PATCH] menu: Re-enable the ANSI codes

Sat Jun 24 11:01:13 CEST 2023

On Saturday 17 June 2023 22:28:24 Heinrich Schuchardt wrote:
> On 6/17/23 12:48, Simon Glass wrote:
> > 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.

So install all required dependencies? It is really stupid argument to
say that _I have removed X from my PC_ and then _I cannot compile Y
because it depends on X_.

> 
> Our Docker image uses Ubuntu 22.04 (Jammy). Ubuntu 22.10 (Kinetic) was
> the last Ubuntu release providing Python 2.7. So when upgrading our
> Docker image next year we will have to quit emulating that 14 year old
> device.

Why to quit? Why you cannot compile and install required dependency?

Also was not the whole purpose of using containers to make easier of
using different languages in different versions and also different
software?

Also what is the problem with 14 year old device and software? It is
_working_. Or are you saying that you do not want to support _working_
device just because it is _old_? What is the logic here?

> > > 
> > > 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?
> 
> The Nokia N900 support never existed in upstream QEMU. Is is only in a
> Linaro repository that has not been update since 2015. (No clue why our
> CI uses the 2013 version and not the 2015 one.)

We are using the latest version which contains working n900 emulator. It
is written also in the u-boot documentation.

> I anybody cares about N900 being used in U-Boot's CI he should rework
> the N900 patch from the Linaro QEMU archive and get it integrated into
> upstream QEMU.

Why? Emulator was already written and it is _working_. Why to invest new
time for writing a new emulator?

I think you completely missed the point here. U-Boot is being test in
this emulator to ensure that changes in u-boot does not break something.
In the past it already caught issues which were not uncovered by any
people review and neither by any other CI tests. Also for things which
are not n900-related.

> Best regards
> 
> Heinrich
> 
> 
> > 
> > > 
> > > 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.

Why not? U-Boot should work on any compatible hardware, so also on older
qemu version.

Released hardware is _not_ being changed, so any software testing should
use also older emulators, which are bound to the release date of
hardware to better match hardware testing.

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