pm: adds a c/file-based power management configuration to select required implementations by roberthartung · Pull Request #7648 · RIOT-OS/RIOT

roberthartung · 2017-09-26T09:47:06Z

Supersedes #7597 and porposes an alternative for the submodule/buildsystem-based version.
I just ran the buildtest by now, but didnt test any hardware yet!

@kaspar030 @MichelRottleuthner @haukepetersen Feedback appreciated!

kaspar030 · 2017-09-28T07:11:33Z

This might be nicer than the submodule version.
Some thoughts:

There are a lot of "pm_conf.h" which just include "pm_conf_common.h". If I'm not mistaken, "pm_conf.h" either does that or defines all needed values. I think we should be able to rely on the include path order here (first cpu/specific/include, then cpu/family_common/include, ...), and just rename all "pm_conf_common.h" to "pm_conf.h". If the order is not deterministic, we should fix that (in another PR) anyways.
the defines get veeery long. IMO we can shorten them. Maybe drop "PERIPH_PM" prefix, and drop "COMMON", which should turn into sufficiently unique names. E.g.: "PERIPH_PM_NEEDS_ARM7_COMMON_FALLBACK_REBOOT" -> "NEEDS_ARM7_PM_REBOOT".
Some functions are guarded by both general and specific defines, e.g., "NEEDS_PM_FALLBACK | NEEDS_PM_FALBACK_OFF". Please, if possible, bundle the generic selection into one ifdef block (at the top of the file?), and keep only one condition at the function definition:

#ifdef NEEDS_PM_FALLBACK
#define NEEDS_PM_OFF_FALLBACK
#define NEEDS_PM_SET_LOWEST_FALLBACK
...
#endif

roberthartung · 2017-10-04T09:44:04Z

@kaspar030 sounds nice. Will make the changes once I am back from vacation!

cladmi · 2017-10-06T14:12:21Z

@kaspar030
Is there other places in the codebase right now where some similar configuration is done ?
Like a cpu/board header changing which functions are actually built in another module.

And same for using include path order to resolve header files include.

kaspar030 · 2017-10-09T08:56:18Z

Is there other places in the codebase right now where some similar configuration is done ?

periph_spi_common is doing something similar. grep PERIPH_SPI_NEEDS_.

roberthartung · 2017-10-12T11:33:40Z

@kaspar030 Almost finished. If I provide my own configuration for pm, the pm_conf.h will be empty. Is there a better way? Or should we just place an empty pm_conf.h in that case with according comments?

Additionally, I cannot build for native:

make BUILD_IN_DOCKER=1 BOARD=native
...
cc1: fatal error: /data/riotbuild/riotproject/examples/default/bin/native/riotbuild/riotbuild.h: No such file or directory

EDIT: Nevermind, can build native now .. for some reason..

…ecting fallbacks

roberthartung · 2017-10-12T11:36:27Z

@kaspar030 @cladmi I just pushed a shortened version. I really like this one!

haukepetersen

Sorry for jumping the ship so late and being quiet so far. I very much like the direction of this PR, but there are still things I don't like and which can be further simplified.

For one, I am not too much a fan of introducing all these pm specific pm_conf.h header files. I actually think we can do without, by switching the paradigm of the config parameters from NEEDS to PROVIDE.

For pm_off and pm_reboot this would simplify things -> the fallback implementation (pm_reboot() in cpu/ARCH/pm.c and pm_off() in drivers/periph_common/pm.c) are simply guarded with:

#ifndef PROVIDES_PM_OFF
...
#endif

This define should simply reside in periph_cpu.h (or any shared file like periph_cpu_common.h if valid for a complete family of CPUs).

Next, the pm_off() in sys/pm_layered/pm.c is guarded with

#ifndef PROVIDES_PM_LAYERED_OFF
...
#endif

Now a CPU that does not use pm_layered simply does nothing if it wants to use the default implementation in periph_common.

Next, the pm_layered module does define PROVIDES_PM_OFF, so if a CPU uses pm_layered, it would automatically use the pm_off function provided by pm_layered.

The only 'hack' that has to be done here, is to conditionally include the pm_layered.h header in pm.h.

Does this make sense? I could draft a quick branch if it would be easier to understand, should I proceed here?

haukepetersen · 2017-10-12T13:21:03Z

cpu/arm7_common/include/pm_conf.h

+ * Select the required implementations from the common CPUs or fallbacks
+ * @{
+ */
+#define NEEDS_ARM7_PM_REBOOT


might be nitpicky, but I would definitely prefer NEEDS_PM_ARM7_REBOOT to keep pm as prefix and make clear that this is a pm specific option

haukepetersen · 2017-10-12T13:26:02Z

cpu/cortexm_common/periph/pm.c

 }
 #endif

+#if defined(NEEDS_CORTEXM_PM_REBOOT)


Question: why is pm_reboot guarded, if it is always used anyway? There is no case in the source tree, where the pm_reboot function provided by the architecture is not used... So isn't this a case of 'add the guard once we encounter a case where its needed' instead of 'adding it now because sometime in the future it might be needed'?

Because one could define their own implementation. For all *_common CPUs I added guards around their implementations.

haukepetersen · 2017-10-12T13:46:37Z

cpu/mips32r2_common/periph/pm.c

-#ifndef FEATURES_PERIPH_PM
+#ifdef NEEDS_MIPS32R2_PM_FALLBACK
+#define NEEDS_MIPS32R2_PM_OFF
+#define NEEDS_MIPS32R2_PM_OFF


