Power management stat : add verbosity level for MBED_SLEEP_TRACING_ENABLED #14610

jeromecoutant · 2021-04-30T12:42:24Z

Summary of changes

Using MBED_SLEEP_TRACING_ENABLED is not reliable with STM32 targets.

See discussions in #11497 #14580 ...

[edit]
~~This new MBED_SLEEP_STAT_ENABLED macro is the same one~~
~~removing prints at each sleep_manager_lock/unlock_deep_sleep~~

deepsleep stats can be enabled now with json config
default configuration is not changed (full verbosity adding a console line for each lock/unlock command)
except for STM32 targets, verbosity is reduced by default

Impact of changes

Migration actions required

Documentation

STM32 readme file is updated

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/team-st-mcd

ciarmcom · 2021-04-30T13:00:20Z

@jeromecoutant, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2021-04-30T14:46:05Z

platform/include/platform/mbed_power_mgmt.h

@@ -90,7 +90,7 @@ extern "C" {
 *
 */

-#ifdef MBED_SLEEP_TRACING_ENABLED
+#if defined(MBED_SLEEP_TRACING_ENABLED) || defined(MBED_SLEEP_STAT_ENABLED)


I need to read the discussion but still this one commit does not explain one why there would be a need for additional macro (there is || between so one of them needs t obe truth at least,why ? ).

Because ST LPTICKER API calls the deepsleep lock function very very often:
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/lp_ticker.c#L471

So I let the current macro as it is and as it is described in mbed-os doc,
and created a new one, a bit less verbose.

0xc0170 · 2021-04-30T14:46:37Z

targets/TARGET_STM/README.md

+- deepsleep can also be disabled by application or drivers using sleep_manager_lock_deep_sleep()
+
+To enable the tracing, it is recommended to **not** define MBED_SLEEP_TRACING_ENABLED macro,
+but **MBED_SLEEP_STAT_ENABLED** macro.


To my question above, why is not recommended/ Shoudn't the one be fixed rather than create two (it might confuse people) ?

Add here some details why stats only.

ciarmcom · 2021-04-30T15:00:20Z

@jeromecoutant, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2021-05-03T13:42:09Z

Because ST LPTICKER API calls the deepsleep lock function very very often:

#11497 (comment) - if this is required to be so, locking so often, then printing is correct. The problem is how to filter these as there are too many locks?

#14580 - how is this related to the issue?

We will need to review this in detail. I don't believe adding a new macro is an answer. I find it confusing which one I should define and why.

jeromecoutant · 2021-05-03T14:55:19Z

#14580 - how is this related to the issue?

Not directly related to this issue. This issue confirms that we need something to be able to trace sleep lock with STM32 use case.

We will need to review this in detail. I don't believe adding a new macro is an answer. I find it confusing which one I should define and why.

The default macro is not usable for STM32.

ciarmcom · 2021-05-10T15:50:20Z

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please complete review of the changes to move the PR forward. Thank you for your contributions.

0xc0170

MBED_SLEEP_STATS_ENABLED could it be named similarly as the functions in the power mgmt

Not against introducing the stats only. My understanding is tracing in this case for stm32 is way to verbose (happens very frequently) and thus can't be used. Stats however are useful information and can be printed. What a user should do if they need tracing (are there any options for them) ?

Please review @ARMmbed/mbed-os-core review

0xc0170 · 2021-05-18T09:23:27Z

platform/source/mbed_power_mgmt.c

@@ -239,6 +243,9 @@ void sleep_manager_sleep_auto(void)
 #endif
 hal_deepsleep();
 } else {
+#if defined(MBED_SLEEP_STAT_ENABLED)


you get stats at the top of this function, do we need to print this again when only sleeping?

0xc0170 · 2021-05-18T09:27:44Z

targets/TARGET_STM/README.md

+- deepsleep can also be disabled by application or drivers using sleep_manager_lock_deep_sleep()
+
+To enable the tracing, it is recommended to **not** define MBED_SLEEP_TRACING_ENABLED macro,
+but **MBED_SLEEP_STAT_ENABLED** macro.


Add here some details why stats only.

ciarmcom · 2021-05-21T11:50:37Z

This pull request has automatically been marked as stale because it has had no recent activity. , please complete review of the changes to move the PR forward. Thank you for your contributions.

ciarmcom · 2021-06-03T15:50:47Z

This pull request has automatically been marked as stale because it has had no recent activity. @jeromecoutant, please carry out any necessary work to get the changes merged. Thank you for your contributions.

jeromecoutant · 2021-10-28T16:10:58Z

ping :-)

0xc0170

The printf there with allo does not provide sufficient meaning to a reader. What should be printed?

0xc0170 · 2021-10-15T14:01:15Z

platform/source/mbed_power_mgmt.c

