-
Notifications
You must be signed in to change notification settings - Fork 199
bootloader: detect boot #1191
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.
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
base: master
Are you sure you want to change the base?
bootloader: detect boot #1191
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #1191 +/- ##
========================================
+ Coverage 55.8% 55.9% +0.1%
========================================
Files 170 170
Lines 13377 13389 +12
========================================
+ Hits 7469 7496 +27
+ Misses 5908 5893 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@Bastian-Krause We want to do some Watchdog tests and rely on the bootloader driver to detect the reboot. We could do this in the test but we like the idea of having this beside the other bootloader configs like the prompt. So short having the feature back and only disable the check if we force a state would be good for out use cases. Not sure If this acceptable for the project. This PR tries to solve it without changing too much. (Without addressing accidentally running boards) |
I'd probably implement the I'm torn between having @Emantor What do you think? |
We also have trouble if we accidentally connect to a running bootloader. Before deprecation we run into a timeout in this case. (Our power switch has a manual "always on" overwrite...) Would this a acceptable solution?: |
The decision to remove this support was to let drivers be deactivated and activated even if the bootloader is already running, which is necessary to support reusing the current state of the board within a labgrid client state transition. In my opionion testing a watchdog reset belongs to the testcase and using labgrid functionality for this is not the correct approach. What you want to do for watchdog testing anway is to set a global variable inside the bootloader, deactivate the driver, wait for the watchdog reset and reactivate the driver. Than the test can check whether the variable is not there any longer and this is a new running instance of the bootloader and even check the reset reason if this is possible with the board. I don't have a solution for the case where you have a manual override on the power switch. Can this be queried by software? I think this is more of an organizational problem than something that can be solved with a technical solution inside of labgrid. |
Yes I agree. Would adding "boot_expression" to the driver again and adding a check for it to labgrid/labgrid/driver/bareboxdriver.py Lines 147 to 187 in ed4e12e
We can set the "boot_detected" flag there and use it in strategies and tests. |
04393a2
to
efb5f35
Compare
@Emantor I added the flag to driver as discussed. |
labgrid/driver/bareboxdriver.py
Outdated
bootstring = attr.ib(default=r"Linux version \d", validator=attr.validators.instance_of(str)) | ||
password = attr.ib(default="", validator=attr.validators.instance_of(str)) | ||
login_timeout = attr.ib(default=60, validator=attr.validators.instance_of(int)) | ||
|
||
def __attrs_post_init__(self): | ||
super().__attrs_post_init__() | ||
self._status = 0 | ||
self.boot_detected = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use an attr.ib
for this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sure here because it makes this also an Argument. Added it and a note to configuration.rst
labgrid/driver/ubootdriver.py
Outdated
if self.boot_expression: | ||
import warnings | ||
warnings.warn("boot_expression is deprecated and will be ignored", DeprecationWarning) | ||
self.boot_detected = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use an attr.ib
for this as well.
16bbdcd
to
23c8e98
Compare
23c8e98
to
226b3eb
Compare
@Emantor gently ping |
226b3eb
to
670e0ca
Compare
labgrid/driver/bareboxdriver.py
Outdated
bootstring = attr.ib(default=r"Linux version \d", validator=attr.validators.instance_of(str)) | ||
password = attr.ib(default="", validator=attr.validators.instance_of(str)) | ||
login_timeout = attr.ib(default=60, validator=attr.validators.instance_of(int)) | ||
boot_detected = attr.ib(default=False, validator=attr.validators.instance_of(int)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be instance_of(bool)
. We also probably want to set this to on_setattr=None
, so users can't change this attribute.
There seems to be no way to protect this from external modification vs internal modification with the on_setattr()
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the type.
With attr I also found no way to protect it. Not sure if it worth _boot_detected
and get_boot_detected
like done with _status
Sets self.boot_detected in _await_prompt. This is used in reset to check if a reboot has occurred. Can also be used in strategies. Signed-off-by: Jan Remmet <[email protected]>
Sets self.boot_detected in _await_prompt. This is used in reset to check if a reboot has occurred. Can also be used in strategies. Direct checking was deprecated with: 7b55a19 ("driver/ubootdriver: deprecate boot_expression attribute") Signed-off-by: Jan Remmet <[email protected]>
670e0ca
to
4b9a7a2
Compare
The boot_expression feature for u-boot was deprecated to sync barebox with features and to allow to connect to a running bootloader.
Readd the feature for both bootloaders and use the check only to set the
boot_detected
flag.So after getting a prompt we can check if the bootloader runs trough a reboot.
Show this in the 'reset' method.
Checklist