duplicate line?

yes. it's a typo.

roberthartung · 2017-10-12T14:26:04Z

Does this make sense? I could draft a quick branch if it would be easier to understand, should I proceed here?

@haukepetersen I know what you mean but can we cover the same functionality? Especially when mixing different fallback implementations? e.g. can we cover the cases in cpu/nrf5x_common/include/pm_conf.h and cpu/stm32_common/include/pm_conf.h.

haukepetersen · 2017-10-12T14:26:09Z

Couldn't resist to try it out: #7726

haukepetersen · 2017-10-12T14:28:42Z

Especially when mixing different fallback implementations?

Yes, just tried it in #7726. The nrf5x falls back to the pm_set_lowest() as provided in cortexm/periph/pm.c, while the stm32 uses the pm_set_lowest() provided in sys/pm_layered/pm.c. At least as far as I have seen in my quick tests -> please correct me if I **ucked up :-)

roberthartung · 2017-10-12T14:31:40Z

Have you tried to run buildtest on the default example?

haukepetersen · 2017-10-12T14:47:52Z

I did just Murdock do that job for me -> fails all the way :-) Looking into it now.

…esses

kaspar030 · 2017-10-28T18:50:56Z

Closed in favor of #7726. @roberthartung Thanks a lot for trying this out!

roberthartung mentioned this pull request Sep 26, 2017

Implement power management pm for CPUs #6802

Closed

20 tasks

roberthartung mentioned this pull request Oct 4, 2017

make: use submodules for periph #7241

Merged

miri64 requested review from haukepetersen and kaspar030 October 4, 2017 14:12

miri64 assigned haukepetersen, kaspar030 and MichelRottleuthner and unassigned haukepetersen and kaspar030 Oct 4, 2017

miri64 requested a review from MichelRottleuthner October 4, 2017 14:13

miri64 added the Area: pm Area: (Low) power management label Oct 4, 2017

pm: removes weak definitions from pm and adds pm_conf.h files for sel…

29895aa

…ecting fallbacks

roberthartung force-pushed the pm-c-header-conf branch from f22d8b3 to 29895aa Compare October 12, 2017 11:35

haukepetersen requested changes Oct 12, 2017

View reviewed changes

haukepetersen reviewed Oct 12, 2017

View reviewed changes

haukepetersen mentioned this pull request Oct 12, 2017

pm: fix pm fallback selection #7726

Merged

schedstats: revert RIOT-OS#6975, use 32Bit again

fdee200

miri64 and others added 27 commits October 20, 2017 12:43

nrfmin: port for gnrc_netif2

31dff67

xtimer: Set XTIMER_SHIFT to 0 if underlying timer is 32768 Hz

74496b7

cbor: fix integer overflow in decode_bytes

6ab0320

cpu/stm32_common: use pseudomodule for UART HW FC

89f1dd5

drivers/sx127x: fix set modem logic

f572bc2

drivers/sx127x: fix driver inconsistencies with semtech original driver

332fac9

tests/driver_sx127x: put radio to sleep after transmission

d2f02c3

drivers/sx127x: move netdev related callback to the right file

ed983d5

cpu, sam0_common: fix uart TXC check

c428265

sys: random: add xorshift prng

94ced82

cppcheck: Provide consistent reason formatting for all cppcheck-suppr…

163d48a

…esses

dist/tools/cppcheck: Allow individual files to be cppchecked

c34e377

shell_commands: port ifconfig command for gnrc_netif2

31cc902

auto_init: add netdev_tap gnrc_netif2 initialization for testing

7a73876

gnrc_netif2: make gnrc_netif2_create idempotent and crash on error

d4b9221

nrfmin: adapt for changes to gnrc_netif2_create()

2e1af59

tests: adapt gnrc_netif2 test for changes in gnrc_netif2_create()

cd24538

auto_init: upgrade bootstrap of Ethernet devices for gnrc_netif2

8eba0f3

gnrc_netif2: add IEEE 802.15.4 support

261ad49

tests: add tests for IEEE 802.15.4 to gnrc_netif2 tests

40e10d5

auto_init: upgrade bootstrap of IEEE 802.15.4 devices for gnrc_netif2

2179130

slip: port to be used with netdev

f5747ee

can/device: improve bus-off management

eb57b17

cpu/stm32_common: fix clk_conf

6941c3c

drivers: remove accidentally added binary Makefile

7025ed7

saul_reg: make ptr to device descriptor non const

982c783

fixup! fix typo

ba1544e

kaspar030 closed this Oct 28, 2017

roberthartung mentioned this pull request Feb 7, 2020

github: add CODEOWNERS file #13289

Merged

Conversation

roberthartung commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Sep 28, 2017

Uh oh!

roberthartung commented Oct 4, 2017

Uh oh!

cladmi commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Oct 9, 2017

Uh oh!

roberthartung commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberthartung commented Oct 12, 2017

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

roberthartung Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

roberthartung Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

roberthartung commented Oct 12, 2017

Uh oh!

haukepetersen commented Oct 12, 2017

Uh oh!

haukepetersen commented Oct 12, 2017

Uh oh!

roberthartung commented Oct 12, 2017

Uh oh!

haukepetersen commented Oct 12, 2017

Uh oh!

kaspar030 commented Oct 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

roberthartung commented Sep 26, 2017 •

edited

Loading

cladmi commented Oct 6, 2017 •

edited

Loading

roberthartung commented Oct 12, 2017 •

edited

Loading