tests: periph_pm: add peripheral test. by basilfx · Pull Request #8848 · RIOT-OS/RIOT

basilfx · 2018-03-28T21:31:12Z

Contribution description

This PR adds a test application for periph_pm. Although you need a way to measure power consumption to really use this test, I think its a nice way to play with (or debug) the power modes and observe what the CPU will do.

I had this one lying around, somewhere. Initially I used this (before periph_pm) to test the power modes on the EFM32 development kits, but rearranged it for periph_pm.

Issues/PRs references

None

jnohlgard · 2018-03-28T21:51:58Z

Cool! Useful for reference measurements to compare with what the real application uses

aabadie · 2018-03-29T06:07:19Z

Sorry, but it looks like a duplicate of #7666.

basilfx · 2018-03-29T06:16:59Z

@aabadie Oops, yes. I did search for periph_pm before submitting :-)

@vincent-d Maybe we can merge both PR's?

vincent-d · 2018-03-29T17:38:28Z

I have no time to work on this at the moment, so you can go ahead with this one! I'll try to review it.

If it gets merged, we'll close mine.

basilfx · 2018-03-29T22:10:38Z

Merged this with the work by @vincent-d in #7666.

I chose to add an additional unblock_rtc to simplify code/ifdefs. It only works if the CPU provides pm_layered and periph_rtc.

Screenshot below show a gap of three seconds of scheduled unblocking. The other gap is a normal unblock (with UART events inbetween).

vincent-d · 2018-03-30T00:51:44Z

tests/periph_pm/README.md

@@ -0,0 +1,14 @@
+Expected result
+===============
+You should be presented with the RIOT shell, providing you with three commands to test the power


There are a bit more than three commands ;)

Yes :-)

Fixed.

vincent-d

Quickly tested on stm32f4, pm_unblock_rtc works fine.

Except minor comment, it's looks OK.

aabadie

Looks good. I have a few comments and questions.

I'll test it on stm32l0.

aabadie · 2018-03-30T07:41:46Z

tests/periph_pm/Makefile

@@ -0,0 +1,11 @@
+APPLICATION = periph_pm


You don't need to set this variable

aabadie · 2018-03-30T07:42:23Z

tests/periph_pm/README.md

+
+Some power modes may require a certain event to wake up. Reset the CPU if needed.
+
+If a RTC peripheral is available, an additional command to temporary unblock a power mode is


s/temporary/temporarily/

aabadie · 2018-03-30T07:43:46Z

tests/periph_pm/main.c

+#ifdef MODULE_PERIPH_RTC
+#include "periph/rtc.h"
+#endif
+#include "pm_layered.h"


I think this include should be made conditional with #ifdef MODULE_PM_LAYERED

Yes. Fixed.

aabadie · 2018-03-30T07:49:35Z

tests/periph_pm/main.c

