Skip to content

driver/bareboxdriver.py: Add parameter 'boot_command' #1071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

driver/bareboxdriver.py: Add parameter 'boot_command' #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba03b9c
driver/bareboxdriver.py: Add parameter 'boot_command'
meuren Jan 18, 2023
6f3e055
tests/test_bareboxdriver.py: Add barebox boot test
meuren Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/configuration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1595,6 +1595,7 @@ Arguments:
- bootstring (regex, default="Linux version \\d"): regex that indicating that the Linux Kernel is
booting
- password (str): optional, password to use for access to the shell
- boot_command (str, default="boot -v"): optional, boot command to boot target
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

- login_timeout (int, default=60): timeout for access to the shell

ExternalConsoleDriver
Expand Down
6 changes: 4 additions & 2 deletions labgrid/driver/bareboxdriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class BareboxDriver(CommandMixin, Driver, CommandProtocol, LinuxBootProtocol):
interrupt (str): optional, string to interrupt autoboot (use "\x03" for CTRL-C)
bootstring (regex): optional, regex indicating that the Linux Kernel is booting
password (str): optional, password to use for access to the shell
boot_command (str): optional, boot command to boot target
login_timeout (int): optional, timeout for access to the shell
"""
bindings = {"console": ConsoleProtocol, }
Expand All @@ -37,6 +38,7 @@ class BareboxDriver(CommandMixin, Driver, CommandProtocol, LinuxBootProtocol):
interrupt = attr.ib(default="\n", validator=attr.validators.instance_of(str))
bootstring = attr.ib(default=r"Linux version \d", validator=attr.validators.instance_of(str))
password = attr.ib(default="", validator=attr.validators.instance_of(str))
boot_command = attr.ib(default="boot -v", validator=attr.validators.instance_of(str))
login_timeout = attr.ib(default=60, validator=attr.validators.instance_of(int))

def __attrs_post_init__(self):
Expand Down Expand Up @@ -198,12 +200,12 @@ def await_boot(self):
self.console.expect(self.bootstring)

@Driver.check_active
def boot(self, name: str):
def boot(self, name: str = ""):
"""Boot the default or a specific boot entry

Args:
name (str): name of the entry to boot"""
if name:
self.console.sendline(f"boot -v {name}")
else:
self.console.sendline("boot -v")
self.console.sendline(self.boot_command)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@meuren meuren Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@Bastian-Krause Bastian-Krause Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

9 changes: 9 additions & 0 deletions tests/test_bareboxdriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,12 @@ def test_barebox_run_error(self, target_with_fakeconsole, mocker):
res = d.run_check("test")
res = d.run("test")
assert res == (['error'], [], 1)

def test_barebox_boot(self, target_with_fakeconsole):
t = target_with_fakeconsole
d = BareboxDriver(t, "barebox", boot_command='boot -v foo')
d = t.get_driver(BareboxDriver)
d.boot()
assert d.console.txq.pop() == b"boot -v foo\n"
d.boot(name='bar')
assert d.console.txq.pop() == b"boot -v bar\n"