driver/bareboxdriver.py: Add parameter 'boot_command'

[meuren]

To adapt the BareboxDriver and be consistent with the UBootDriver, the parameter 'boot_command' is added. Now you can set the boot source and do something like this:

targets:
main:
drivers:
BareboxDriver:
boot_command: "boot net"

Signed-off-by: Fabian Meuren [email protected]

[codecov]

Codecov Report

Base: 63.2% // Head: 63.2% // No change to project coverage 👍

Coverage data is based on head (dd43a97) compared to base (dd43a97).
Patch has no changes to coverable lines.

❗ Current head dd43a97 differs from pull request most recent head 6f3e055. Consider uploading reports for the commit 6f3e055 to get more accurate results

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1071   +/-   ##
======================================
  Coverage    63.2%   63.2%           
======================================
  Files         152     152           
  Lines       11330   11330           
======================================
  Hits         7169    7169           
  Misses       4161    4161           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Bastian-Krause]

Looks good, but boot_command should be documented in doc/configuration.rst.

[Bastian-Krause]

Looks good.

[Bastian-Krause]

After looking at this again: I think this can be quite confusing. self.boot_command is only used if name is set to an empty string. I don't think that's right. At the very least, this should be documented.

[meuren]

For me, it's like: With boot_command you can overwrite the global default 'boot -v' and thus set a 'new' global default. This in turn can be overwritten with name if necessary. def boot(self, name: str = ""):
Do you have a suggestion on how it might be more intuitive?

[Bastian-Krause]

Letting name default to an empty string looks good to me and should be adopted also by the UBootDriver.

Is setting boot_command and calling barebox.boot("mmc0") a valid use case? If it isn't, we could raise an exception in that case. This also applies to the UBootDriver.

In general: I don't really understand why you'd want to set a custom boot command and use the boot method, neither in barebox nor in U-Boot (and I know that boot_command is already implemented in the UBootDriver, but still). If you don't use the standard boot command, what keeps you from simply calling barebox.console.sendline("bootm ..") in your strategy/script? What's the benefit here?

[jremmet]

Think we have 2 usecases:

1. Set a default boot source in the config using it the complete test. This works for UbootDriver via boot_command, but not with the BareboxDriver. The BareboxStrategy calls the BareboxDriver,boot("") so you need to configure the barebox itself. It would be nice to do this in the config with the supposed change.
2. With GraphStrategy we can define different boot scenarios and go with "via" to a shell booted from the selected source. For this a direct choose through BareboxDriver,boot(SOURCE) would be helpful. This works for BareboxDriver, but not for UbootDriver. (U-Boot boot commands Support #1082)

Is there a prefer Way to select the boot_command? Should we get rid of the Driver.boot functions and set the drivers boot source only via strategy?

The difference between the barebox "boot -v NAME" and uboot "COMMAND" was choosen because we consider the barebox interface to be more stable. If needed we could remove the "run" from the uboot boot_command and also select a target (environment script).

[Bastian-Krause]

Think we have 2 usecases:

1. Set a default boot source in the config using it the complete test. This works for UbootDriver via boot_command, but not with the BareboxDriver. The BareboxStrategy calls the BareboxDriver,boot("") so you need to configure the barebox itself. It would be nice to do this in the config with the supposed change.
2. With GraphStrategy we can define different boot scenarios and go with "via" to a shell booted from the selected source. For this a direct choose through BareboxDriver,boot(SOURCE) would be helpful. This works for BareboxDriver, but not for UbootDriver. (Wip/uboot boot #1082)

This does not really answer my question, does it? I see the boot methods as a convenience feature to boot the regular way (i.e. boot [<name>]) . If you need anything special, you could just do it yourself (see my suggestion). I see no downside to this.

Is there a prefer Way to select the boot_command? Should we get rid of the Driver.boot functions and set the drivers boot source only via strategy?

I think for the regular case, they are convenient, see above. Everything else could be handled in a strategy.

The difference between the barebox "boot -v NAME" and uboot "COMMAND" was choosen because we consider the barebox interface to be more stable. If needed we could remove the "run" from the uboot boot_command and also select a target (environment script).

I don't really understand this.

Don't get me wrong here, I'm not strictly against the PR, especially since we have the same in the UBootDriver, but I'd like to prevent implementing special cases and overcomplicating the code if the alternative has no obvious downside and does not involve patching the driver.

I guess @Emantor and @jluebbe should weigh in because they reviewed #621.

[jremmet]

Think we have 2 usecases:

1. Set a default boot source in the config using it the complete test. This works for UbootDriver via boot_command, but not with the BareboxDriver. The BareboxStrategy calls the BareboxDriver,boot("") so you need to configure the barebox itself. It would be nice to do this in the config with the supposed change.
2. With GraphStrategy we can define different boot scenarios and go with "via" to a shell booted from the selected source. For this a direct choose through BareboxDriver,boot(SOURCE) would be helpful. This works for BareboxDriver, but not for UbootDriver. (Wip/uboot boot #1082)

This does not really answer my question, does it? I see the boot methods as a convenience feature to boot the regular way (i.e. boot [<name>]) . If you need anything special, you could just do it yourself (see my suggestion). I see no downside to this.

We currently work with a static strategy for most tests, like

labgrid/labgrid/strategy/bareboxstrategy.py

Line 56 in e05f530

self.barebox.boot("")

The idea was to handle the boot source via the driver. def boot seams to be place to do this.
Adding it as a strategy parameter would also work. I would be nice to have the same API for barebox and u-boot driver.

Is there a prefer Way to select the boot_command? Should we get rid of the Driver.boot functions and set the drivers boot source only via strategy?

I think for the regular case, they are convenient, see above. Everything else could be handled in a strategy.

The difference between the barebox "boot -v NAME" and uboot "COMMAND" was choosen because we consider the barebox interface to be more stable. If needed we could remove the "run" from the uboot boot_command and also select a target (environment script).

I don't really understand this.

This was mostly about #1082

Don't get me wrong here, I'm not strictly against the PR, especially since we have the same in the UBootDriver, but I'd like to prevent implementing special cases and overcomplicating the code if the alternative has no obvious downside and does not involve patching the driver.

I guess @Emantor and @jluebbe should weigh in because they reviewed #621.

[meuren]

Sorry, I am not yet familiar with your processes.

[Emantor]

Is this still relevant since we now have proper boot command support for the UBootDriver?

[jremmet]

Is this still relevant since we now have proper boot command support for the UBootDriver?

Yes this is the equivalent default boot command for the BareboxDriver

[jremmet]

gently ping