-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
tests: Improve test hardware configuration by putting board-specific settings in target_wiring
module
#17954
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?
Conversation
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
@projectgus as we discussed, this isn't 100% complete but should be good enough for a first review. @hmaerki this should solve a lot of our issues with Octoprobe and configuring the wiring for each board. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17954 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 171 171
Lines 22296 22296
=======================================
Hits 21937 21937
Misses 359 359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Damien George <damien@micropython.org>
This is, eg, `rpi_pico`. Signed-off-by: Damien George <damien@micropython.org>
685c657
to
29854e3
Compare
Signed-off-by: Damien George <damien@micropython.org>
Actually, maybe the other way around is better: if there's a The alternative would be new CLI options to |
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 approach looks very good and simple to me, cuts out a lot of complex code!
It also lowers the bar for adding more tests, I've hesitated to add small hardware tests in the past because it seems "expensive" to copy the big block of hardware config and pay the cost of maintaining it.
There is a bit of new user documentation we should probably repeat even though it's redundant, for example in each file where we have # For UART loopback tests
we should be explicit about # For UART loopback tests (connect these pins together)
.
Actually, maybe the other way around is better: if there's a target_wiring.py on the filesystem on the device then that should take precedence? That way, you can set up your own boards for testing with custom wiring and run the tests as normal (without any special arguments) and they just work.
I think that's a good approach (test runner defaults to a module on the target filesystem if it's there). Two additional suggestions:
run-tests.py
should log a line i.e.Using host file PATH as target_wiring
orUsing target_wiring module from target
so you never have to guess which file is being used.- Add a command line option for
run-test.py --target-wiring PATH
so can also override the selection for custom boards or experiments without needing to copy anything to the target.
Thanks for looking at this!
Yes, definitely will make it much easier to add new hardware-based tests.
OK, I'll implement that.
It actually already does log this info (which file is used from the host), it's in the first line that also includes other target info (eg arch, threading, unicode, float). Do you think it should be a separate line? (Also, I think I should make it so that info is only printed if it's going to run one of the tests that requires |
Signed-off-by: Damien George <damien@micropython.org>
Added a commit that:
Example output when host file is used:
and when there's already a file on the target filesystem:
|
Signed-off-by: Damien George <damien@micropython.org>
Oh sorry, I didn't see that was already in there when I reviewed the code. I think that's also fine, if you prefer to do it that way. It might actually be easier to read out quickly that way. |
Fully agree! |
Signed-off-by: Damien George <damien@micropython.org>
Added a commit that puts the |
Summary
There are currently a few "hardware" tests that need external board connections to be made for them to work, eg bridging a pair of pins. Eg all tests in
tests/extmod_hardware
need some kind of connection.Along with the physical connections -- which are different for each board -- there needs to be corresponding Python settings, eg which UART instance to use and which pins for TX/RX.
These settings are currently hard-coded in each test file. That has a few problems:
tests/extmod_hardware/machine_pwm.py
)This PR aims to solve all the above problems by splitting the board-specific settings out into separate files, one for each board (or port). They are placed in the
tests/target_wiring/
directory. Therun-tests.py
test runner then loads the appropriate configuration for the target that is being tested, sends it to the board so it's available asimport target_wiring
(without needing a filesystem), and then executes the test (only tests that needtarget_wiring
have it loaded).This PR adds support for this
target_wiring
module, and converts all the existingmachine.UART
tests to use it.Testing
Tested locally on PYBV10 and ESP32-S2. Still needs further testing once we decide this is the way forward.
Trade-offs and Alternatives
Although the idea is straightforward, there were many decisions to make in the implementation:
target_wiring
module follows a hierarchy: if an exact match for the board name is found (viasys.implementation._build
) then that's used, otherwise there's a possible wildcard match on the board name (eg for PYB* boards, they can all use the same config), otherwise there's a match on the port name (eg rp2 boards can all use the same config). This allows a lot of flexibility (maybe too much?).target_wiring
module to the board without using the filesystem or VFS hook. It just evaluates a new class calledtarget_wiring
with the body of the class the script with the settings. This should be pretty minimal and work on targets without a filesystem/VFS.extmod/machine_uart*.py
tests ... they are not strictly hardware tests (they don't require external connections) but they do need to know which UART instance to use, and they can reuse the same instance used byextmod_hardware/machine_uart*.py
tests. Hopefully this won't be an issue, but it does confuse somewhat which tests requiretarget_wiring
configuration and which don't (ie it's no longer as simple as saying that onlyextmod_hardware/
tests needtarget_wiring
).uart_loopback_args
anduart_loopback_kwargs
. This shows the general problem that the approach may not scale well if lots of tests need subtly different configurations, egI2CTarget
will need a loopback configuration (4 pins) and a non-loopback configuration (another 2 pins). To make it work, the config for these either need to be named differently, or selected very carefully so the pins can overlap.extmod_hardware/machine_pwm.py
can reuse the UART loopback pins, but probably they should be named differently, in case they can't be reused.run-tests.py
for zephyr, or exposesys.implementation._build
on that port).target_wiring
module (without usingrun-tests.py
) requires the user to explicitly copy across the correcttarget_wiring.py
file to their device.target_wiring.py
on the device then it is overridden by the one injected byrun-tests.py
. I think this is desired behaviour, so you don't have a stale/old file on there that messes up the test.run-tests.py
for the user to select/override whichtarget_wiring.py
module to use, eg for custom boards, eg for Octoprobe.