Skip to content
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

POC for a new harness designed for power measurement purposes. #85130

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

Conversation

gbarkadiusz
Copy link
Collaborator

@gbarkadiusz gbarkadiusz commented Feb 4, 2025

POC of Power twister harness.

This pr adds support for power measurements for twister and three example 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.

Setup
image
image

To reproduce.:

  • Configure the stm32l562e_dk board according to the instructions provided above.
  • Connect both USB ports of the board to your host system. The board will appear as two separate targets:
    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
  • Run twister commend:
twister --device-testing --device-serial /dev/ttyACM1 -p stm32l562e_dk -T tests/subsys/pm/power_residency_time/ --west-flash="--dev-id=004800483137510D33333639" --west-runner=pyocd -vv  -ll DEBUG -xCONFIG_BOOT_DELAY=500 --pm-probe=stm_powershield --pm-probe-port=/dev/serial/by-id/usb-S
TMicroelectronics_PowerShield__Virtual_ComPort_in_FS_Mode__FFFFFFFEFFFF-if00

@gbarkadiusz gbarkadiusz added the DNM This PR should not be merged (Do Not Merge) label Feb 4, 2025
@gbarkadiusz gbarkadiusz force-pushed the topic/power_management_harness branch 2 times, most recently from 679de9e to c474ca5 Compare February 4, 2025 14:12
Comment on lines 13 to 14
pytest_args: [--probe=stm_powershield,
--probe-port=<path_to>/STMicroelectronics_PowerShield__Virtual_ComPort]
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two args (--probe and --probe-port ) should be provided via the command line, as they vary, as opposed to the pytest_dut_scope: session and the properties in test_expected_values.yaml file which are "portable"/"fixed" regardless of the probe and port used :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to move

-  name: "wakeup_timer"
     measurement_time: 2
     num_of_transitions: 1
     rms: [0.26, 140]

to

harness_config:
  measurements:
    - name: "wakeup_timer"
      measurement_time: 2
      num_of_transitions: 1
      rms: [0.26, 140]

instead of the separate file? not sure what having the commands in a separate file acheives :)

@gbarkadiusz gbarkadiusz force-pushed the topic/power_management_harness branch 4 times, most recently from 4109da0 to 1424b32 Compare February 17, 2025 15:21
@gbarkadiusz
Copy link
Collaborator Author

@bjarki-andreasen Please take another look. I'm not sure that moving --probe and --probe-port to the Twister parameter is a good approach. They're more closely related to the harness's parameters rather than Twister's, so I suggest leaving them in the harness_config section.

@bjarki-andreasen
Copy link
Collaborator

bjarki-andreasen commented Feb 17, 2025

@bjarki-andreasen Please take another look. I'm not sure that moving --probe and --probe-port to the Twister parameter is a good approach. They're more closely related to the harness's parameters rather than Twister's, so I suggest leaving them in the harness_config section.

Would you argue the same for the --serial option and its parameters? The probe and probe-port are instances of actual hardware right? If i ran the same test locally, would I not need to change the port to match my setup?

@gbarkadiusz gbarkadiusz force-pushed the topic/power_management_harness branch 2 times, most recently from 380d8c0 to 91fa4eb Compare February 18, 2025 12:24
@gbarkadiusz
Copy link
Collaborator Author

@bjarki-andreasen Please take another look. I'm not sure that moving --probe and --probe-port to the Twister parameter is a good approach. They're more closely related to the harness's parameters rather than Twister's, so I suggest leaving them in the harness_config section.

Would you argue the same for the --serial option and its parameters? The probe and probe-port are instances of actual hardware right? If i ran the same test locally, would I not need to change the port to match my setup?

Yes, you need to change the port to match your setup.
Regarding the twister parameters. You right, so I've applied your suggestions, There's the example twister's command with these two parameters --pm-probe and --pm-probe-port.

twister --device-testing --device-serial /dev/ttyACM1 -p stm32l562e_dk -T tests/subsys/pm/power_residency_time/ --west-flash="--dev-id=004800483137510D33333639" --west-runner=pyocd -vv  -ll DEBUG -xCONFIG_BOOT_DELAY=500 --pm-probe=stm_powershield --pm-probe-port=/dev/serial/by-id/usb-S
TMicroelectronics_PowerShield__Virtual_ComPort_in_FS_Mode__FFFFFFFEFFFF-if00

@bjarki-andreasen , @nashif Please take a look.

@gbarkadiusz gbarkadiusz force-pushed the topic/power_management_harness branch 7 times, most recently from f202877 to c0e83bd Compare February 18, 2025 13:47
@gbarkadiusz gbarkadiusz marked this pull request as ready for review February 18, 2025 13:51
@gbarkadiusz gbarkadiusz removed the DNM This PR should not be merged (Do Not Merge) label Feb 18, 2025
@PerMac
Copy link
Member

PerMac commented Feb 18, 2025

cc @nordic-piks

raise ValueError("Invalid unit. Use 'us', 'ms', or 's'.")

@staticmethod
def current_RMS(data, exclude_first_600=True, num_peaks=1):
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, number of samples which needs to be excluded from analysis should be taken also from sample.yml,
so that this can be easily configures for given test.
Of course this is just a hint if you want to make this approach more general.

data = [float(x) for x in data]