+ }
+
+ if (sleep_stats[i].identifier[0] == '\0') {
+ mbed_error_printf("allo");


Why is it allo ? Should this be something more meaningful ?

I reviewed this but kept the review pending (not certain why it was not posted).

…ABLED Full verbosity is adding a console line for each lock/unlock API call - stats can be enabled with json config - default configuration is full verbosity and add a console line for each lock/unlock command - for STM32 targets, verbosity is reduced by default

Pull request has been modified.

mbed-ci · 2021-11-08T10:49:12Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2021-11-08T13:31:42Z

@LDong-Arm Can you please review?

LDong-Arm

LGTM as per previous discussion regarding the documentation of the new config.

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Apr 30, 2021

ciarmcom requested a review from a team April 30, 2021 13:00

ciarmcom added needs: review devices: st component: core labels Apr 30, 2021

0xc0170 reviewed Apr 30, 2021

View reviewed changes

ciarmcom requested a review from a team April 30, 2021 15:00

ciarmcom added the stale Stale Pull Request label May 10, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 14, 2021

jeromecoutant mentioned this pull request May 17, 2021

STM32: add information in README for SLEEP feature #14670

Merged

0xc0170 requested a review from a team May 18, 2021 09:18

0xc0170 removed the stale Stale Pull Request label May 18, 2021

0xc0170 reviewed May 18, 2021

View reviewed changes

ciarmcom added the stale Stale Pull Request label May 21, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 21, 2021

adbridge added needs: work and removed needs: review labels May 24, 2021

ciarmcom removed the stale Stale Pull Request label May 24, 2021

ciarmcom added the stale Stale Pull Request label Jun 3, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 4, 2021

0xc0170 requested review from LDong-Arm and 0xc0170 October 15, 2021 08:24

0xc0170 previously requested changes Nov 3, 2021

View reviewed changes

mergify bot added needs: work and removed needs: review labels Nov 3, 2021

jeromecoutant added 2 commits November 8, 2021 09:46

STM32: readme update for MBED_SLEEP_TRACING_ENABLED

8e26a05

jeromecoutant force-pushed the DEV_SLEEP_TRACE branch from 426c57c to 8e26a05 Compare November 8, 2021 08:47

jeromecoutant requested a review from 0xc0170 November 8, 2021 08:47

0xc0170 added needs: review and removed needs: work labels Nov 8, 2021

0xc0170 approved these changes Nov 8, 2021

View reviewed changes

Mbed Core automation moved this from Review in progress (4) to Reviewer approved & awaiting CI & merge (3) Nov 8, 2021

mergify bot added needs: CI and removed needs: review labels Nov 8, 2021

mergify bot added ready for merge and removed needs: CI labels Nov 8, 2021

LDong-Arm approved these changes Nov 8, 2021

View reviewed changes

0xc0170 merged commit d4c6b37 into ARMmbed:master Nov 8, 2021

Mbed Core automation moved this from Reviewer approved & awaiting CI & merge (3) to Done Nov 8, 2021

mergify bot removed the ready for merge label Nov 8, 2021

jeromecoutant deleted the DEV_SLEEP_TRACE branch November 8, 2021 16:09

mbedmain added release-version: 6.15.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Power management stat : add verbosity level for MBED_SLEEP_TRACING_ENABLED #14610

Power management stat : add verbosity level for MBED_SLEEP_TRACING_ENABLED #14610

jeromecoutant commented Apr 30, 2021 •

edited

Loading

ciarmcom commented Apr 30, 2021

0xc0170 Apr 30, 2021

jeromecoutant Apr 30, 2021 •

edited

Loading

0xc0170 Apr 30, 2021

0xc0170 May 18, 2021

ciarmcom commented Apr 30, 2021

0xc0170 commented May 3, 2021

jeromecoutant commented May 3, 2021

ciarmcom commented May 10, 2021

0xc0170 left a comment •

edited

Loading

0xc0170 May 18, 2021

0xc0170 May 18, 2021

ciarmcom commented May 21, 2021

ciarmcom commented Jun 3, 2021

jeromecoutant commented Oct 28, 2021

0xc0170 left a comment

0xc0170 Oct 15, 2021

0xc0170 Nov 3, 2021

jeromecoutant Nov 8, 2021

mbed-ci commented Nov 8, 2021

0xc0170 commented Nov 8, 2021

LDong-Arm left a comment

Power management stat : add verbosity level for MBED_SLEEP_TRACING_ENABLED #14610

Power management stat : add verbosity level for MBED_SLEEP_TRACING_ENABLED #14610

Conversation

jeromecoutant commented Apr 30, 2021 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Apr 30, 2021

Choose a reason for hiding this comment

jeromecoutant Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciarmcom commented Apr 30, 2021

0xc0170 commented May 3, 2021

jeromecoutant commented May 3, 2021

ciarmcom commented May 10, 2021

0xc0170 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciarmcom commented May 21, 2021

ciarmcom commented Jun 3, 2021

jeromecoutant commented Oct 28, 2021

0xc0170 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbed-ci commented Nov 8, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

0xc0170 commented Nov 8, 2021

LDong-Arm left a comment

Choose a reason for hiding this comment

jeromecoutant commented Apr 30, 2021 •

edited

Loading

jeromecoutant Apr 30, 2021 •

edited

Loading

0xc0170 left a comment •

edited

Loading