| Message ID | 20221022092058.106052-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] cli: always show cursor | expand |
Hi Heinrich, On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > We may enter the command line interface in a state where on the remote > console the cursor is not shown. Send an escape sequence to enable it. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > common/main.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/common/main.c b/common/main.c > index 682f3359ea..4e7e7b17d6 100644 > --- a/common/main.c > +++ b/common/main.c > @@ -7,6 +7,7 @@ > /* #define DEBUG */ > > #include <common.h> > +#include <ansi.h> > #include <autoboot.h> > #include <bootstage.h> > #include <cli.h> > @@ -66,6 +67,9 @@ void main_loop(void) > > autoboot_command(s); > > + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) > + printf(ANSI_CURSOR_SHOW "\n"); Could we create a library which emits these codes? Like ansi_cursor_show() ? > + > cli_loop(); > panic("No CLI available"); > } > -- > 2.37.2 > Regards, Simon
On 10/26/22 01:35, Simon Glass wrote: > Hi Heinrich, > > On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> We may enter the command line interface in a state where on the remote >> console the cursor is not shown. Send an escape sequence to enable it. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> common/main.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/common/main.c b/common/main.c >> index 682f3359ea..4e7e7b17d6 100644 >> --- a/common/main.c >> +++ b/common/main.c >> @@ -7,6 +7,7 @@ >> /* #define DEBUG */ >> >> #include <common.h> >> +#include <ansi.h> >> #include <autoboot.h> >> #include <bootstage.h> >> #include <cli.h> >> @@ -66,6 +67,9 @@ void main_loop(void) >> >> autoboot_command(s); >> >> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) >> + printf(ANSI_CURSOR_SHOW "\n"); > > Could we create a library which emits these codes? Like ansi_cursor_show() ? The question is not if we could but if we should. Up to now we tried to call printf() only once where ANSI_* is only one part of the output string. E.g. cmd/eficonfig.c:415: printf(ANSI_CLEAR_CONSOLE ANSI_CURSOR_POSITION ANSI_CURSOR_SHOW, 1, 1); This minimizes the number of call library calls but may not be very elegant. Creating a library function for ANSI_CURSOR_SHOW alone does not make much sense to me. Should you want to convert our code base from using ANSI_* to library functions this should cover the whole code base. So I think we should let this patch pass as it solves a current problem and leave creating a ANSI_* function library to a separate exercise. Best regards Heinrich > >> + >> cli_loop(); >> panic("No CLI available"); >> } >> -- >> 2.37.2 >> > > Regards, > Simon
Hi Heinrich, On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 10/26/22 01:35, Simon Glass wrote: > > Hi Heinrich, > > > > On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> We may enter the command line interface in a state where on the remote > >> console the cursor is not shown. Send an escape sequence to enable it. > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> common/main.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/common/main.c b/common/main.c > >> index 682f3359ea..4e7e7b17d6 100644 > >> --- a/common/main.c > >> +++ b/common/main.c > >> @@ -7,6 +7,7 @@ > >> /* #define DEBUG */ > >> > >> #include <common.h> > >> +#include <ansi.h> > >> #include <autoboot.h> > >> #include <bootstage.h> > >> #include <cli.h> > >> @@ -66,6 +67,9 @@ void main_loop(void) > >> > >> autoboot_command(s); > >> > >> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) > >> + printf(ANSI_CURSOR_SHOW "\n"); > > > > Could we create a library which emits these codes? Like ansi_cursor_show() ? > > The question is not if we could but if we should. > > Up to now we tried to call printf() only once where ANSI_* is only one > part of the output string. > > E.g. cmd/eficonfig.c:415: > printf(ANSI_CLEAR_CONSOLE > ANSI_CURSOR_POSITION > ANSI_CURSOR_SHOW, 1, 1); > > This minimizes the number of call library calls but may not be very elegant. > > Creating a library function for ANSI_CURSOR_SHOW alone does not make > much sense to me. Should you want to convert our code base from using > ANSI_* to library functions this should cover the whole code base. > > So I think we should let this patch pass as it solves a current problem > and leave creating a ANSI_* function library to a separate exercise. Then we should add this to the serial API...what we have here is quite bizarre in a way, since it outputs escape characters as the default for U-Boot. Mostly we don't need it. So add a set_cursor_visible() method to the serial API and allow it to be controlled via platform data in the serial driver. Regards, SImon
On 10/27/22 17:22, Simon Glass wrote: > Hi Heinrich, > > On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> >> >> On 10/26/22 01:35, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt >>> <heinrich.schuchardt@canonical.com> wrote: >>>> >>>> We may enter the command line interface in a state where on the remote >>>> console the cursor is not shown. Send an escape sequence to enable it. >>>> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>> --- >>>> common/main.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/common/main.c b/common/main.c >>>> index 682f3359ea..4e7e7b17d6 100644 >>>> --- a/common/main.c >>>> +++ b/common/main.c >>>> @@ -7,6 +7,7 @@ >>>> /* #define DEBUG */ >>>> >>>> #include <common.h> >>>> +#include <ansi.h> >>>> #include <autoboot.h> >>>> #include <bootstage.h> >>>> #include <cli.h> >>>> @@ -66,6 +67,9 @@ void main_loop(void) >>>> >>>> autoboot_command(s); >>>> >>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) >>>> + printf(ANSI_CURSOR_SHOW "\n"); >>> >>> Could we create a library which emits these codes? Like ansi_cursor_show() ? >> >> The question is not if we could but if we should. >> >> Up to now we tried to call printf() only once where ANSI_* is only one >> part of the output string. >> >> E.g. cmd/eficonfig.c:415: >> printf(ANSI_CLEAR_CONSOLE >> ANSI_CURSOR_POSITION >> ANSI_CURSOR_SHOW, 1, 1); >> >> This minimizes the number of call library calls but may not be very elegant. >> >> Creating a library function for ANSI_CURSOR_SHOW alone does not make >> much sense to me. Should you want to convert our code base from using >> ANSI_* to library functions this should cover the whole code base. >> >> So I think we should let this patch pass as it solves a current problem >> and leave creating a ANSI_* function library to a separate exercise. > > Then we should add this to the serial API...what we have here is quite > bizarre in a way, since it outputs escape characters as the default > for U-Boot. Mostly we don't need it. > > So add a set_cursor_visible() method to the serial API and allow it to > be controlled via platform data in the serial driver. I have no clue which platform data you might be thinking of. It is also not my intention to eject the escape sequence whenever a serial console is probed. Using serial_puts() in this patch instead of printf() would help to avoid the CONFIG dependency. Best regards Heinrich > > Regards, > SImon
Hi Heinrich, On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 10/27/22 17:22, Simon Glass wrote: > > Hi Heinrich, > > > > On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> > >> > >> On 10/26/22 01:35, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt > >>> <heinrich.schuchardt@canonical.com> wrote: > >>>> > >>>> We may enter the command line interface in a state where on the remote > >>>> console the cursor is not shown. Send an escape sequence to enable it. > >>>> > >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>>> --- > >>>> common/main.c | 4 ++++ > >>>> 1 file changed, 4 insertions(+) > >>>> > >>>> diff --git a/common/main.c b/common/main.c > >>>> index 682f3359ea..4e7e7b17d6 100644 > >>>> --- a/common/main.c > >>>> +++ b/common/main.c > >>>> @@ -7,6 +7,7 @@ > >>>> /* #define DEBUG */ > >>>> > >>>> #include <common.h> > >>>> +#include <ansi.h> > >>>> #include <autoboot.h> > >>>> #include <bootstage.h> > >>>> #include <cli.h> > >>>> @@ -66,6 +67,9 @@ void main_loop(void) > >>>> > >>>> autoboot_command(s); > >>>> > >>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) > >>>> + printf(ANSI_CURSOR_SHOW "\n"); > >>> > >>> Could we create a library which emits these codes? Like ansi_cursor_show() ? > >> > >> The question is not if we could but if we should. > >> > >> Up to now we tried to call printf() only once where ANSI_* is only one > >> part of the output string. > >> > >> E.g. cmd/eficonfig.c:415: > >> printf(ANSI_CLEAR_CONSOLE > >> ANSI_CURSOR_POSITION > >> ANSI_CURSOR_SHOW, 1, 1); > >> > >> This minimizes the number of call library calls but may not be very elegant. > >> > >> Creating a library function for ANSI_CURSOR_SHOW alone does not make > >> much sense to me. Should you want to convert our code base from using > >> ANSI_* to library functions this should cover the whole code base. > >> > >> So I think we should let this patch pass as it solves a current problem > >> and leave creating a ANSI_* function library to a separate exercise. > > > > Then we should add this to the serial API...what we have here is quite > > bizarre in a way, since it outputs escape characters as the default > > for U-Boot. Mostly we don't need it. > > > > So add a set_cursor_visible() method to the serial API and allow it to > > be controlled via platform data in the serial driver. > > I have no clue which platform data you might be thinking of. > > It is also not my intention to eject the escape sequence whenever a > serial console is probed. > > Using serial_puts() in this patch instead of printf() would help to > avoid the CONFIG dependency. Here's what should happen: 1. ANSI codes should not appear in logs or on CI (this is currently a problem with EFI tests) 2. We cannot show an ANSI code on boot by default, since that puts ctrl characters into every start-up of U-Boot So I think my suggestion makes sense. See drivers/serial/sandbox.c and have a way of calling isatty() to check the terminal type (with a cmdline flag to override it, see -t for example). Then we can get the correct behaviour. Regards, Simon
On 10/30/22 02:43, Simon Glass wrote: > Hi Heinrich, > > On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> >> >> On 10/27/22 17:22, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt >>> <heinrich.schuchardt@canonical.com> wrote: >>>> >>>> >>>> >>>> On 10/26/22 01:35, Simon Glass wrote: >>>>> Hi Heinrich, >>>>> >>>>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt >>>>> <heinrich.schuchardt@canonical.com> wrote: >>>>>> >>>>>> We may enter the command line interface in a state where on the remote >>>>>> console the cursor is not shown. Send an escape sequence to enable it. >>>>>> >>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>>>> --- >>>>>> common/main.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/common/main.c b/common/main.c >>>>>> index 682f3359ea..4e7e7b17d6 100644 >>>>>> --- a/common/main.c >>>>>> +++ b/common/main.c >>>>>> @@ -7,6 +7,7 @@ >>>>>> /* #define DEBUG */ >>>>>> >>>>>> #include <common.h> >>>>>> +#include <ansi.h> >>>>>> #include <autoboot.h> >>>>>> #include <bootstage.h> >>>>>> #include <cli.h> >>>>>> @@ -66,6 +67,9 @@ void main_loop(void) >>>>>> >>>>>> autoboot_command(s); >>>>>> >>>>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) >>>>>> + printf(ANSI_CURSOR_SHOW "\n"); >>>>> >>>>> Could we create a library which emits these codes? Like ansi_cursor_show() ? >>>> >>>> The question is not if we could but if we should. >>>> >>>> Up to now we tried to call printf() only once where ANSI_* is only one >>>> part of the output string. >>>> >>>> E.g. cmd/eficonfig.c:415: >>>> printf(ANSI_CLEAR_CONSOLE >>>> ANSI_CURSOR_POSITION >>>> ANSI_CURSOR_SHOW, 1, 1); >>>> >>>> This minimizes the number of call library calls but may not be very elegant. >>>> >>>> Creating a library function for ANSI_CURSOR_SHOW alone does not make >>>> much sense to me. Should you want to convert our code base from using >>>> ANSI_* to library functions this should cover the whole code base. >>>> >>>> So I think we should let this patch pass as it solves a current problem >>>> and leave creating a ANSI_* function library to a separate exercise. >>> >>> Then we should add this to the serial API...what we have here is quite >>> bizarre in a way, since it outputs escape characters as the default >>> for U-Boot. Mostly we don't need it. >>> >>> So add a set_cursor_visible() method to the serial API and allow it to >>> be controlled via platform data in the serial driver. >> >> I have no clue which platform data you might be thinking of. >> >> It is also not my intention to eject the escape sequence whenever a >> serial console is probed. >> >> Using serial_puts() in this patch instead of printf() would help to >> avoid the CONFIG dependency. > > Here's what should happen: > > 1. ANSI codes should not appear in logs or on CI (this is currently a > problem with EFI tests) A program outputting different things in continuous integration then in real live does not provide realistic testing. It is the CI that has to adapt not the program under test. Which logs are you talking about? > 2. We cannot show an ANSI code on boot by default, since that puts > ctrl characters into every start-up of U-Boot Why would that be a problem? > > So I think my suggestion makes sense. See drivers/serial/sandbox.c and > have a way of calling isatty() to check the terminal type (with a > cmdline flag to override it, see -t for example). Then we can get the > correct behaviour. isatty() is not a U-Boot function. So it is irrelevant in this context. You cannot detect if a serial console understands ANSI sequences without sending any. Best regards Heinrich
Hi Heinrich, On Sat, 29 Oct 2022 at 20:56, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 10/30/22 02:43, Simon Glass wrote: > > Hi Heinrich, > > > > On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> > >> > >> On 10/27/22 17:22, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt > >>> <heinrich.schuchardt@canonical.com> wrote: > >>>> > >>>> > >>>> > >>>> On 10/26/22 01:35, Simon Glass wrote: > >>>>> Hi Heinrich, > >>>>> > >>>>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt > >>>>> <heinrich.schuchardt@canonical.com> wrote: > >>>>>> > >>>>>> We may enter the command line interface in a state where on the remote > >>>>>> console the cursor is not shown. Send an escape sequence to enable it. > >>>>>> > >>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>>>>> --- > >>>>>> common/main.c | 4 ++++ > >>>>>> 1 file changed, 4 insertions(+) > >>>>>> > >>>>>> diff --git a/common/main.c b/common/main.c > >>>>>> index 682f3359ea..4e7e7b17d6 100644 > >>>>>> --- a/common/main.c > >>>>>> +++ b/common/main.c > >>>>>> @@ -7,6 +7,7 @@ > >>>>>> /* #define DEBUG */ > >>>>>> > >>>>>> #include <common.h> > >>>>>> +#include <ansi.h> > >>>>>> #include <autoboot.h> > >>>>>> #include <bootstage.h> > >>>>>> #include <cli.h> > >>>>>> @@ -66,6 +67,9 @@ void main_loop(void) > >>>>>> > >>>>>> autoboot_command(s); > >>>>>> > >>>>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) > >>>>>> + printf(ANSI_CURSOR_SHOW "\n"); > >>>>> > >>>>> Could we create a library which emits these codes? Like ansi_cursor_show() ? > >>>> > >>>> The question is not if we could but if we should. > >>>> > >>>> Up to now we tried to call printf() only once where ANSI_* is only one > >>>> part of the output string. > >>>> > >>>> E.g. cmd/eficonfig.c:415: > >>>> printf(ANSI_CLEAR_CONSOLE > >>>> ANSI_CURSOR_POSITION > >>>> ANSI_CURSOR_SHOW, 1, 1); > >>>> > >>>> This minimizes the number of call library calls but may not be very elegant. > >>>> > >>>> Creating a library function for ANSI_CURSOR_SHOW alone does not make > >>>> much sense to me. Should you want to convert our code base from using > >>>> ANSI_* to library functions this should cover the whole code base. > >>>> > >>>> So I think we should let this patch pass as it solves a current problem > >>>> and leave creating a ANSI_* function library to a separate exercise. > >>> > >>> Then we should add this to the serial API...what we have here is quite > >>> bizarre in a way, since it outputs escape characters as the default > >>> for U-Boot. Mostly we don't need it. > >>> > >>> So add a set_cursor_visible() method to the serial API and allow it to > >>> be controlled via platform data in the serial driver. > >> > >> I have no clue which platform data you might be thinking of. > >> > >> It is also not my intention to eject the escape sequence whenever a > >> serial console is probed. > >> > >> Using serial_puts() in this patch instead of printf() would help to > >> avoid the CONFIG dependency. > > > > Here's what should happen: > > > > 1. ANSI codes should not appear in logs or on CI (this is currently a > > problem with EFI tests) > > A program outputting different things in continuous integration then in > real live does not provide realistic testing. > > It is the CI that has to adapt not the program under test. > > Which logs are you talking about? > > > 2. We cannot show an ANSI code on boot by default, since that puts > > ctrl characters into every start-up of U-Boot > > Why would that be a problem? > > > > > So I think my suggestion makes sense. See drivers/serial/sandbox.c and > > have a way of calling isatty() to check the terminal type (with a > > cmdline flag to override it, see -t for example). Then we can get the > > correct behaviour. > > isatty() is not a U-Boot function. So it is irrelevant in this context. > > You cannot detect if a serial console understands ANSI sequences without > sending any. NAK Please try a little harder to understand my POV above. Regards, Simon
On 10/31/22 20:27, Simon Glass wrote: > Hi Heinrich, > > On Sat, 29 Oct 2022 at 20:56, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> >> >> On 10/30/22 02:43, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt >>> <heinrich.schuchardt@canonical.com> wrote: >>>> >>>> >>>> >>>> On 10/27/22 17:22, Simon Glass wrote: >>>>> Hi Heinrich, >>>>> >>>>> On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt >>>>> <heinrich.schuchardt@canonical.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 10/26/22 01:35, Simon Glass wrote: >>>>>>> Hi Heinrich, >>>>>>> >>>>>>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt >>>>>>> <heinrich.schuchardt@canonical.com> wrote: >>>>>>>> >>>>>>>> We may enter the command line interface in a state where on the remote >>>>>>>> console the cursor is not shown. Send an escape sequence to enable it. >>>>>>>> >>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>>>>>> --- >>>>>>>> common/main.c | 4 ++++ >>>>>>>> 1 file changed, 4 insertions(+) >>>>>>>> >>>>>>>> diff --git a/common/main.c b/common/main.c >>>>>>>> index 682f3359ea..4e7e7b17d6 100644 >>>>>>>> --- a/common/main.c >>>>>>>> +++ b/common/main.c >>>>>>>> @@ -7,6 +7,7 @@ >>>>>>>> /* #define DEBUG */ >>>>>>>> >>>>>>>> #include <common.h> >>>>>>>> +#include <ansi.h> >>>>>>>> #include <autoboot.h> >>>>>>>> #include <bootstage.h> >>>>>>>> #include <cli.h> >>>>>>>> @@ -66,6 +67,9 @@ void main_loop(void) >>>>>>>> >>>>>>>> autoboot_command(s); >>>>>>>> >>>>>>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) >>>>>>>> + printf(ANSI_CURSOR_SHOW "\n"); >>>>>>> >>>>>>> Could we create a library which emits these codes? Like ansi_cursor_show() ? >>>>>> >>>>>> The question is not if we could but if we should. >>>>>> >>>>>> Up to now we tried to call printf() only once where ANSI_* is only one >>>>>> part of the output string. >>>>>> >>>>>> E.g. cmd/eficonfig.c:415: >>>>>> printf(ANSI_CLEAR_CONSOLE >>>>>> ANSI_CURSOR_POSITION >>>>>> ANSI_CURSOR_SHOW, 1, 1); >>>>>> >>>>>> This minimizes the number of call library calls but may not be very elegant. >>>>>> >>>>>> Creating a library function for ANSI_CURSOR_SHOW alone does not make >>>>>> much sense to me. Should you want to convert our code base from using >>>>>> ANSI_* to library functions this should cover the whole code base. >>>>>> >>>>>> So I think we should let this patch pass as it solves a current problem >>>>>> and leave creating a ANSI_* function library to a separate exercise. >>>>> >>>>> Then we should add this to the serial API...what we have here is quite >>>>> bizarre in a way, since it outputs escape characters as the default >>>>> for U-Boot. Mostly we don't need it. >>>>> >>>>> So add a set_cursor_visible() method to the serial API and allow it to >>>>> be controlled via platform data in the serial driver. >>>> >>>> I have no clue which platform data you might be thinking of. >>>> >>>> It is also not my intention to eject the escape sequence whenever a >>>> serial console is probed. >>>> >>>> Using serial_puts() in this patch instead of printf() would help to >>>> avoid the CONFIG dependency. >>> >>> Here's what should happen: >>> >>> 1. ANSI codes should not appear in logs or on CI (this is currently a >>> problem with EFI tests) >> >> A program outputting different things in continuous integration then in >> real live does not provide realistic testing. >> >> It is the CI that has to adapt not the program under test. >> >> Which logs are you talking about? >> >>> 2. We cannot show an ANSI code on boot by default, since that puts >>> ctrl characters into every start-up of U-Boot >> >> Why would that be a problem? >> >>> >>> So I think my suggestion makes sense. See drivers/serial/sandbox.c and >>> have a way of calling isatty() to check the terminal type (with a >>> cmdline flag to override it, see -t for example). Then we can get the >>> correct behaviour. >> >> isatty() is not a U-Boot function. So it is irrelevant in this context. >> >> You cannot detect if a serial console understands ANSI sequences without >> sending any. > > NAK > > Please try a little harder to understand my POV above. I don't understand what your point of view as you did not answer my question. The only case of logs being written in a production U-Boot is given by CONGIG_LOG_SYSLOG=y which is not affected by the patch. So no clue what logs you refer to. Concerning CI it is clear that the CI has to handle ANSI sequences in the U-Boot output sensibly. The idea of U-Boot producing different output running in CI than outside CI would contradict the purpose of the CI. I further have no clue what problem you face with the output of the EFI unit tests. Could you, please, be more specific. Best regards Heinrich
Hi Heinrich, On Mon, 31 Oct 2022 at 15:58, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 10/31/22 20:27, Simon Glass wrote: > > Hi Heinrich, > > > > On Sat, 29 Oct 2022 at 20:56, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> > >> > >> On 10/30/22 02:43, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt > >>> <heinrich.schuchardt@canonical.com> wrote: > >>>> > >>>> > >>>> > >>>> On 10/27/22 17:22, Simon Glass wrote: > >>>>> Hi Heinrich, > >>>>> > >>>>> On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt > >>>>> <heinrich.schuchardt@canonical.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 10/26/22 01:35, Simon Glass wrote: > >>>>>>> Hi Heinrich, > >>>>>>> > >>>>>>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt > >>>>>>> <heinrich.schuchardt@canonical.com> wrote: > >>>>>>>> > >>>>>>>> We may enter the command line interface in a state where on the remote > >>>>>>>> console the cursor is not shown. Send an escape sequence to enable it. > >>>>>>>> > >>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>>>>>>> --- > >>>>>>>> common/main.c | 4 ++++ > >>>>>>>> 1 file changed, 4 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/common/main.c b/common/main.c > >>>>>>>> index 682f3359ea..4e7e7b17d6 100644 > >>>>>>>> --- a/common/main.c > >>>>>>>> +++ b/common/main.c > >>>>>>>> @@ -7,6 +7,7 @@ > >>>>>>>> /* #define DEBUG */ > >>>>>>>> > >>>>>>>> #include <common.h> > >>>>>>>> +#include <ansi.h> > >>>>>>>> #include <autoboot.h> > >>>>>>>> #include <bootstage.h> > >>>>>>>> #include <cli.h> > >>>>>>>> @@ -66,6 +67,9 @@ void main_loop(void) > >>>>>>>> > >>>>>>>> autoboot_command(s); > >>>>>>>> > >>>>>>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) > >>>>>>>> + printf(ANSI_CURSOR_SHOW "\n"); > >>>>>>> > >>>>>>> Could we create a library which emits these codes? Like ansi_cursor_show() ? > >>>>>> > >>>>>> The question is not if we could but if we should. > >>>>>> > >>>>>> Up to now we tried to call printf() only once where ANSI_* is only one > >>>>>> part of the output string. > >>>>>> > >>>>>> E.g. cmd/eficonfig.c:415: > >>>>>> printf(ANSI_CLEAR_CONSOLE > >>>>>> ANSI_CURSOR_POSITION > >>>>>> ANSI_CURSOR_SHOW, 1, 1); > >>>>>> > >>>>>> This minimizes the number of call library calls but may not be very elegant. > >>>>>> > >>>>>> Creating a library function for ANSI_CURSOR_SHOW alone does not make > >>>>>> much sense to me. Should you want to convert our code base from using > >>>>>> ANSI_* to library functions this should cover the whole code base. > >>>>>> > >>>>>> So I think we should let this patch pass as it solves a current problem > >>>>>> and leave creating a ANSI_* function library to a separate exercise. > >>>>> > >>>>> Then we should add this to the serial API...what we have here is quite > >>>>> bizarre in a way, since it outputs escape characters as the default > >>>>> for U-Boot. Mostly we don't need it. > >>>>> > >>>>> So add a set_cursor_visible() method to the serial API and allow it to > >>>>> be controlled via platform data in the serial driver. > >>>> > >>>> I have no clue which platform data you might be thinking of. > >>>> > >>>> It is also not my intention to eject the escape sequence whenever a > >>>> serial console is probed. > >>>> > >>>> Using serial_puts() in this patch instead of printf() would help to > >>>> avoid the CONFIG dependency. > >>> > >>> Here's what should happen: > >>> > >>> 1. ANSI codes should not appear in logs or on CI (this is currently a > >>> problem with EFI tests) > >> > >> A program outputting different things in continuous integration then in > >> real live does not provide realistic testing. > >> > >> It is the CI that has to adapt not the program under test. > >> > >> Which logs are you talking about? > >> > >>> 2. We cannot show an ANSI code on boot by default, since that puts > >>> ctrl characters into every start-up of U-Boot > >> > >> Why would that be a problem? > >> > >>> > >>> So I think my suggestion makes sense. See drivers/serial/sandbox.c and > >>> have a way of calling isatty() to check the terminal type (with a > >>> cmdline flag to override it, see -t for example). Then we can get the > >>> correct behaviour. > >> > >> isatty() is not a U-Boot function. So it is irrelevant in this context. > >> > >> You cannot detect if a serial console understands ANSI sequences without > >> sending any. > > > > NAK > > > > Please try a little harder to understand my POV above. > > I don't understand what your point of view as you did not answer my > question. > > The only case of logs being written in a production U-Boot is given by > CONGIG_LOG_SYSLOG=y which is not affected by the patch. So no clue what > logs you refer to. If you run the tests (e.g. make check) and get a failure you can see the output from them on the console. At the moment some tests clear the console, change colour and all sorts of things. That is not good behaviour. Can you see that? > > Concerning CI it is clear that the CI has to handle ANSI sequences in > the U-Boot output sensibly. The idea of U-Boot producing different > output running in CI than outside CI would contradict the purpose of the CI. Have you actually looked at the CI output? I am attaching it at the end of this email so you can see it. > > I further have no clue what problem you face with the output of the EFI > unit tests. Could you, please, be more specific. That is a whole other topic. We definitely need more C tests and fewer of these large Python end-to-end tests. But for now, all I am saying is that ANSI characters should not be output in CI. For sandbox they can be output but only if sending to a terminal, or specifically enabled with a flag. Tests that clear the console or output thousands of lines of gibberish output are not acceptable in my book. Regards, Simon output: $ pyt eficonfig ================================================ test session starts ================================================= platform linux -- Python 3.10.6, pytest-6.2.5, py-1.10.0, pluggy-0.13.0 rootdir: /scratch/sglass/cosarm/src/third_party/u-boot/files/test/py, configfile: pytest.ini plugins: hypothesis-6.36.0, forked-1.4.0, xdist-2.5.0 collected 1022 items / 1021 deselected / 1 selected test/py/tests/test_eficonfig/test_eficonfig.py F [100%] ** UEFI Maintenance Menu ** Add Boot Option Edit Boot Option Change Boot Order Delete Boot Option Quit ====================================================== FAILURES ====================================================== _________________________________________________ test_efi_eficonfig
On 11/1/22 00:52, Simon Glass wrote: > Hi Heinrich, > > On Mon, 31 Oct 2022 at 15:58, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> >> >> On 10/31/22 20:27, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Sat, 29 Oct 2022 at 20:56, Heinrich Schuchardt >>> <heinrich.schuchardt@canonical.com> wrote: >>>> >>>> >>>> >>>> On 10/30/22 02:43, Simon Glass wrote: >>>>> Hi Heinrich, >>>>> >>>>> On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt >>>>> <heinrich.schuchardt@canonical.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 10/27/22 17:22, Simon Glass wrote: >>>>>>> Hi Heinrich, >>>>>>> >>>>>>> On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt >>>>>>> <heinrich.schuchardt@canonical.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 10/26/22 01:35, Simon Glass wrote: >>>>>>>>> Hi Heinrich, >>>>>>>>> >>>>>>>>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt >>>>>>>>> <heinrich.schuchardt@canonical.com> wrote: >>>>>>>>>> >>>>>>>>>> We may enter the command line interface in a state where on the remote >>>>>>>>>> console the cursor is not shown. Send an escape sequence to enable it. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>>>>>>>> --- >>>>>>>>>> common/main.c | 4 ++++ >>>>>>>>>> 1 file changed, 4 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/common/main.c b/common/main.c >>>>>>>>>> index 682f3359ea..4e7e7b17d6 100644 >>>>>>>>>> --- a/common/main.c >>>>>>>>>> +++ b/common/main.c >>>>>>>>>> @@ -7,6 +7,7 @@ >>>>>>>>>> /* #define DEBUG */ >>>>>>>>>> >>>>>>>>>> #include <common.h> >>>>>>>>>> +#include <ansi.h> >>>>>>>>>> #include <autoboot.h> >>>>>>>>>> #include <bootstage.h> >>>>>>>>>> #include <cli.h> >>>>>>>>>> @@ -66,6 +67,9 @@ void main_loop(void) >>>>>>>>>> >>>>>>>>>> autoboot_command(s); >>>>>>>>>> >>>>>>>>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) >>>>>>>>>> + printf(ANSI_CURSOR_SHOW "\n"); >>>>>>>>> >>>>>>>>> Could we create a library which emits these codes? Like ansi_cursor_show() ? >>>>>>>> >>>>>>>> The question is not if we could but if we should. >>>>>>>> >>>>>>>> Up to now we tried to call printf() only once where ANSI_* is only one >>>>>>>> part of the output string. >>>>>>>> >>>>>>>> E.g. cmd/eficonfig.c:415: >>>>>>>> printf(ANSI_CLEAR_CONSOLE >>>>>>>> ANSI_CURSOR_POSITION >>>>>>>> ANSI_CURSOR_SHOW, 1, 1); >>>>>>>> >>>>>>>> This minimizes the number of call library calls but may not be very elegant. >>>>>>>> >>>>>>>> Creating a library function for ANSI_CURSOR_SHOW alone does not make >>>>>>>> much sense to me. Should you want to convert our code base from using >>>>>>>> ANSI_* to library functions this should cover the whole code base. >>>>>>>> >>>>>>>> So I think we should let this patch pass as it solves a current problem >>>>>>>> and leave creating a ANSI_* function library to a separate exercise. >>>>>>> >>>>>>> Then we should add this to the serial API...what we have here is quite >>>>>>> bizarre in a way, since it outputs escape characters as the default >>>>>>> for U-Boot. Mostly we don't need it. >>>>>>> >>>>>>> So add a set_cursor_visible() method to the serial API and allow it to >>>>>>> be controlled via platform data in the serial driver. >>>>>> >>>>>> I have no clue which platform data you might be thinking of. >>>>>> >>>>>> It is also not my intention to eject the escape sequence whenever a >>>>>> serial console is probed. >>>>>> >>>>>> Using serial_puts() in this patch instead of printf() would help to >>>>>> avoid the CONFIG dependency. >>>>> >>>>> Here's what should happen: >>>>> >>>>> 1. ANSI codes should not appear in logs or on CI (this is currently a >>>>> problem with EFI tests) >>>> >>>> A program outputting different things in continuous integration then in >>>> real live does not provide realistic testing. >>>> >>>> It is the CI that has to adapt not the program under test. >>>> >>>> Which logs are you talking about? >>>> >>>>> 2. We cannot show an ANSI code on boot by default, since that puts >>>>> ctrl characters into every start-up of U-Boot >>>> >>>> Why would that be a problem? >>>> >>>>> >>>>> So I think my suggestion makes sense. See drivers/serial/sandbox.c and >>>>> have a way of calling isatty() to check the terminal type (with a >>>>> cmdline flag to override it, see -t for example). Then we can get the >>>>> correct behaviour. >>>> >>>> isatty() is not a U-Boot function. So it is irrelevant in this context. >>>> >>>> You cannot detect if a serial console understands ANSI sequences without >>>> sending any. >>> >>> NAK >>> >>> Please try a little harder to understand my POV above. >> >> I don't understand what your point of view as you did not answer my >> question. >> >> The only case of logs being written in a production U-Boot is given by >> CONGIG_LOG_SYSLOG=y which is not affected by the patch. So no clue what >> logs you refer to. > > If you run the tests (e.g. make check) and get a failure you can see > the output from them on the console. At the moment some tests clear > the console, change colour and all sorts of things. That is not good > behaviour. Can you see that? None of the tests in lib/efi_selftests clears the screen. To provide better readability they use colored output which should not pose any problem if your console in ANSI enabled. To view logs with ANSI colors correctly displayed just use the 'less -rf' command. For the bootmenu there is a test that invokes the bootmenu command which clears the screen. This is unavoidable for testing. Inside the CI that does not cause problems bug you may dislike it when running the tests manually. We could think of adding some property to the test to allow you to exclude the test more easily. > >> >> Concerning CI it is clear that the CI has to handle ANSI sequences in >> the U-Boot output sensibly. The idea of U-Boot producing different >> output running in CI than outside CI would contradict the purpose of the CI. > > Have you actually looked at the CI output? I am attaching it at the > end of this email so you can see it. > >> >> I further have no clue what problem you face with the output of the EFI >> unit tests. Could you, please, be more specific. > > That is a whole other topic. We definitely need more C tests and fewer > of these large Python end-to-end tests. > > But for now, all I am saying is that ANSI characters should not be > output in CI. For sandbox they can be output but only if sending to a > terminal, or specifically enabled with a flag. Tests that clear the > console or output thousands of lines of gibberish output are not > acceptable in my book. In the log I see no gibberish but the actual screen output. If you think the output is too verbose, we must find a way to exclude the test. Best regards Heinrich > > Regards, > Simon
Hi Heinrich, On Tue, 1 Nov 2022 at 03:03, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 11/1/22 00:52, Simon Glass wrote: > > Hi Heinrich, > > > > On Mon, 31 Oct 2022 at 15:58, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> > >> > >> On 10/31/22 20:27, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Sat, 29 Oct 2022 at 20:56, Heinrich Schuchardt > >>> <heinrich.schuchardt@canonical.com> wrote: > >>>> > >>>> > >>>> > >>>> On 10/30/22 02:43, Simon Glass wrote: > >>>>> Hi Heinrich, > >>>>> > >>>>> On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt > >>>>> <heinrich.schuchardt@canonical.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 10/27/22 17:22, Simon Glass wrote: > >>>>>>> Hi Heinrich, > >>>>>>> > >>>>>>> On Wed, 26 Oct 2022 at 00:13, Heinrich Schuchardt > >>>>>>> <heinrich.schuchardt@canonical.com> wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> On 10/26/22 01:35, Simon Glass wrote: > >>>>>>>>> Hi Heinrich, > >>>>>>>>> > >>>>>>>>> On Sat, 22 Oct 2022 at 03:21, Heinrich Schuchardt > >>>>>>>>> <heinrich.schuchardt@canonical.com> wrote: > >>>>>>>>>> > >>>>>>>>>> We may enter the command line interface in a state where on the remote > >>>>>>>>>> console the cursor is not shown. Send an escape sequence to enable it. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>>>>>>>>> --- > >>>>>>>>>> common/main.c | 4 ++++ > >>>>>>>>>> 1 file changed, 4 insertions(+) > >>>>>>>>>> > >>>>>>>>>> diff --git a/common/main.c b/common/main.c > >>>>>>>>>> index 682f3359ea..4e7e7b17d6 100644 > >>>>>>>>>> --- a/common/main.c > >>>>>>>>>> +++ b/common/main.c > >>>>>>>>>> @@ -7,6 +7,7 @@ > >>>>>>>>>> /* #define DEBUG */ > >>>>>>>>>> > >>>>>>>>>> #include <common.h> > >>>>>>>>>> +#include <ansi.h> > >>>>>>>>>> #include <autoboot.h> > >>>>>>>>>> #include <bootstage.h> > >>>>>>>>>> #include <cli.h> > >>>>>>>>>> @@ -66,6 +67,9 @@ void main_loop(void) > >>>>>>>>>> > >>>>>>>>>> autoboot_command(s); > >>>>>>>>>> > >>>>>>>>>> + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) > >>>>>>>>>> + printf(ANSI_CURSOR_SHOW "\n"); > >>>>>>>>> > >>>>>>>>> Could we create a library which emits these codes? Like ansi_cursor_show() ? > >>>>>>>> > >>>>>>>> The question is not if we could but if we should. > >>>>>>>> > >>>>>>>> Up to now we tried to call printf() only once where ANSI_* is only one > >>>>>>>> part of the output string. > >>>>>>>> > >>>>>>>> E.g. cmd/eficonfig.c:415: > >>>>>>>> printf(ANSI_CLEAR_CONSOLE > >>>>>>>> ANSI_CURSOR_POSITION > >>>>>>>> ANSI_CURSOR_SHOW, 1, 1); > >>>>>>>> > >>>>>>>> This minimizes the number of call library calls but may not be very elegant. > >>>>>>>> > >>>>>>>> Creating a library function for ANSI_CURSOR_SHOW alone does not make > >>>>>>>> much sense to me. Should you want to convert our code base from using > >>>>>>>> ANSI_* to library functions this should cover the whole code base. > >>>>>>>> > >>>>>>>> So I think we should let this patch pass as it solves a current problem > >>>>>>>> and leave creating a ANSI_* function library to a separate exercise. > >>>>>>> > >>>>>>> Then we should add this to the serial API...what we have here is quite > >>>>>>> bizarre in a way, since it outputs escape characters as the default > >>>>>>> for U-Boot. Mostly we don't need it. > >>>>>>> > >>>>>>> So add a set_cursor_visible() method to the serial API and allow it to > >>>>>>> be controlled via platform data in the serial driver. > >>>>>> > >>>>>> I have no clue which platform data you might be thinking of. > >>>>>> > >>>>>> It is also not my intention to eject the escape sequence whenever a > >>>>>> serial console is probed. > >>>>>> > >>>>>> Using serial_puts() in this patch instead of printf() would help to > >>>>>> avoid the CONFIG dependency. > >>>>> > >>>>> Here's what should happen: > >>>>> > >>>>> 1. ANSI codes should not appear in logs or on CI (this is currently a > >>>>> problem with EFI tests) > >>>> > >>>> A program outputting different things in continuous integration then in > >>>> real live does not provide realistic testing. > >>>> > >>>> It is the CI that has to adapt not the program under test. > >>>> > >>>> Which logs are you talking about? > >>>> > >>>>> 2. We cannot show an ANSI code on boot by default, since that puts > >>>>> ctrl characters into every start-up of U-Boot > >>>> > >>>> Why would that be a problem? > >>>> > >>>>> > >>>>> So I think my suggestion makes sense. See drivers/serial/sandbox.c and > >>>>> have a way of calling isatty() to check the terminal type (with a > >>>>> cmdline flag to override it, see -t for example). Then we can get the > >>>>> correct behaviour. > >>>> > >>>> isatty() is not a U-Boot function. So it is irrelevant in this context. > >>>> > >>>> You cannot detect if a serial console understands ANSI sequences without > >>>> sending any. > >>> > >>> NAK > >>> > >>> Please try a little harder to understand my POV above. > >> > >> I don't understand what your point of view as you did not answer my > >> question. > >> > >> The only case of logs being written in a production U-Boot is given by > >> CONGIG_LOG_SYSLOG=y which is not affected by the patch. So no clue what > >> logs you refer to. > > > > If you run the tests (e.g. make check) and get a failure you can see > > the output from them on the console. At the moment some tests clear > > the console, change colour and all sorts of things. That is not good > > behaviour. Can you see that? > > None of the tests in lib/efi_selftests clears the screen. To provide > better readability they use colored output which should not pose any > problem if your console in ANSI enabled. To view logs with ANSI colors > correctly displayed just use the 'less -rf' command. Running less separately is too fiddly. It should show up properly in gitlab as other tests do. This needs to be designed for humans, not computers. That is my criticism of so much of the EFI stuff. > > For the bootmenu there is a test that invokes the bootmenu command which > clears the screen. This is unavoidable for testing. Inside the CI that > does not cause problems bug you may dislike it when running the tests > manually. We could think of adding some property to the test to allow > you to exclude the test more easily. The test should not clear the screen. All of that ANSI stuff should be suppressed (by default) when testing > > > > >> > >> Concerning CI it is clear that the CI has to handle ANSI sequences in > >> the U-Boot output sensibly. The idea of U-Boot producing different > >> output running in CI than outside CI would contradict the purpose of the CI. > > > > Have you actually looked at the CI output? I am attaching it at the > > end of this email so you can see it. > > > >> > >> I further have no clue what problem you face with the output of the EFI > >> unit tests. Could you, please, be more specific. > > > > That is a whole other topic. We definitely need more C tests and fewer > > of these large Python end-to-end tests. > > > > But for now, all I am saying is that ANSI characters should not be > > output in CI. For sandbox they can be output but only if sending to a > > terminal, or specifically enabled with a flag. Tests that clear the > > console or output thousands of lines of gibberish output are not > > acceptable in my book. > > In the log I see no gibberish but the actual screen output. If you think > the output is too verbose, we must find a way to exclude the test. About 4000 of the 4600 lines are blank! Here is what I think you should do: - add a flag to sandbox (like the existing -t) which tells U-Boot whether to send ANSI output to serial output -a auto - use ANSI output if a terminal is detected (isatty()) - this should be the default -a ansi - output ANSI always -a none - don't output ANSI - flag is checked in sandbox serial driver, like -t is now - can create per-device uclass data containing the setting, so all serial drivers are affected - the ANSI stuff is handled in the serial driver with a new call added to struct dm_serial_ops - ANSI sequences are handled in C so they can be sent or not sent (perhaps there is a clever way of using printf() format characters like %sC to clear the screen?) Regards, Simon
On Saturday 22 October 2022 11:20:58 Heinrich Schuchardt wrote: > We may enter the command line interface in a state where on the remote > console the cursor is not shown. Send an escape sequence to enable it. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > common/main.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/common/main.c b/common/main.c > index 682f3359ea..4e7e7b17d6 100644 > --- a/common/main.c > +++ b/common/main.c > @@ -7,6 +7,7 @@ > /* #define DEBUG */ > > #include <common.h> > +#include <ansi.h> > #include <autoboot.h> > #include <bootstage.h> > #include <cli.h> > @@ -66,6 +67,9 @@ void main_loop(void) > > autoboot_command(s); > > + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) > + printf(ANSI_CURSOR_SHOW "\n"); I think that this does not work anymore. Support for "h"/"l" ansi escape sequences to show and hide cursor was removed from U-Boot together with cfg_console.c driver done by Simon for v2022.07 version. Now I verified that cursor is really permanently turned off (meaning unsupported) on Nokia N900 even when CONFIG_VIDEO_ANSI is enabled. So prior adding ANSI_CURSOR_SHOW on new places, I would suggest to first revert and implement functions for showing and hiding cursor. > + > cli_loop(); > panic("No CLI available"); > } > -- > 2.37.2 >
diff --git a/common/main.c b/common/main.c index 682f3359ea..4e7e7b17d6 100644 --- a/common/main.c +++ b/common/main.c @@ -7,6 +7,7 @@ /* #define DEBUG */ #include <common.h> +#include <ansi.h> #include <autoboot.h> #include <bootstage.h> #include <cli.h> @@ -66,6 +67,9 @@ void main_loop(void) autoboot_command(s); + if (!CONFIG_IS_ENABLED(DM_VIDEO) || CONFIG_IS_ENABLED(VIDEO_ANSI)) + printf(ANSI_CURSOR_SHOW "\n"); + cli_loop(); panic("No CLI available"); }
We may enter the command line interface in a state where on the remote console the cursor is not shown. Send an escape sequence to enable it. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- common/main.c | 4 ++++ 1 file changed, 4 insertions(+)