Re: [riot-devel] Discussion of Power Management
Hi, On 31.08.2017 17:42, Kaspar Schleiser wrote: > >> >> In any case, if I added USE_MODULES += periph_common_pm_fallback, it >> will not get compiled. Any idea why? > > Did you use "USEMODULE" instead of "USE_MODULES"? Yes, did not work :( > Maybe the periph_common directory doesn't get selected. But we can't > make periph_common_pm_fallback depend on periph_common, that might have > side effects. > > How about drivers/pm_fallback/... for now? Then I'd vote for moving pm_layered to drivers as well? Or should be make it sys/pm_fallback/? - Robert -- Robert Hartung, M.Sc. Technische Universität Braunschweig Institut für Betriebssysteme und Rechnerverbund Mühlenpfordtstr. 23, Raum 115 38106 Braunschweig Fon: +49 (531) 391 - 3264 Fax: +49 (531) 391 - 5936 E-Mail: hart...@ibr.cs.tu-bs.de ___ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
Re: [riot-devel] Discussion of Power Management
Hi, On 08/31/2017 05:13 PM, Robert Hartung wrote: >> SRC := $(pm_fallback.c,$(wildcard *.c)) >> to drivers/periph_common/Makefile >> >> - add "PSEUDOMODULES += periph_common_%" to makefiles/pseudomodules.mk >> >> That would compile pm_fallback *only* if periph_common_pm_fallback is >> selected. (with #5757, all periph files will become submodules...) > > This does not work for me, should it be > SRC := $(filter-out pm_fallback.c,$(wildcard *.c)) > ? Ah, yes. > > In any case, if I added USE_MODULES += periph_common_pm_fallback, it > will not get compiled. Any idea why? Did you use "USEMODULE" instead of "USE_MODULES"? Maybe the periph_common directory doesn't get selected. But we can't make periph_common_pm_fallback depend on periph_common, that might have side effects. How about drivers/pm_fallback/... for now? Kaspar ___ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
Re: [riot-devel] Discussion of Power Management
Hi Kaspar, On 31.08.2017 17:02, Kaspar Schleiser wrote: > Hi Robert, > > On 08/31/2017 04:37 PM, Robert Hartung wrote: >>> - if not, possibly e.g., kinetis_common has pm_set(), then that should >>> depend on pm_layered >> >> This means that kinetis_common should provide a module >> **kinetis_common_pm** that provides pm_set. The CPU should then depend >> on this module. Additionally, it should select pm_layered. > > Yes! > >>> - if not, it depends on periph_pm_fallback (which should wrap current >>> drivers/periph_common/pm.c) >> >> In this case we use the existing pm_* functions from >> drivers/periph_common/pm.c, but I would move them to the module I >> created (pm_fallback?). > > Perfect! > >> I am working on a pm_fallback module (moved it from drivers/periph/pm.c >> to sys/pm_fallback/pm.c) - is that the right location? Or where should >> it be located? > > I think as the periph common code is in drivers/, so should the fallback > code. Just asked because pm_layered is in sys/ ;-) > > How about using submodules within drivers/periph_common/Makefile? E.g., > > - rename drivers/periph_common/pm.c to pm_fallback.c > - add > # exclude submodule sources from *.c wildcard source selection > SRC := $(pm_fallback.c,$(wildcard *.c)) > to drivers/periph_common/Makefile > > - add "PSEUDOMODULES += periph_common_%" to makefiles/pseudomodules.mk > > That would compile pm_fallback *only* if periph_common_pm_fallback is > selected. (with #5757, all periph files will become submodules...) This does not work for me, should it be SRC := $(filter-out pm_fallback.c,$(wildcard *.c)) ? In any case, if I added USE_MODULES += periph_common_pm_fallback, it will not get compiled. Any idea why? > kinetis_common/periph/pm.c can just go (and be replaced with a > dependency on the cortem_common code). I will check that - Robert -- Robert Hartung, M.Sc. Technische Universität Braunschweig Institut für Betriebssysteme und Rechnerverbund Mühlenpfordtstr. 23, Raum 115 38106 Braunschweig Fon: +49 (531) 391 - 3264 Fax: +49 (531) 391 - 5936 E-Mail: hart...@ibr.cs.tu-bs.de ___ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
Re: [riot-devel] Discussion of Power Management
Hi Robert, On 08/31/2017 04:37 PM, Robert Hartung wrote: >> - if not, possibly e.g., kinetis_common has pm_set(), then that should >> depend on pm_layered > > This means that kinetis_common should provide a module > **kinetis_common_pm** that provides pm_set. The CPU should then depend > on this module. Additionally, it should select pm_layered. Yes! >> - if not, it depends on periph_pm_fallback (which should wrap current >> drivers/periph_common/pm.c) > > In this case we use the existing pm_* functions from > drivers/periph_common/pm.c, but I would move them to the module I > created (pm_fallback?). Perfect! > I am working on a pm_fallback module (moved it from drivers/periph/pm.c > to sys/pm_fallback/pm.c) - is that the right location? Or where should > it be located? I think as the periph common code is in drivers/, so should the fallback code. How about using submodules within drivers/periph_common/Makefile? E.g., - rename drivers/periph_common/pm.c to pm_fallback.c - add # exclude submodule sources from *.c wildcard source selection SRC := $(pm_fallback.c,$(wildcard *.c)) to drivers/periph_common/Makefile - add "PSEUDOMODULES += periph_common_%" to makefiles/pseudomodules.mk That would compile pm_fallback *only* if periph_common_pm_fallback is selected. (with #5757, all periph files will become submodules...) kinetis_common/periph/pm.c can just go (and be replaced with a dependency on the cortem_common code). Kaspar ___ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
Re: [riot-devel] Discussion of Power Management
Hi Kaspar, thanks for the reply. See answers inline On 31.08.2017 16:15, Kaspar Schleiser wrote: > All of them who define pm_set() support pm_layered. Makes sense. **CPUs should always define what they are going to use!** > cortexm_common just provides a fallback for pm_set_lowest() which at > least sets the ARM CPU into idle. Oh, damn! It's not defining a pm_set. You are completely right, this is how it should be! Got a little confused here. I wonder why the kinetis_common cpu defines a pm_set, but cortexm_common does not? Because the function is cortexm_sleep(0), which should be available to all cortexm_common-based boards/cpus? So why not merge these two? > > This is how I envision it: > > - if a CPU has a proper pm_set(), it depends on pm_layered ACK. pm_layered does everything, except for pm_set. pm_layered will then do all other stuff. > - if not, possibly e.g., kinetis_common has pm_set(), then that should > depend on pm_layered This means that kinetis_common should provide a module **kinetis_common_pm** that provides pm_set. The CPU should then depend on this module. Additionally, it should select pm_layered. > - if not, the cpu (or e.g., kinetis_common) might depend on > cortexm_periph_pm_fallback (where the current cortexm weak defines > should go) Also sounds good. > - if the cpu (or any common ancestor) has it's own way of power > management, it implements pm_set_lowest() Absolutely. Then we just don't use any of the existing pm_* modules. This is the case that can always be done. > - if not, it depends on periph_pm_fallback (which should wrap current > drivers/periph_common/pm.c) In this case we use the existing pm_* functions from drivers/periph_common/pm.c, but I would move them to the module I created (pm_fallback?). I am working on a pm_fallback module (moved it from drivers/periph/pm.c to sys/pm_fallback/pm.c) - is that the right location? Or where should it be located? > That should cover all cases, right? I think so. Let's discuss the naming and location of the fallback modules across CPUs and RIOT in general and I will take care of this. - Robert -- Robert Hartung, M.Sc. Technische Universität Braunschweig Institut für Betriebssysteme und Rechnerverbund Mühlenpfordtstr. 23, Raum 115 38106 Braunschweig Fon: +49 (531) 391 - 3264 Fax: +49 (531) 391 - 5936 E-Mail: hart...@ibr.cs.tu-bs.de ___ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
Re: [riot-devel] Discussion of Power Management
Hi Robert, (I'll CC the list, this may be interesting to others) On 08/31/2017 03:52 PM, Robert Hartung wrote: > The main problem is that it is NOT sufficient to provide pm_layered and > periph_pm. As the various CPUs provide different implementations. Actually, that is sufficient. > The various defines look something like this: > > cpu/saml21: cpu/cortexm_common, cpu/sam0_common > defines pm_set() > > cpu/samd21: cpu/cortexm_common, cpu/sam0_common > defines pm_set() > > boards/mulle: cpu/k60 > > cpu/k60: cpu/cortexm_common, cpu/kinetis_common > > cpu/cortexm_common: > defines pm_set(), pm_set_lowest(), pm_reboot() > > cpu/kinetis_common: > defines pm_set() All of them who define pm_set() support pm_layered. cortexm_common just provides a fallback for pm_set_lowest() which at least sets the ARM CPU into idle. > These are only two examples of conflicts. The question here would be why > do we have multiple "common" CPUs and where should the sleep modes be > implemented? The common cpus are nested, e.g., k60 -> kinetis_common -> cortexm_common. This is how I envision it: - if a CPU has a proper pm_set(), it depends on pm_layered - if not, possibly e.g., kinetis_common has pm_set(), then that should depend on pm_layered - if not, the cpu (or e.g., kinetis_common) might depend on cortexm_periph_pm_fallback (where the current cortexm weak defines should go) - if the cpu (or any common ancestor) has it's own way of power management, it implements pm_set_lowest() - if not, it depends on periph_pm_fallback (which should wrap current drivers/periph_common/pm.c) That should cover all cases, right? > I guess I could just remove the periph/pm.c from kinetis_common? Yes! kinetis_common should use the cortexm_fallback, they're equivalent. Kaspar ___ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
[riot-devel] Discussion of Power Management
Dear everyone,
I've worked on the power management for the atmega platform at the RIOT
Hackathon.
While creating PRs for the different parts, I stopped at a critial point
I would like to discuss here:
Several functions are used for the power management and stubs are
provided in drivers/periph_common/pm.c. However as mentioned in one
issue, the weak implementation did *not* work correctly.
Therefore we agreed on removing weak definitions for the module and use
some #ifdefs instead:
#if !defined( MODULE_PM_LAYERED )
void pm_set_lowest(void) { }
#endif
Unfortunately, this does not work because some platforms and especially
the stm32_common/kinetis_common/cortexm_common platforms define these
functions all over the place!
My first question is there for, is there a good system to
implementations for a module such as the PM module at different levels?
E.g. the CPU should always have the possibility to implement specific
pm_* functions. A fallback could be provided in the common CPU.
Additionally, the pm_layered module uses some of these CPU functions.
And finally, the fallback functions in pm.c should be used, if none exist.
Is it ok to use #define / #ifdef constructs for these? Any opinions?
I think we need a very fine-granular system here. I started to add
special features to handle this case:
makefiles/features.inc.mk
DEFAULT_FEATURES += periph_pm periph_pm_off periph_pm_set_lowest
periph_pm_reboot
cpu/cortexm_common/Makefile.features
FEATURES_PROVIDED += periph_pm_set_lowest
I thought that maybe features can be used to describe which functions
are provided. Later on based on include/compile order, missing functions
can be added if needed. Any opinions on this?
Best Regards,
Robert
--
Robert Hartung, M.Sc.
Technische Universität Braunschweig
Institut für Betriebssysteme und Rechnerverbund
Mühlenpfordtstr. 23, Raum 115
38106 Braunschweig
Fon: +49 (531) 391 - 3264
Fax: +49 (531) 391 - 5936
E-Mail: hart...@ibr.cs.tu-bs.de
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
