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

periph/pm: pm_layered never used #6655

Closed
immesys opened this issue Feb 25, 2017 · 5 comments
Closed

periph/pm: pm_layered never used #6655

immesys opened this issue Feb 25, 2017 · 5 comments
Assignees
Labels
Area: pm Area: (Low) power management Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@immesys
Copy link
Contributor

immesys commented Feb 25, 2017

On SAMR21 the pm_set_lowest implemented in sys/pm_layered is never called because the weakly defined noop version in drivers/periph_common takes precedence. If I knew why, this would be a pull request not an issue.

To see this, you first have to fix #6640 which at the time of writing is not merged. So

git apply <<EOF
diff --git a/cpu/cortexm_common/periph/pm.c b/cpu/cortexm_common/periph/pm.c
index dacf1b6..1993074 100644
--- a/cpu/cortexm_common/periph/pm.c
+++ b/cpu/cortexm_common/periph/pm.c
@@ -23,7 +23,7 @@
 #include "cpu.h"
 #include "periph/pm.h"
 
-#ifndef FEATURES_PERIPH_PM
+#ifndef FEATURE_PERIPH_PM
 void pm_set_lowest(void)
 {
     cortexm_sleep(0);
EOF

Then compile a trivial example for samr21

cd examples/timer_periodic_wakeup
BOARD=samr21-xpro make 

Now look at the symbol for pm_set_lowest:

arm-none-eabi-readelf bin/samr21-xpro/timer_periodic_wakeup.elf -s | grep "pm_set"
   508: 00000439     2 FUNC    WEAK   DEFAULT    1 pm_set_lowest

It's the weak symbol that was defined in drivers/periph_common which does nothing.

Now remove the fallback weak symbol:

git apply <<EOF
diff --git a/drivers/periph_common/pm.c b/drivers/periph_common/pm.c
index afd31b7..37b03c3 100644
--- a/drivers/periph_common/pm.c
+++ b/drivers/periph_common/pm.c
@@ -24,7 +24,6 @@
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
-void __attribute__((weak)) pm_set_lowest(void) {}
 
 void __attribute__((weak)) pm_off(void)
 {
EOF

Now recompile the app and check the symbol. The proper one in sys/pm_layered is now there and it is called at runtime. Consequently the cpu's pm_set function is no longer stripped and actually appears too.

arm-none-eabi-readelf bin/samr21-xpro/timer_periodic_wakeup.elf -s | grep "pm_set"
   521: 00000fe5    68 FUNC    GLOBAL DEFAULT    1 pm_set_lowest
   531: 000012d5    68 FUNC    GLOBAL DEFAULT    1 pm_set

At present this bug means that the new power management system doesn't work and the SAMR21 stays at 5mA+ even when doing nothing. I'm not sure why testing did not detect this, but we should fix it soon.

@immesys
Copy link
Contributor Author

immesys commented Feb 25, 2017

In case it's relevant. I am compiling with arm-none-eabi-gcc 4.9.3 20150529 (prerelease) (15:4.9.3+svn231177-1).

@cgundogan cgundogan added Area: pm Area: (Low) power management Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Mar 7, 2017
@cgundogan cgundogan added this to the Release 2017.04 milestone Mar 7, 2017
@kYc0o
Copy link
Contributor

kYc0o commented Apr 25, 2017

Same as #6709, should we close one of them?

@miri64
Copy link
Member

miri64 commented Apr 25, 2017

I would propose to close this one. It is referenced in #6709 so the information provided here is still accessible, but the discussion in #6709 went a little bit on already.

@roberthartung
Copy link
Member

@immesys can you recheck with the 2017.10 branch?

@kaspar030
Copy link
Contributor

The weak functions don't exist anymore since #7726.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pm Area: (Low) power management Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

No branches or pull requests

8 participants