Skip to content

Conversation

maribu
Copy link
Member

@maribu maribu commented Aug 19, 2025

Contribution description

While the existing documentation already contains all the info in a very dense form, there are still often confusion on what pm_layered does and how to correctly use and integrate with it.

Maybe extending the documentation does help to make things more clear.

Testing procedure

Read the new documentation and compare it to the old one. Things should be easier to understand with the new doc.

Issues/PRs references

None

While the existing documentation already contains all the info in a very
dense form, there are still often confusion on what `pm_layered` does
and how to correctly use and integrate with it.

Maybe extending the documentation does help to make things more clear.
@maribu maribu added Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 19, 2025
@github-actions github-actions bot added Area: sys Area: System and removed Area: doc Area: Documentation labels Aug 19, 2025
@maribu maribu requested a review from crasbe August 19, 2025 20:48
Copy link
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two smal things because I'm really tired.

Also I thought about whether this wouldn't be more at home in a doc.md file.

@riot-ci
Copy link

riot-ci commented Aug 19, 2025

Murdock results

✔️ PASSED

499e734 Update sys/include/pm_layered.h

Success Failures Total Runtime
1 0 1 02m:14s

Artifacts

@derMihai
Copy link
Contributor

Some of the current implementations don't comply with this. I'm not sure whether the scope of these changes is to document the current state, or what it ought to be and fixes will follow up. For example:

 * @ref pm_set must ignore any power mode that does not have full RAM retention,
 * as losing data whenever no (non-idle) thread is runnable is not acceptable.

samd5x may very well enter backup mode through pm_layered and lose RAM contents, save for the backup region.

Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc helped me. But if it is all about clocks, an API like <clock_domain>_acquire() and <clock_domain>_release() calling the pm API would have been more clear to me from the beginning.

@maribu
Copy link
Member Author

maribu commented Aug 20, 2025

But if it is all about clocks, an API like <clock_domain>_acquire() and <clock_domain>_release() calling the pm API would have been more clear to me from the beginning.

I do agree. But there might be an MCU that allows power management not only by enabling different clock domains, but also other power consuming functions such as e.g. DMA access by peripherals (which would then need to be blocked when setting up DMA transfers and unblocked when all expected DMA transfers concluded).

@maribu
Copy link
Member Author

maribu commented Aug 21, 2025

samd5x may very well enter backup mode through pm_layered and lose RAM contents, save for the backup region.

I raised this issue in the Matrix chat to quickly get feedback whether my understanding here is correct or not and got supporting emoji reactions from fellow long term contributors, and nobody disagreed.

But this shows that indeed some more detail in the doc would be helpful.

It would also be nice to have some explicit pm_hibernate() in there for an app to force the system into a super low power mode without RAM retention. But that would require per MCU family documentation what interrupt sources would still be able to wake up the MCU and best practices to figure out if the system booted due to a reset (external reset, watchdog reset, power-on reset, ...) or a wakeup from hibernation and if the latter is the case, how one would identify which wake up signal caused the wake up.

@derMihai
Copy link
Contributor

I raised this issue in the Matrix chat to quickly get feedback whether my understanding here is correct or not and got supporting emoji reactions from fellow long term contributors, and nobody disagreed.

In that case I guess the MCU implementation should also provide pm_block() and pm_unblock(), s.t. higher level stuff (higher than MCU-specific drivers - ztimer comes right off my head) can pm_block(MCU_SPECIFIC_MODE_THAT_WOULD_BREAK_THE_FOLLOWING).

Or, is this supposed to be handled implicitly by acquiring/releasing the MCU-specific driver instance, which in turns blocks that power mode? E.g. ztimer would timer_acquire(MCU_SPECIFIC_TIMER_THAT_KEEPS_ZTIMER_RUNNING).

@maribu
Copy link
Member Author

maribu commented Aug 22, 2025

It is expected the e.g. timer_init() and timer_start() would block whatever power modes are incompatible with the specified timer running an the configured frequency. (But they need to take some care to not double-block a power mode in case the timer was already running before.)

A call to timer_stop() should then decrement the reference counter again.

However, ztimer does have a work around to block at that level power modes, which was faster for getting ztimer in than having to go through the code base and fix all the bugs in there.

Someone(TM) really should go through the periph_timer implementations and fix all the bugs ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants