Rahix / tbot

Automation/Testing tool for Embedded Linux Development
https://tbot.tools
GNU General Public License v3.0
84 stars 21 forks source link

machine: u-boot: Add a hacky workaround for the crc32 command #112

Open Rahix opened 6 months ago

Rahix commented 6 months ago

The crc32 command in U-Boot unfortunately prints the character sequence => as part of its output. This character sequence is an often used U-Boot prompt, which means tbot's prompt-searching code gets confused by crc32 output.

Unfortunately, there is no real solution to this. At least none that works universally. So to at least ease the pain a bit for people running into this, attempt automatically applying a workaround for this specific situation.

Closes #111.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (910eeb7) 62.38% compared to head (fe78bfa) 62.53%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #112 +/- ## ========================================== + Coverage 62.38% 62.53% +0.15% ========================================== Files 53 53 Lines 3719 3729 +10 ========================================== + Hits 2320 2332 +12 + Misses 1399 1397 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Rahix commented 6 months ago

Hey @Adnan-Elhammoudi, I've been thinking about #111 for a bit. I came up with this hacky "solution" to make things a bit easier. Could you give this a test, to see whether it works as well as the workaround suggested in the issue? I.e. drop the workaround you added and rerun with the bare crc32 command. tbot should print a warning about it, but make it work correctly on its own.

Adnan-Elhammoudi commented 6 months ago

Hey @Rahix, it works as a charm, with the bare crc32 command:

        fileaddr =  ub.env("fileaddr")
        filesize =  ub.env("filesize")
        output = ub.exec0("crc32", f"{fileaddr}" ,f"{filesize}")
        assert expected_crc in output

but it does not drop any warning!

Rahix commented 6 months ago

Hm, that's unexpected! Are you really sure there is no Applying workaround for `crc32` command in U-Boot! message anywhere? If not, what is your prompt string for U-Boot? Maybe you have changed it to something different than prompt = "=> "?

Adnan-Elhammoudi commented 6 months ago

oh, I was on another board my bad! I can see the message now!

│   ├─[device-u-boot] printenv fileaddr
│   │    ## 
│   │    ## fileaddr=80280000
│   ├─[device-u-boot] printenv filesize
│   │    ## 
│   │    ## filesize=2c
│   ├─Warning: Applying workaround for `crc32` command in U-Boot!
│   │   See https://github.com/rahix/tbot/issues/111 for details.
│   ├─[device-u-boot] crc32 '=80280000␍' '=2c␍'
│   │    ## 
│   │    ## > '
│   │    ## crc32 for 00000000 ... ffffffffffffffff ==> 00000000
FAILEDPOWEROFF (imx8q)

but if you notice the printenv command doesn't get the value correctly! also it does not reproduce the same behavior on another board the only difference is the U-boot prompt this board has "=>" the other one with "u-boot=>"

Rahix commented 6 months ago

Hm, that's not looking right... :(

the only difference is the U-boot prompt this board has "=>" the other one with "u-boot=>"

If your U-Boot prompt is u-boot=>, then we do not have any problem at all - this prompt won't interfere with crc32, so no warning and no workaround is needed. The problem only shows up when using => as the prompt.

but if you notice the printenv command doesn't get the value correctly!

This looks very wrong... I assume this didn't happen before the fix? Maybe, just to be sure, you can run once again, but from tbot commit 910eeb7a323c45da01c5b7309340d6a6f943c087? That version should fail at crc32 again but I'd hope the printenv still works there...

In any case, it seems

https://github.com/Rahix/tbot/blob/910eeb7a323c45da01c5b7309340d6a6f943c087/tbot/machine/board/uboot.py#L351-L351

does not get the output it expects and thus cuts it apart in the wrong location... There seems to be an additional leading character, which pushes the slicing to the left such that it includes the = in the beginning. Then, there must be an unexpected trailing character which causes the inclusion of a \r at the end of the resulting string. I'll try to reproduce...

Rahix commented 6 months ago

So while trying to reproduce your behavior (so far I wasn't able to), I did find another inconsistency in this patch. I've fixed it now, maybe retry with the latest version of this branch to see whether this has an effect on your problem?

Adnan-Elhammoudi commented 6 months ago

This looks very wrong... I assume this didn't happen before the fix? Maybe, just to be sure, you can run once again, but from tbot commit 910eeb7? That version should fail at crc32 again but I'd hope the printenv still works there...

it reproduces the same behavior with this commit [910eeb7(https://github.com/Rahix/tbot/commit/910eeb7a323c45da01c5b7309340d6a6f943c087)

I assume this didn't happen before the fix?

I was using this snippet as a workaround:

        pattern = re.compile(r'=(\w+)')
        fileaddr =  pattern.findall(ub.env("fileaddr"))
        filesize =  pattern.findall(ub.env("filesize"))

        with ub.ch.with_prompt('\n=> '):
            output = ub.exec0("crc32", f"{fileaddr[0]}" ,f"{filesize[0]}")
        assert expected_crc in output

maybe retry with the latest version of this branch to see whether this has an effect on your problem?

no effect with this branch either tbot 0.10.7.dev13+gfe78bfa!

Rahix commented 6 months ago

Huh, okay... This is certainly not the intended behavior. Can you open an issue about the broken ub.env() please? And while you're at it, please rerun the testcase with an additional -v to log channel i/o. I'll take a look then.

Adnan-Elhammoudi commented 6 months ago

opened a new issue ticket https://github.com/Rahix/tbot/issues/113#issue-2154490005