On Thu, Oct 31, 2024 at 07:03:19PM +0100, Simon Glass wrote: > Hi Tom, > > On Fri, 27 Sept 2024 at 04:52, Tom Rini <tr...@konsulko.com> wrote: > > > > On Thu, Sep 26, 2024 at 11:36:00PM +0200, Simon Glass wrote: > > > Hi Tom, > > > > > > On Wed, 25 Sept 2024 at 19:26, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > On Wed, Sep 25, 2024 at 02:49:56PM +0200, Simon Glass wrote: > > > > > Hi Tom, > > > > > > > > > > On Mon, 23 Sept 2024 at 22:35, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > On Fri, Sep 20, 2024 at 08:01:36AM +0200, Simon Glass wrote: > > > > > > > > > > > > > > > > > > > When Labgrid is used, it can get U-Boot ready for running tests. > > > > > > > It > > > > > > > prints a message when it has done so. > > > > > > > > > > > > > > Add logic to detect this message and accept it. > > > > > > > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > > > > --- > > > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > > > test/py/u_boot_console_base.py | 9 +++++---- > > > > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > > > > > What happens is that labgrid can also be told to look for and then > > > > > > interrupt autoboot, just like our pytests can do, and the system is > > > > > > at > > > > > > the prompt. But this is also what it's like for a system with > > > > > > autoboot > > > > > > disabled. Do we actually need this patch to achieve the > > > > > > functionality > > > > > > you want? Doesn't that already just happen? > > > > > > > > > > The point of this patch is actually to remove code in pytest, by > > > > > allowing it to skip all the banner-detection stuff. It does not affect > > > > > things in Labgrid, since it still needs to watch for banners, etc. > > > > > > > > But you can't remove code from pytest, people can and will run the suite > > > > outside of labgrid. > > > > > > Yes, that's right. I meant that with Labgrid the code is not used. > > > > OK. I still think this (and I assume the related flag to tell whichever > > helper that was that the platform is ready to go) are too implementation > > specific. > > OK, but it does have the virtue of working properly on all the boards > I have, rather than just a subset, without this patch. > > > > > > > > Without this patch, we have to tell Labgrid's U-Boot driver to do > > > > > nothing, so that pytest does it. But that is not a good idea, since > > > > > Labgrid has a lot more info about the board than pytest has. For > > > > > example, look at all the SPL-banner-count stuff. > > > > > > > > Why do you have to tell it to do nothing? The pytest suite works fine, > > > > today, if the board stops at the prompt automatically. To be clear, the > > > > labgrid yaml file I'm using with my scripts has the information to stop > > > > autoboot in it and it's not causing a problem. > > > > > > But if it wasn't causing a problem I would not have invented this > > > annoying scheme. > > > > Well, I honestly thought it might be a remnant from development that > > wasn't needed once everything else was done. I do that from time to time > > myself. > > OK. > > > > > > For several boards, by the time pytest gets to see > > > the output it is too late to press a key and stop. > > > > Why / how? Do we perhaps need to adjust the timeout upwards, slightly? > > Especially in light of the changes you also did to detect a "dead" board > > and so we don't wait 30 minutes for a test run to fail. > > Why / how - because the board prints its SPL banner as soon as reset > is released, then waits for U-Boot proper to be sent, at which case > the U-Boot banner is sent .Then labgrid disconnects from the console > and runs microcom, which connects to the console over ser2net and > misses some of what has already been sent. > > This is actually a fundamental problem, not something we can adjust > with timeouts.
Wait, what? *Labgrid* is the thing that's the problem here with dropping the connection to the board and re-establishing it at some point? That is the kind of detail which (a) needs to be in the commit message (b) perhaps an issue filed (or if already exists Link:'d in the commit message) and (c) would have made it clear why we need this work-around and then I'd have been a lot happier to accept it as a work-around back on v1 or so. > > > Also, some boards > > > have a different prompt which is not detected by pytest. > > > > > > For example: > > > configs/am62x_beagleplay_a53_defconfig:CONFIG_AUTOBOOT_PROMPT="Press > > > SPACE to abort autoboot in %d seconds\n" > > > > Yeah, I had forgotten that as I turn that off along with turning on > > other tests via config fragment, on that platform. That really is a > > deficiency in our pytest version of this. > > OK > > > > > > > > Basically, without this patch we cannot use '-s uboot' to tell the > > > > > Labgrid strategy to take us to a U-Boot prompt. We must just use a raw > > > > > console with no strategy, relying on pytest to do all the work. > > > > > > > > > > I hope that helps explain the problem? > > > > > > > > I think I see what you're saying, and it's based on the assumption that > > > > we'll make everyone either use labgrid? > > > > > > Not at all. I tested this version of the series again with my > > > pre-Labgrid lab and it works fine. But I do believe that the hooks > > > that pytest has are not ideal for use with Labgrid. > > > > OK. But, why can't you make use of the labgrid functionality to pause > > the board? It's the state flag for labgrid-client yes? Dropping that in > > with my labgrid support would be just a tweak to the console script to > > check if the board set labgrid_strategy or something, and I think you > > could adapt yours to that too? > > I am really not sure what to say here. I have explained the reality of > the situation. I have spent hours debugging it and figuring out the > root cause. There is no other tweak I can think of that will solve > this problem. Well, I hate to get on a tangent here, but I think a common thread is that after you've spent hours debugging a problem and working out a solution you then omit much of the details. I'd much rather see a much longer commit message here describing the problem, or a longer description under the "---" where you explain that labgrid uses minicom, gets things up, drops the connection and uses ser2net and so there's a chance we miss messages and so need something like this. Or, that you've been debugging on *x86_64* a bunch of EFI_LOADER things and off the shelf distributions aren't working (and ExitBootServices() is where you had a problem with Ubuntu I gather still on x86_64). > > Regards, > Simon -- Tom
signature.asc
Description: PGP signature