+
+    rtc_get_time(&time);
+    time.tm_sec += duration;
+    while (time.tm_sec > 60) {


Do you think it would make sense to use mktime/localtime functions to automatically compute the correct struct tm ?

Not sure how I would use these methods to just add a certain amount of seconds.

I copied this part from #7666.

there can be side effects if adding the duration (in seconds) also increases hour. I think those functions handles that better (not tested though)

Ah I see. Found something here, and it seems to work (ok, I didn't test whole hours, but minutes is OK).

aabadie · 2018-03-30T07:53:36Z

tests/periph_pm/main.c

+    char line_buf[SHELL_DEFAULT_BUFSIZE];
+
+    /* print some information about the modes */
+    printf("This application allows you to test the power management\n"


Not sure if power management is a peripheral. I would just say to test the CPU power management

aabadie · 2018-03-30T07:54:30Z

tests/periph_pm/main.c

+/**
+ * @brief   List of shell commands for this example.
+ */
+static const shell_command_t shell_commands[] = {


In general this array is created just before the main function

I don't understand this. Can you explain?

Do you mean that I should move this just before the main method?

Yes, and then you don't need to declare the command functions.

aabadie · 2018-03-30T07:58:16Z

tests/periph_pm/main.c

+#ifdef MODULE_PM_LAYERED
+    { "block", "block power mode", cmd_block },
+#endif
+    { "lowest", "enter lowest mode", cmd_lowest },


Why is this command needed ? if one unblocks a power mode with unblock command, then the main thread (only thread running) will be blocked (waiting for incoming commands), so the system should switch to idle thread that automatically calls pm_set_lowest.

Removed cmd_lowest.

Out-of-curiosity, do you confirm that it works as expected (in terms of power consumption) ?

Yes, I observe a difference when I change the power modes.

really nice!

basilfx · 2018-03-30T08:50:38Z

Fixed the comments and rebased.

aabadie · 2018-03-30T08:52:12Z

tests/periph_pm/main.c

+#include "periph/rtc.h"
+#endif
+#endif
+#include "pm_layered.h"


include is still out of the conditional #ifdef

Oops, sorry. Fixed that one.

aabadie · 2018-03-30T09:02:26Z

tests/periph_pm/main.c

+
+    return 0;
+}
+#endif


Maybe add a comment mentioning which block is closed here (MODULE_PM_LAYERED)

aabadie · 2018-03-30T09:03:39Z

tests/periph_pm/main.c

+}
+
+#ifdef MODULE_PM_LAYERED
+static int cmd_set(int argc, char **argv)


Maybe these functions could be grouped in the same #ifdef MODULE_PM_LAYERED above block.

aabadie · 2018-03-30T09:04:36Z

tests/periph_pm/main.c

+ */
+static const shell_command_t shell_commands[] = {
+#ifdef MODULE_PM_LAYERED
+    { "block", "block power mode", cmd_block },


Same here, this line can be moved before the unblock line.

Wanted to keep it sorted alphabetically. Will move it to first one in the other MODULE_PM_LAYERED block.

aabadie · 2018-03-30T09:11:13Z

tests/periph_pm/Makefile

+BOARD ?= slstk3401a
+include ../Makefile.tests_common
+
+FEATURES_REQUIRED += periph_pm


I don't think this is needed, this feature is already optional by default (see Makefile.dep). And that's probably why the CI is failing.

aabadie · 2018-03-30T09:35:09Z

tests/periph_pm/main.c

+#endif /* MODULE_PERIPH_RTC */
+#endif /* MODULE_PM_LAYERED */
+
+static int cmd_off(int argc, char **argv)


cmd_off and cmd_reboot should be moved to the top and then all functions that requires MODULE_PM_LAYERED could be grouped in the same conditional #ifdef.

I'd like to keep it this way: I now have the commands grouped together, and utility functions on top.

aabadie · 2018-03-30T10:00:59Z

tests/periph_pm/main.c

+           "The available power modes are 0 - %d. Lower-numbered power modes\n"
+           "save more power, but may require an event/interrupt to wake up\n"
+           "the CPU. Reset the CPU if needed.\n",
+           PM_NUM_MODES - 1);


The use of PM_NUM_MODE should be excluded for boards not providing the pm_layered module.

basilfx · 2018-04-02T19:37:10Z

Few MSP430-based board don't compile due to missing fflush. Any ideas?

jnohlgard · 2018-04-02T19:46:40Z

If we can get #6445 finished all such problems would go away.

basilfx · 2018-04-03T08:46:18Z

Thanks @gebart for pointing that out.

Since #6445 is not active, I propose to either blacklist the boards, or to put #ifndef MODULE_MSP430_COMMON around fflush.

I prefer the latter, since the test itself is still valuable.

aabadie · 2018-04-03T08:47:32Z

I propose to either blacklist the boards

+1 for that solution (already used in other places)

basilfx · 2018-04-03T16:15:36Z

@aabadie Can I squash? Murdock is almost happy now :-)

aabadie

Few remaining comments, mainly regarding the parse_mode function.

aabadie · 2018-04-04T08:51:34Z

tests/periph_pm/main.c

+        return -1;
+    }
+
+    int mode = atoi(argv[1]);


mode could be an uint8_t

aabadie · 2018-04-04T08:51:55Z

tests/periph_pm/main.c

