core|sys: Guard periph_pm calls

[MrKevinWeiss]

Contribution description

It would seem that either we need to require the periph_pm module in shell or make it optional... since we have many other optional modules here and we still may want the RIOT_VERSION command, lets make it optional for now.

Edit: I also added guards in other places that use periph_pm without requiring it. The syscalls in newlib and picolib are probably also a good candidate for having this optional. There are many features withing the *lib that can be used without pm stuff.

Testing procedure

Remove the periph_pm feature from a board and try to examples/saul such as on this PR and you should not get a linking error that looks like:

/opt/gcc-arm-none-eabi-10.3-2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: /data/riotbuild/riotbase/examples/saul/bin/gba_cartridge/shell_cmds/sys.o: in function `_reboot_handler':
/data/riotbuild/riotbase/sys/shell/cmds/sys.c:38: undefined reference to `pm_reboot'
collect2: error: ld returned 1 exit status

Issues/PRs references

discovered while testing #19519

[riot-ci]

Murdock results

✔️ PASSED

b8c4617 sys/*: expose periph/pm.h

Success	Failures	Total	Runtime
10045	0	10045	14m:40s

Artifacts

• Documentation preview

[maribu]

Interesting. Historically, I think all platforms have implemented pm_reboot() without implementing any other pm_<foobar>() and did not provide periph_pm as feature. E.g. AVR was one of these (but now does provide proper periph_pm) and MSP430 still is one of these.

I don't believe the historic way of having one pm_<foobar>() function singled out does not make any sense.

I guess, it would have made sense to have separate features for real power management and reboot capabilities. Also, I'm not sure if the reboot feature should really be pm_reboot(), but maybe rather some cpu_reboot() and grouped elsewhere. The reboot feature can be implemented in a number of ways, and most don't interact with the power management subsystem at all but rather e.g. abuse the watchdog or similar.

tl;dr

I think this is an improvement and should go in. But it is not yet the final solution and will cause regressions e.g. for MSP430.

[MrKevinWeiss]

Hmm, I updated it a bit more after discovering more unguarded calls.

But it is not yet the final solution and will cause regressions e.g. for MSP430.

I will look into that... I don't quite understand the issue with the MSP430.

[MrKevinWeiss]

Also, I'm not sure if the reboot feature should really be pm_reboot(), but maybe rather some cpu_reboot() and grouped elsewhere.

Let's first fix the current system then worry about this later... Makes sense though.

[MrKevinWeiss]

I don't think this will change the MSP430 behavior as the whole CPU provides periph_pm, thus the guards won't make any difference or change.

[MrKevinWeiss]

Anyone interested? ping @Teufelchen1 or maybe @maribu

[benpicco]

Why do you need to guard the header?

[MrKevinWeiss]

Why would we include a header that we shouldn't use?

Other places also guard headers and I think it is someone cleaner is all (I think we always include the periph headers anyways).

[benpicco]

It adds visual clutter and the header is always available.

[MrKevinWeiss]

removed

[maribu]

The coding convention btw. clearly says that @MrKevinWeiss original code is mandatory: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#includes

However, the lived practice in RIOT's code base is to not add the guards.

[MrKevinWeiss]

Hmmm I have 2 acks now... Maybe @Teufelchen1 can reACK?

[MrKevinWeiss]

Or maybe it just needs a nudge