[PATCH 1/1] cli: always show cursor

[Heinrich Schuchardt]

On 10/31/22 20:27, Simon Glass wrote:
> Hi Heinrich,
> 
> On Sat, 29 Oct 2022 at 20:56, Heinrich Schuchardt
> <heinrich.schuchardt at 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 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.
> 
> 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