-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
tests: pm: Adding tests based on new approach to PM testing. #80692
base: main
Are you sure you want to change the base?
tests: pm: Adding tests based on new approach to PM testing. #80692
Conversation
bb6e11e
to
cc760a2
Compare
data = current_measurement_output | ||
# Empirical RMS values in mA with descriptive keys | ||
rms_expected = { | ||
"k_cpu_idle":57.0, # K_cpu_idle |
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.
can we move those expected data to a yaml file for different platform?
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.
Of course we can, the good hint. Thank you.
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.
PS: Btw, Please take a look. #80989. Feel free to share ideas or warries. :)
while (1) { | ||
printk("\nGoing to k_cpu_idle.\n"); | ||
k_msleep(400); | ||
printk("\nWake Up.\n"); |
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.
can we use this tests\subsys\pm\power_mgmt_soc\src\power_mgmt.c, instead of the sleep time?
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 don't know yet, let me see.
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.
Yep, get the information from the device tree is the way to go imho, otherwise it will not be portable and the previous comment is useless.
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.
@ceolin I added the use of DTS, is this what you meant?
@@ -0,0 +1,30 @@ | |||
# Copyright (c) 2024 Intel Corporation. |
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.
would this be better to move this function to scripts/pm/pm_helper.py, and rms is not the only implement, e.g. we can have average mean
f959bb2
to
980a943
Compare
1f00e48
to
69e3a4f
Compare
2525c2f
to
dc18404
Compare
@hakehuang I've moved the hardcoded expected value to the appropriate yaml file. Please take a look. |
dc18404
to
cbc99c3
Compare
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.
looks good
pytest_args: [--powershield=<path_to>/STMicroelectronics_PowerShield__Virtual_ComPort, | ||
--expected-values=<path_to>/stm32l562e_dk.yaml] |
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.
IIUC these need to be set manually before testing, right ?
Any way we can spare the need for manual update ?
At least, <path_to>/stm32l562e_dk.yaml
is rather static, so it looks reachable.
For <path_to>/STMicroelectronics_PowerShield__Virtual_ComPort
maybe a dedicated twister configuration could be used or added ?
I believe the approach in this PR is too highly coupled to pm power states, a specific power monitor, and a specific test case. Measuring current/power or anything else is "generic", its measured in SI units, and can be performed by an "endless" variation of probes. I compare measuring power with "any probe" to flashing a board with "any runner", be it jlink, openocd, pyocd, or whatever. I believe it can and should be as simple to define a testcase requiring power measurements as this
The measurements could be anything, a mode, some peripheral being on, like a comparator in low power mode:
and that specific probes, like runners, should be defined like This is important, as the test case should not depend on any specific probe, if one developer has a This PR keeps a bunch of this local, using pytest, but I don't think that is a nice, scalable approach to power measurements. |
agree this could be abstracted to support future probes and other measurements. We need to start somewhere though.
Ok, probably @gbarkadiusz can you look into this? AFAIK that should be possible with the current implementation while allowing for future addition of probes. |
@nashif @bjarki-andreasen Regarding the proposal for a separate harness for power measurement, IIUC, Is it right to create a new harness that duplicates the functionality of pytest (which would lead to code duplication)? In fact, we can use the existing pytest harness to achieve our goal. We can simply extend its configuration to include additional details about the probe and port being used. |
pytest is a generic framework, any harness could theoretically be implemented with pytest I believe, a custom harness would be purpose built, its less a question of duplication than it is simplicity of use :) |
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.
It is looking good, I think that is a good start, even being target specific.
We need to start somewhere :) Overall comments but nothing major.
Thanks a lot for this effort.
|
||
Returns: | ||
List[float]: An array of measured current values in amperes. | ||
""" |
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.
Wondering if that isn't generic enough to be the interface for this sort of test and not specific for stm32l56e_dk
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 allows us to easily extend the system to support other platforms without needing to rewrite the entire test logic. The goal is to keep the test interface flexible and adaptable to different devices.
self.target_voltage = round(voltage, 3) | ||
return self.target_voltage | ||
except ValueError: | ||
logging.error("Error: Could not convert temperature value.") |
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.
temperature ?
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.
Writing the library for PowerShield.py I implemented this functionality only for testing. If not necessary, I'll remove it.
logging.warning("No response for voltage command.") | ||
return None | ||
|
||
def get_temperature(self, unit: str = PowerShieldConf.TemperatureUnit.CELCIUS) -> float: |
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.
Is this being used anywhere ?
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.
As above, only for testing, showing that the firmware of PowerShield allows to read the value of temperature.
if len(x) < 1 or x == 0xF0: | ||
self.acqComplete = True | ||
return s.replace("\0", "").strip().replace("\r", "").replace("\n\n\n", "\n") | ||
s += str(x, encoding='ascii', errors='ignore') |
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.
is it missing an explicit return ?
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 loop should continue as long as data exists from the serial connection.
""" | ||
Handle metadata end of acquisition message. | ||
""" | ||
# Read the next 4 bytes |
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.
4 or 2 bytes ?
while True: | ||
if self.dataQueue.empty() and bool(self.acqComplete): | ||
outputFile.close() | ||
return |
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.
can't you just break
it here ?
self.start_measurement() | ||
|
||
# calculate the data | ||
# self.get_measured_data(measure_unit) |
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.
Can't it be removed ?
|
||
static const struct device *rtc = DEVICE_DT_GET(DT_NODELABEL(rtc)); | ||
|
||
volatile k_tid_t my_tid; |
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.
why using volatile here ? shouldn't the scope be local too ?
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.
That's right it has not to be a volatile variable. I used that attribute to be sure that this variable is not going to be optimized by compiler.
struct k_thread my_thread_data; | ||
K_THREAD_STACK_DEFINE(my_stack_area, MY_STACK_SIZE); | ||
|
||
void my_entry_point(void *, void *, void *) |
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.
static void my_entry_point(void *arg1, void *arg2, void *arg3)
|
||
while (1) { | ||
arch_nop(); | ||
} |
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.
why do you have these loops ? you don't want the app to idle ?
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've noticed that the STM32L562E-DK board enters sleep mode without this loop, which causes issues with reconnecting and executing the next test.
Twister harness log.
DEBUG:twister_harness.device.factory: Get device type "hardware"
DEBUG:twister_harness.device.hardware_adapter: Opening serial connection for /dev/ttyACM1
DEBUG:twister_harness.device.hardware_adapter: Flashing command: west flash --skip-rebuild --build-dir <path_to>/twister-out/stm32l562e_dk_stm32l562xx/tests/subsys/pm/power_wakeup_timer/pm.power_wakeup_timer.pm_device --runner pyocd -- --dev-id=004800483137510D33333639
ERROR:twister_harness.device.hardware_adapter: Could not flash device None
DEBUG:twister_harness.device.hardware_adapter: Closed serial connection for /dev/ttyACM1
So, the quick fix was added this infinite loop.
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.
PS: Pyocd throws:
0003817 E Error while initing target: STLink error (9): Get IDCODE error [commander]
And then it has to be erased manually by:
pyocd erase --mass -t 'stm32l562qeixq' -O reset_type=hw -O connect_mode='under-reset'
cbc99c3
to
cb99e55
Compare
@ceolin Thank you for feedback, I applied your suggestions, please take a look again. |
see #85033 |
Introduces scripts for powerShiled on stm32l562e_dk board. Adds three new power management pytests based on a new approach. Signed-off-by: Arkadiusz Cholewinski <[email protected]>
Thank you @nashif for the suggestions. I've implemented the Power Harness, and here is the link #85130 to the POC Draft PR. Please have a look and let me know if this is what you had in mind for the additional harness implementation |
Introduces two scripts
abstract_power_monitor.py
andpwsh_stm32l562.py
.Adds three new power management pytests based on a new approach.
SETUP
TESTS
pm.states: Verifies the transition to all available power states on
stm32l562e_dk
.During the test, pytest detects each step and calculates the RMS (Root Mean Square) current for every detected state. The test then compares the calculated RMS values against the expected values, which have been determined manually through experimental observation
pm.residency_time: Tests the residency time of each power state.
This test evaluates the residency time of different power states on the
stm32l562e_dk
board.pm.wakeup_timer: Tests the RTC alarm’s ability to wake the CPU from the lowest power state.
Each test calculates RMS current values for each detected state and compares them to expected values, which were manually set based on experimental data.
To Reproduce
Power Monitor: e.g.,
/dev/ttyACM0
or/dev/serial/by-id/usb-STMicroelectronics_PowerShield__Virtual_ComPort_in_FS_Mode__FFFFFFFEFFFF-if00
Target: e.g.,
/dev/ttyACM1
or/dev/serial/by-id/usb-STMicroelectronics_STLINK-V3_004##############39-if02
<path_to_power_monitor>
in thetests/subsys/pm/power_states/testcase.yaml
file under the pytest_args section as follows:pytest_args: [--powershield=<path_to_power_monitor>]
twister --device-testing --device-serial <path_to_target> -p stm32l562e_dk -s <test_name> -x=CONFIG_BOOT_DELAY=500