Re: [PATCH v6 0/6] Implement fastboot multiresponce

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Ion,

On jeu., janv. 04, 2024 at 17:44, Ion Agorria <ionpl9@gmail.com> wrote:

> Hello,
>
> It seems like without the fix the ut_assert_skipline(); didn't clear
> console and running ut_assert_skipline(); many times would give always
> OK. With my fix the line is cleared correctly and next assert fails
> because now there is nothing to clean which is correct if we look the
> this a bit above the failing assert:
>
>        if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {
>                /*
>                 * For some strange reasons, the console is not empty after
>                 * running above command.
>                 * So, we reset it to not have side effects for other tests.
>                 */
>                console_record_reset_enable();
>        } else if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {
>                ut_assert_console_end();
>        }
>
> Which further confirms that tests workaround the old problem and now
> that problem is fixed we can remove the whole if blocks and simply
> place ut_assert_console_end() right after ut_assert_skipline() without
> any conditional and will pass green.
>
> So this part of code goes from:
>        ut_assert_skipline();
>        ut_assert_skipline();
>
>        if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {
>                /* See above comments. */
>                console_record_reset_enable();
>        } else if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {
>                ut_assert_console_end();
>        }
>
> to become:
>        ut_assert_skipline();
>        ut_assert_console_end();
>
> Same thing should be done with the if block mentioned at start of
> email that calls console_record_reset_enable().

That makes sense. Thank you for looking into this. I see that Svyatoslav
included your suggestion in

https://patchwork.ozlabs.org/project/uboot/patch/20240105072212.6615-8-clamor95@gmail.com/

I will review it there.

>
> Regards,
> Ion
>
> El jue, 4 ene 2024 a las 17:15, Svyatoslav Ryhel
> (<clamor95@gmail.com>) escribió:
>>
>> чт, 4 січ. 2024 р. о 17:00 Mattijs Korpershoek <mkorpershoek@baylibre.com> пише:
>> >
>> > Hello Svyatoslav,
>> >
>> > On jeu., déc. 28, 2023 at 20:01, Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>> >
>> > > Currently u-boot fastboot can only send one message back to host,
>> > > so if there is a need to print more than one line messages must be
>> > > kept sending until all the required data is obtained. This behavior
>> > > can be adjusted using multiresponce ability (getting multiple lines
>> > > of response) proposed in this patch set.
>> > >
>> > > Implementation of multiresponce leads to ability to dump content of
>> > > console buffer which, with use of "oem run", allows to entirely avoid
>> > > need in UART.
>> > >
>> > > ---
>> > > Changes in v6:
>> > >  - remove prev console changes
>> > >  - add console_record_isempty helper
>> > >  - set record flag on init
>> > >
>> > > Changes in v5:
>> > >  - restored membuff_readline behavior changed in v4
>> > >
>> > > Changes in v4:
>> > >  - adjust membuff_readline behavior with overflow
>> > >
>> > > Changes in v3:
>> > >  - fix missing function calls if CONFIG_CONSOLE_RECORD is not enabled
>> > >
>> > > Changes in v2:
>> > >  - changed variables to static
>> > >  - fixed multiresponce for udp
>> > >  - documented use of "MORE"
>> > >  - converted #if to if (...)
>> > > ---
>> > >
>> > > Ion Agorria (6):
>> > >   fastboot: multiresponse support
>> > >   fastboot: implement "getvar all"
>> > >   common: console: introduce console_record_isempty helper
>> > >   common: console: record console from the beginning
>> > >   lib: membuff: fix readline not returning line in case of overflow
>> > >   fastboot: add oem console command support
>> >
>> > I went on to apply the series, however CI detected a regression in the
>> > unit tests:
>> >
>> > https://source.denx.de/u-boot/custodians/u-boot-dfu/-/jobs/764396
>> >
>> > This can be reproduced with:
>> > $ ./test/py/test.py --bd sandbox --build -k ut_hush_hush_test_simple_dollar
>> >
>> > I've narrowed this down to:
>> >  [PATCH v6 5/6] lib: membuff: fix readline not returning line in case of overflow
>> >
>> > Could you please have a look?
>> >
>> > Thank you!
>> >
>>
>> Test contains 2 skiplines, commenting one fixes test
>>
>> ut_asserteq(1, run_command("dollar_foo=bar quux\"", 0));
>>         /* Two next lines contain error message */
>>         ut_assert_skipline();
>>         ut_assert_skipline();
>>
>> > >
>> > >  boot/bootmeth_extlinux.c          |  2 +-
>> > >  common/console.c                  | 10 +++-
>> > >  doc/android/fastboot-protocol.rst |  3 ++
>> > >  doc/android/fastboot.rst          |  1 +
>> > >  drivers/fastboot/Kconfig          |  7 +++
>> > >  drivers/fastboot/fb_command.c     | 52 +++++++++++++++++++++
>> > >  drivers/fastboot/fb_getvar.c      | 77 +++++++++++++++++++++++++------
>> > >  drivers/usb/gadget/f_fastboot.c   | 29 ++++++++++++
>> > >  include/console.h                 | 13 ++++++
>> > >  include/fastboot-internal.h       |  7 +++
>> > >  include/fastboot.h                | 19 ++++++++
>> > >  include/membuff.h                 |  5 +-
>> > >  lib/membuff.c                     |  4 +-
>> > >  net/fastboot_udp.c                | 29 +++++++++---
>> > >  14 files changed, 233 insertions(+), 25 deletions(-)
>> > >
>> > > --
>> > > 2.40.1