[PATCH 1/1] cli: always show cursor

[Heinrich Schuchardt]

On 10/30/22 02:43, Simon Glass wrote:
> Hi Heinrich,
> 
> On Thu, 27 Oct 2022 at 10:41, Heinrich Schuchardt
> <heinrich.schuchardt at 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 at 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 at 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 at 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