+
+    int mode = atoi(argv[1]);
+
+    if (mode < 0 || mode >= (int) PM_NUM_MODES) {


then here there's no need to check that mode is negative

aabadie · 2018-04-04T08:54:54Z

tests/periph_pm/main.c

+#ifdef MODULE_PM_LAYERED
+static int cmd_block(int argc, char **argv)
+{
+    int mode = parse_mode(argc, argv);


mode could be uint8_t

aabadie · 2018-04-04T08:55:23Z

tests/periph_pm/main.c

+{
+    int mode = parse_mode(argc, argv);
+
+    if (mode < 0) {


parse_mode function is already testing this

This would indicate a error code, so I need to check this here.

aabadie · 2018-04-04T08:55:40Z

tests/periph_pm/main.c

+{
+    int mode = parse_mode(argc, argv);
+
+    if (mode < 0) {


same comment about parse_mode

This would indicate a error code, so I need to check this here.

aabadie · 2018-04-04T08:55:51Z

tests/periph_pm/main.c

+
+static int cmd_unblock(int argc, char **argv)
+{
+    int mode = parse_mode(argc, argv);


Can you explain?

arf, sorry, re-read the parse_mode function and testing the negative returned value is perfectly fine. The comment about uint8_t (in parse_mode) still stands.

aabadie · 2018-04-04T08:56:08Z

tests/periph_pm/main.c

+    int mode = parse_mode(argc, argv);
+    int duration = parse_duration(argc, argv);
+
+    if (mode < 0 || duration < 0) {


again parse_mode

This would indicate a error code, so I need to check this here.

basilfx · 2018-04-04T11:15:14Z

Processed feedback and rebased.

basilfx · 2018-04-04T18:47:09Z

@aabadie Good enough to squash?

aabadie

Played a bit with the test application and I have other comments.
I think it works but the commands messages (in case of usage errors) could be improved. I'm proposing a refactoring of parse_mode and parse_duration function.

aabadie · 2018-04-05T07:14:09Z

tests/periph_pm/main.c

+#ifdef MODULE_PM_LAYERED
+static int parse_mode(int argc, char **argv)
+{
+    if (argc < 2) {


This should be moved to each command function handler and if a wrong number of arguments is given, the command prints a usage message Usage: unblock <power mode> or Usage: unblock_rtc <power mode> <duration>.
Otherwise the error message doesn't give any hint on how to correctly use the command. Maybe introduce a _check_usage(int argc, int expected_argc) function for this ?

Then the parse_mode and parse_duration could be simplified with this signature:

static int _parse_mode(const char * mode) static int _parse_duration(const char * duration)

basilfx · 2018-04-05T08:25:52Z

@aabadie Have improved argument parsing + usage message. Is this better?

aabadie

no more comment, ACK

Please squash.

For the record, I tested a bit the application with an stm32l0 based board and had some problems: low power modes doesn't switch automatically the first time, I have to insist a bit with the unblock command (several calls in a row). This is unrelated to this PR but at least the application is useful to exhibit the problem.

basilfx · 2018-04-05T09:19:49Z

Squashed. Murdock is green.

aabadie · 2018-04-05T09:32:45Z

Then go!

basilfx added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Mar 28, 2018

basilfx force-pushed the feature/test_periph_pm branch from 10766ec to 2a4baff Compare March 29, 2018 22:05

basilfx force-pushed the feature/test_periph_pm branch from 2a4baff to fa9846c Compare March 29, 2018 22:44

vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 30, 2018

vincent-d reviewed Mar 30, 2018

View reviewed changes

vincent-d approved these changes Mar 30, 2018

View reviewed changes

aabadie requested changes Mar 30, 2018

View reviewed changes

basilfx force-pushed the feature/test_periph_pm branch from fa9846c to ba346ae Compare March 30, 2018 08:48

aabadie reviewed Mar 30, 2018

View reviewed changes

basilfx force-pushed the feature/test_periph_pm branch from 76411af to 3bf1f8a Compare March 30, 2018 09:30

basilfx added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 30, 2018

aabadie reviewed Mar 30, 2018

View reviewed changes

basilfx mentioned this pull request Apr 1, 2018

cpu: lpc1768: provide periph_pm #8861

Merged

basilfx force-pushed the feature/test_periph_pm branch from 47fd045 to 095d8a8 Compare April 2, 2018 17:08

basilfx force-pushed the feature/test_periph_pm branch from 095d8a8 to 78cb81e Compare April 3, 2018 11:42

aabadie requested changes Apr 4, 2018

View reviewed changes

basilfx force-pushed the feature/test_periph_pm branch from 78cb81e to ea04b6f Compare April 4, 2018 10:53

aabadie requested changes Apr 5, 2018

View reviewed changes

basilfx force-pushed the feature/test_periph_pm branch from ea04b6f to ad6b12d Compare April 5, 2018 08:25

aabadie approved these changes Apr 5, 2018

View reviewed changes

tests: periph_pm: add peripheral test.

74d496f

basilfx force-pushed the feature/test_periph_pm branch from ad6b12d to 74d496f Compare April 5, 2018 08:58

aabadie merged commit 1eaec49 into RIOT-OS:master Apr 5, 2018

aabadie mentioned this pull request Apr 5, 2018

pm_layered: add test app #7666

Closed

basilfx deleted the feature/test_periph_pm branch January 14, 2020 22:05


		Some power modes may require a certain event to wake up. Reset the CPU if needed.

		If a RTC peripheral is available, an additional command to temporary unblock a power mode is


		int mode = atoi(argv[1]);

		if (mode < 0 \|\| mode >= (int) PM_NUM_MODES) {

Conversation

basilfx commented Mar 28, 2018

Contribution description

Issues/PRs references

Uh oh!

jnohlgard commented Mar 28, 2018

Uh oh!

aabadie commented Mar 29, 2018

Uh oh!

basilfx commented Mar 29, 2018

Uh oh!

vincent-d commented Mar 29, 2018

Uh oh!

basilfx commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincent-d left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basilfx commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aabadie Mar 30, 2018 •

edited

Loading

aabadie Mar 30, 2018 •

edited

Loading

aabadie Mar 30, 2018 •

edited

Loading

aabadie Mar 30, 2018 •

edited

Loading

aabadie Mar 30, 2018 •

edited

Loading

aabadie Mar 30, 2018 •

edited

Loading

aabadie Mar 30, 2018 •

edited

Loading