# Find the peaks in the data using the `find_peaks` function
peaks = signal.find_peaks(data, distance=40, height=0.008)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, minimal number of samples between peaks and its "hight" should be also taken from sample.yaml
Of course this is just a hint if you want to make this approach more general.

- name: "power states"
measurement_time: 6
num_of_transitions: 4
rms: [56.0, 4.0, 1.3, 0.26, 140]
Copy link
Collaborator

@nordic-piks nordic-piks Feb 19, 2025

Choose a reason for hiding this comment

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

IMHO, it would be good to test/verify also the length of each period between peaks.
This will allow to verify if actual sleep time meets expectation (looking from current consumption measurements).

Also, you assume peaks of high current and measures current in between.
There is also beneficial to measure current during active time i.e. being able to specify requirements for that in test.

Of course this is just a hint if you want to make this approach more general :)

self.target_voltage = None
self.target_temperature = None
self.acqTimeoutThread = None
self.power_shield_conf = PowerShieldConf()
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, parameters for PowerShieldConf e.g. voltage, sampling frequency should be taken from some board configuration file (here they just base on defaults). I do not see mechanisms to provide it externally.

Possibly, board parameters should be overwritable by some specific configuration in the test (sample.yaml)
Of course this is just a hint if you want to make this approach more general :)

- name: "power states"
measurement_time: 6
num_of_transitions: 4
rms: [56.0, 4.0, 1.3, 0.26, 140]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Beside RMS, it would be also interesting to verify min and/or max value of current during some period.
This will allow to detect unexpected events (peaks) when you expect to have stable sleep current.

Of course this is just a hint if you want to make this approach more general.

@gbarkadiusz
Copy link
Collaborator Author

@nordic-piks Thank you for your feedback.
This can be improved in the next iteration.
This PR addresses the requirements outlined in PR #80692.
I would like to merge this initial approach now and improve it in subsequent iterations.
@bjarki-andreasen FYI.

self.target_temperature = self.get_temperature(self.power_shield_conf.temperature_unit)
self.target_voltage = self.get_voltage_level()

def connect(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Either declare all public methods in ABC base or make methods not referenced in ABC api private.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to limit the function in ABC to init measure and get_data.

Copy link
Contributor

Choose a reason for hiding this comment

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

In such case methods that do not override abstract methods should be private.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

except Exception as e:
logging.error(f"Failed to reopen connection after reset: {e}")

def get_voltage_level(self) -> float:
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify the unit of the return value (V, mV etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

return None
else:
logging.warning("No response for voltage command.")
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

Please return float('nan') in case of 'no response', None will crash any further calculations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Sends the temperature command and returns the temperature as a float.

:param unit: The unit to request the temperature in, either 'degc' or 'degf'.
:return: The temperature value as a float.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, please specify unit (C, K, F)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

else:
logging.warning(f"Failed to set data format to {data_format}.")

def set_frequency(self, frequency: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Change type of this 'frequency' argument to either float or 'enum' type.
If this is not direct frequency in Hz then change the argument name to something like 'frequency_option'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

else:
logging.warning(f"Failed to set sampling frequency to {frequency}.")

def set_acquire_time(self, acquire_time: str = '0'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: it should be: set_acquisition_time, acquire is a verb

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you, fixed.

else:
logging.warning(f"Failed to set acquisition time to {acquire_time}.")

def set_voltage(self, voltage: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same nitpick as for 'set_frequency'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

self.function_mode = function_mode
self.target_voltage = target_voltage
self.temperature_unit = temperature_unit
self.acquisition_time = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Please annotate as 'Optional[float]'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As below.


class PowerShieldData:
def __init__(self):
self.data = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Please annotate 'data'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As below.

self.port = port
self.baudrate = baudrate
self.timeout = timeout
self.serial_connection = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Please annotate 'serial_connection'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What you mean?

"Connection to %s at %d baud opened successfully.", self.port, self.baudrate
)
except serial.SerialException as e:
logging.error("Error opening serial port %s: %s", self.port, str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why catch this error and not raise anything else. Test will continue with failed serial connection and will fail later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add simply raise should be enough?
Fixed.


:return: The processed received data as a string.
"""
s = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make meaningful variable name instead of 's'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

"""Closes the connection if the object is deleted."""
self.close()
# Shut down logging
logging.shutdown()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't call shutdown it will bring down entire logging system

Copy link
Collaborator Author

@gbarkadiusz gbarkadiusz Feb 19, 2025

Choose a reason for hiding this comment

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

Fixed.



class UnityFunctions:
@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

If all methods are static class in not needed, they could be declared as functions in UnityFunctions.py module.

Copy link
Collaborator Author

@gbarkadiusz gbarkadiusz Feb 19, 2025

Choose a reason for hiding this comment

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

Fixed.

@gbarkadiusz gbarkadiusz force-pushed the topic/power_management_harness branch 4 times, most recently from 50c4d38 to a46de65 Compare February 19, 2025 11:54
Add support for powerShield of stm32l562e_dk board.

Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>
Add three tests.
 - power_states
 - power_wakeup_timer
 - power_residency_time

Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>
@gbarkadiusz gbarkadiusz force-pushed the topic/power_management_harness branch from a46de65 to 3ecc5a6 Compare February 19, 2025 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants