pm: fix pm fallback selection by haukepetersen · Pull Request #7726 · RIOT-OS/RIOT

haukepetersen · 2017-10-12T14:25:53Z

Created this PR just to illustrate my ideas for #7648 and I'd rather have the changes merged in #7648 instead of merging this PR...

But anyhow, I think this approach simplifies matters compared to #7648 and could be the way to go? This approach does not depend on any weak symbols, nor on the order of the include paths, so it should be pretty dependable. What do you think?

66 additions and 15 deletions vs. 531 additions and 13 deletions

roberthartung · 2017-10-12T14:28:18Z

cpu/cortexm_common/periph/pm.c

 #include "periph/pm.h"

-#ifndef FEATURE_PERIPH_PM
+#ifndef MODULE_PM_LAYERED


Shouldn't this be #ifndef PROVIDES_PM_SET_LOWEST? Because any other CPU could in theory define pm_set_lowest

The problem with using PROVIDES_PM_SET_LOWEST here is, that it does not clearly differentiate from the fallback implementation in drivers/periph_common/pm.c (which uses the that guard).

But true, this line needs to be changed, how about

#if !defined(MODULE_PM_LAYERED) && !defined(PROVIDES_PM_SET_LOWEST_CORTEXM)

So we have 4 possible cases:

CPU branch uses pm_set_lowest() from periph_common -> do nothing

CPU uses 'set_lowest()defined by architecture -> definePROVIDES_PM_SET_LOWEST` by CPU architecture, do nothing else

CPU uses pm_layered -> simply use that module, it defines PROVIDES_PM_SET_LOWEST in any case (if not done already by the CPU architecture config)

CPU wants to use its own implementation -> CPU defines PROVIDES_PM_SET_LOWEST_CORTEXM', and do not use pm_layered`

roberthartung · 2017-10-12T14:29:43Z

cpu/stm32_common/include/periph_cpu_common.h

+ * @name    Power management configuration
+ * @{
+ */
+#if defined(CPU_FAM_STM32F1) || defined(CPU_FAM_STM32F2) || defined(CPU_FAM_STM32F4)


Should be #define PROVIDES_PM_OFF in my opinion. As the function is still called pm_off().

nope, as we want to explicitly override the pm_off function provided by the pm_layered module.

Rationale: for the 3 CPU families specified above, we know that we use the pm_layered module. So if we want to use a custom pm_off function, we need to explicitly override the one provided by pm_layered (which in turn overrides the default one from periph_common).

roberthartung · 2017-10-12T14:30:55Z

sys/include/pm_layered.h

+extern "C" {
+#endif
+
+#ifndef PROVIDES_PM_OFF


I personally don't like mixing PROVIDED_PM_OFF and PROVIDES_PM_LAYERED_OFF. Can we get rid of this? Or is this required?

IMHO this is required, as each of those defines overrides a specific function in the 'fallback hierarchy'. As I see it, this is the price to pay for having multiple tiers of fallback functions.

But maybe we come up with an even better idea?

kaspar030 · 2017-10-12T14:32:26Z

Very nice idea: provide instead of need... Also re-using the existing config headers.

@roberthartung what do you think?

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

fixed all boards but the MIPS, need to install the toolchain locally to debug this... fix follows asap.

roberthartung · 2017-10-12T14:58:37Z

fixed all boards but the MIPS, need to install the toolchain locally to debug this... fix follows asap.

make BUILD_IN_DOCKER=1 buildtest :-)

haukepetersen · 2017-10-12T15:12:52Z

done, at least locally they should be all set.

haukepetersen · 2017-10-13T08:07:03Z

@roberthartung @kaspar030: what is your verdict? And how to proceed? Would be nice to get one of the fixes merged today, so we have it for sure in the release!

roberthartung · 2017-10-16T06:47:51Z

@haukepetersen I'd vote for merging your PR as it is (if all tests are passing ofc). The solution is good and I want to make progress with my own atmega PM implementations.

MichelRottleuthner · 2017-10-16T08:14:29Z

I agree with @roberthartung. I prefer this solution. Also I did a quick measurement on samr21-xpro and can confirm the low power modes actually work as intended. This is already an improvement compared to the broken weak implementation.

roberthartung · 2017-10-16T09:44:40Z

@MichelRottleuthner thanks for checking on the board! That's great. I think it's far more than an improvement. This might be a very good, clean solution while keeping flexibility.
I will pull this PR locally and check if I can rebase my implementation and will then do some energy measurements on my board.

haukepetersen · 2017-10-16T09:52:58Z

Very cool. Then I will fix this PR now and let's merge it!

haukepetersen · 2017-10-16T10:05:07Z

rebased and fixed missing doc for pic32. Will wait for Murdock to do one more run and then squash.

haukepetersen · 2017-10-16T10:54:49Z

Murdock seems ok, so I will squash now.

kaspar030 · 2017-10-16T10:59:19Z

yes, please squash!

haukepetersen · 2017-10-16T11:15:30Z

squashed.

kaspar030 · 2017-10-16T11:18:39Z

cpu/cortexm_common/include/cpu.h


+/**
+ * @brief   All Cortex-m-based CPUs provide pm_set_lowest
+ *


minor: I think there's a "by" missing in this sentence.

jupp. Fixed and squashed

haukepetersen · 2017-10-16T11:43:38Z

Murdock is happy -> feel free to ACK and press the green button :-)

kaspar030 · 2017-10-16T12:07:13Z

should we set PM_BLOCKER_INITIAL to a safe default?

haukepetersen · 2017-10-16T12:14:13Z

should we set PM_BLOCKER_INITIAL to a safe default?

possibly. But how about a quick follow-up PR for this?

kaspar030 · 2017-10-16T12:15:16Z

But how about a quick follow-up PR for this?

I'll make a commit that you can cherry-pick. IMO it belongs in here.

kaspar030 · 2017-10-16T12:20:50Z

169cffa

haukepetersen · 2017-10-16T12:26:57Z

done.

Instead of using `weak` function definitions, this PR handles default implementations using `PROVIDES_x` defines, allowing for cpus/pm realted modules to use their own implementations.

haukepetersen · 2017-10-16T12:28:36Z

rebased.

kaspar030

ACK.

miri64 · 2017-10-16T12:40:15Z

Let's go and start this release!

roberthartung · 2017-10-20T11:37:24Z

@kaspar030 @haukepetersen I just tried to rebase my pm implementation for the atmega_common/atmega1284p and I am stuck at a point, where I select pm_layered im Makefile.include of my CPU. pm_layered is compiled correctly. However both pm.c from periph_common and pm_layered provide pm_set_lowest. Shouldn't we add

#ifndef PROVIDES_PM_SET_LOWEST
#define PROVIDES_PM_SET_LOWEST
#endif

to sys/include/pm_layered.h - at least until the default pm.c module is not build any more by accident?

haukepetersen · 2017-10-25T13:19:28Z

sorry for the delayed reply. Just trying to get my head around this: in the case a CPU is using pm_layered, it always wants to use the pm_set_lowest() function as provided by the pm_layered module, right? So yes, I think you are right! Would you be so kind to open a PR?!

roberthartung · 2017-10-25T16:00:13Z

@haukepetersen Just finished rebasing my atmega_common pm stuff on the latest master ... hooray! Will create a PR tomorrow.

haukepetersen · 2017-10-25T16:01:16Z

Awesome, looking forward to it!

roberthartung · 2017-10-25T16:03:39Z

@haukepetersen I just wonder why this isn't conflicting at the moment as well, as some CPUs also use pm_layered already?

roberthartung · 2017-10-25T16:34:35Z

Confirmed. Does not happen with other boards. But why? Maybe it has something todo with _common. Will investigate.

roberthartung · 2017-10-26T08:11:16Z

@haukepetersen ping #7863

haukepetersen assigned roberthartung, kaspar030 and MichelRottleuthner Oct 12, 2017

haukepetersen mentioned this pull request Oct 12, 2017

pm: adds a c/file-based power management configuration to select required implementations #7648

Closed

roberthartung reviewed Oct 12, 2017

View reviewed changes

haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 12, 2017

haukepetersen force-pushed the opt_pm_fixweak branch from 046eeae to b3cbd29 Compare October 16, 2017 10:04

haukepetersen force-pushed the opt_pm_fixweak branch from 9665a8b to 09ec992 Compare October 16, 2017 11:15

kaspar030 reviewed Oct 16, 2017

View reviewed changes

haukepetersen force-pushed the opt_pm_fixweak branch from 09ec992 to 2e12239 Compare October 16, 2017 11:23

kaspar030 changed the title ~~pm: alternate approach for selecting correct fallback functions~~ pm: fix pm fallback function selection Oct 16, 2017

kaspar030 changed the title ~~pm: fix pm fallback function selection~~ pm: fix pm fallback selection Oct 16, 2017

haukepetersen added 2 commits October 16, 2017 14:27

pm: fix weak-based default implementations

5920d99

Instead of using `weak` function definitions, this PR handles default implementations using `PROVIDES_x` defines, allowing for cpus/pm realted modules to use their own implementations.

cpu/mips_pic32: fix doxygen for GPIO_PIN macro

942bfec

haukepetersen force-pushed the opt_pm_fixweak branch from 8e92517 to 829271d Compare October 16, 2017 12:28

pm: unify and set safe default pm_blocker

416ac62

haukepetersen force-pushed the opt_pm_fixweak branch from 829271d to 416ac62 Compare October 16, 2017 12:32

kaspar030 approved these changes Oct 16, 2017

View reviewed changes

miri64 merged commit 28a6128 into RIOT-OS:master Oct 16, 2017

haukepetersen deleted the opt_pm_fixweak branch October 16, 2017 12:49

photonthunder mentioned this pull request Oct 17, 2017

PM and samd21 #7743

Closed

This was referenced Oct 18, 2017

cpu, cc2538: fix pm, don't go into cortexm_sleep(0) #7752

Merged

cpu, cc26x0: fix pm, don't go into cortexm_sleep(0) #7774

Merged

roberthartung mentioned this pull request Oct 26, 2017

pm_layered: make sure no conflict for pm_set_lowest exists #7863

Merged

This was referenced Oct 28, 2017

pm: WIP for power management that uses submodules #7597

Closed

pm: fix pm configuration #7308

Closed

pm: use of wrong pm_set_lowest? #6709

Closed

periph/pm: pm_layered never used #6655

Closed

Conversation

haukepetersen commented Oct 12, 2017

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!

haukepetersen Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

roberthartung Oct 12, 2017 • 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!

haukepetersen Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

kaspar030 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!

haukepetersen commented Oct 13, 2017

Uh oh!

roberthartung commented Oct 16, 2017

Uh oh!

MichelRottleuthner commented Oct 16, 2017

Uh oh!

roberthartung commented Oct 16, 2017

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

kaspar030 commented Oct 16, 2017

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

kaspar030 Oct 16, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen Oct 16, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

kaspar030 commented Oct 16, 2017

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

kaspar030 commented Oct 16, 2017

Uh oh!

kaspar030 commented Oct 16, 2017

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

haukepetersen commented Oct 16, 2017

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 16, 2017

Uh oh!

roberthartung commented Oct 20, 2017

Uh oh!

haukepetersen commented Oct 25, 2017

Uh oh!

roberthartung commented Oct 25, 2017

Uh oh!

haukepetersen commented Oct 25, 2017

roberthartung Oct 12, 2017 •

edited

Loading