OS timer uses LPTICKER by default, then USTICKER #12988

jeromecoutant · 2020-05-18T09:04:05Z

Summary of changes

Discussion started in #12663 (comment)

Idea is to use lpticker by preference if available, else usticker.

@kjbracey-arm
@LMESTM

Impact of changes

Avoid to define "tickless-from-us-ticker" for each target that doesn't support LPTICKER

Migration actions required

Documentation

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

kjbracey

Seems like a good idea to me. No strong feelings on the error.

kjbracey · 2020-05-18T09:28:12Z

platform/source/mbed_os_timer.cpp

 #else
-        MBED_ASSERT("OS timer not available - check MBED_CONF_TARGET_TICKLESS_FROM_US_TICKER" && false);
-        return NULL;
+        error("OS timer not available - check MBED_CONF_TARGET_TICKLESS_FROM_US_TICKER");


I think this should probably be MBED_ERROR these days, as unwieldy as it is. I suppose it makes sense to not be an assert though - this path can't produce a functioning image in any way, and it will be clearer than just a null pointer exception.

Advantage if this change is to see the error message in the console!

looks like error is still valid, although would assume adding a new error would use MBED_ERROR

kjbracey · 2020-05-18T11:42:02Z

I did a search for the config option, and there's one more place to change - I can see a compile time error here that would need to be removed:

mbed-os/rtos/source/TARGET_CORTEX/mbed_rtx_idle.cpp

Line 47 in e345542

#if !MBED_CONF_TARGET_TICKLESS_FROM_US_TICKER && !DEVICE_LPTICKER

The other error there would still be valid.

Not sure why you don't see that in testing. Non-RTOS build?

jeromecoutant · 2020-05-18T12:04:46Z

Issue comes with targets which don't support LPTICKER in baremetal.

kjbracey · 2020-05-18T12:11:29Z

Issue comes with targets which don't support LPTICKER in baremetal.

And that don't have MBED_TICKLESS set?

Okay, I think I grasp the combinations now. That timer stuff is used either by RTOS builds with MBED_TICKLESS, or by bare metal builds if you use RTOS API timing functions.

Bare metal is inherently "tickless" - and that's why you see it tripping up without MBED_TICKLESS being set. That only controls the RTOS choice, and there is no bare metal choice.

In the RTOS case, that stuff needs to work, and not having it correct may as well be a compile error, which is what we see in mbed_rtx_idle. In the bare metal case, it should be fine up until you use a timing function, so the run-time error in os_timer_init seems reasonable.

kjbracey · 2020-05-18T12:23:52Z

Thinking back, I seem to recall there was a school of thought that tickless-from-us-ticker should be explicit as it means lack of deep sleep, and tickless was meant to imply deep sleep. It should be the exception rather than the norm - people shouldn't be setting MBED_TICKLESS if they didn't have the lpticker and couldn't deep sleep.

That error I suggested be removed is enforcing that.

Don't think I buy the argument (if I'm recalling it correctly). Tickless RTOS without deep sleep is still a power optimisation, if a more minor one. No need to wake 1000 times a second.

jeromecoutant · 2020-05-18T12:28:56Z

What about a target without LPTICKER (so no TICKLESS)
Withh both supports: RTOS and non-RTOS mode ?

The following PR removes the need to override it: ARMmbed/mbed-os#12988 Signed-off-by: Hugues Kamba <[email protected]>

kjbracey · 2020-05-18T12:43:24Z

What about a target without LPTICKER (so no TICKLESS) Withh both supports: RTOS and non-RTOS mode ?

Not sure what you're asking.

This PR as it stands makes the bare-metal mode work automatically regardless of the tickless-from-us-ticker setting, and the RTOS non-tickless mode avoids the #error check.

If we want the RTOS tickless to not be automatic, leave the #error. If we want the RTOS tickless to be automatic like the bare metal, remove the error.

I think some of the flagging may not make total sense at this point. Originally MBED_TICKLESS required lpticker, and had to be set explicitly to state "we know lpticker works well enough for tickless to be viable on this target".

It might have made more sense for MBED_TICKLESS and tickless-from-us-ticker to be the defaults, and you turn tickless-from-us-ticker off on a target when you know lp-ticker is good enough for RTOS timing.

(I believe tickless-from-us-ticker should perform fine on all platforms).

It's possible the bare-metal is being excessively optimistic, compared to the RTOS, by wanting to use lpticker "by default". There's been a mixture of "use lpticker by default if available" and "use lpticker only if explicitly enabled" in various components in the past.

The "only if explictly enabled" was usually due to (a) latency compensation worries (which we can deal with) or (b) unhappiness at being regularly reprogrammed 1/1000 times a second. Neither of which is necessarily a problem for bare-metal use.

hugueskamba · 2020-05-18T12:46:22Z

platform/source/mbed_os_timer.cpp

 #else
-        MBED_ASSERT("OS timer not available - check MBED_CONF_TARGET_TICKLESS_FROM_US_TICKER" && false);
-        return NULL;
+        error("OS timer not available - check MBED_CONF_TARGET_TICKLESS_FROM_US_TICKER");
 #endif
        //os_timer->setup_irq();


Can we also remove this commented out code?

0xc0170 · 2020-05-18T13:28:24Z

Me as well but looking at the goal in this PR, it wants to remove this "thought".

Only my suggested #error removal (which is inside an #ifdef MBED_TICKLESS in the RTOS) does that. Without that the code Jerome is changing is only reached in bare-metal, so there's no change for MBED_TICKLESS.

Should this PR do this, @jeromecoutant what do you think?

I'm not sure I'd advocate that now. Flipping default state of either option is likely to confuse apps - it's the sort of thing people do mess with trying to get platforms to work.

I'd limit the adjustment to removing that #error so that MBED_TICKLESS RTOS aligns with the "automatic use of us-ticker if no lp-ticker" change being done for bare-metal. Keep them consistent. If people think it's reasonable to let people turn on MBED_TICKLESS easily for lp-ticker-less platforms.

jeromecoutant · 2020-05-18T13:53:05Z

Should this PR do this, @jeromecoutant what do you think?

I think I will close my PR when I see Kevin's one :-)

kjbracey · 2020-05-18T13:56:02Z

I think I will close my PR when I see Kevin's one :-)

Not planning one - I don't think significant changes to the way the whole thing works at this point are a good idea. Just that #error could be removed, if others agree.

Pull request has been modified.

0xc0170 · 2020-05-19T08:53:54Z

@jeromecoutant Changes look good. Can you add a reason for this change - details to the commit message? to cover what was discussed here?

This PR makes the bare-metal mode work automatically regardless of the tickless-from-us-ticker setting - in non RTOS mode - and in RTOS tickless mode

Pull request has been modified.

jeromecoutant · 2020-05-19T10:36:03Z

Done

jeromecoutant · 2020-05-22T08:16:15Z

Could we start CI ?

0xc0170 · 2020-05-25T08:49:55Z

I'll start running CI soon for all PRs ready for CI

0xc0170 · 2020-05-25T11:02:12Z

CI started

mbed-ci · 2020-05-25T13:38:14Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

0xc0170

already approved by others as well

mergify · 2020-06-04T13:18:05Z

This PR does not contain release version label after merging.

The following PR removes the need to override it: ARMmbed/mbed-os#12988 Signed-off-by: Hugues Kamba <[email protected]>

kjbracey previously approved these changes May 18, 2020

View reviewed changes

mergify bot added the needs: CI label May 18, 2020

hugueskamba added a commit to ARMmbed/mbed-os-5-docs that referenced this pull request May 18, 2020

Remove the instruction to override "tickless-from-us-ticker"

3a9b79a

The following PR removes the need to override it: ARMmbed/mbed-os#12988 Signed-off-by: Hugues Kamba <[email protected]>

hugueskamba mentioned this pull request May 18, 2020

Remove the instruction to override "tickless-from-us-ticker" ARMmbed/mbed-os-5-docs#1319

Merged

hugueskamba reviewed May 18, 2020

View reviewed changes

jeromecoutant force-pushed the PR_OS_TIMER branch from 308a236 to 42466f0 Compare May 18, 2020 16:23

jeromecoutant force-pushed the PR_OS_TIMER branch from 42466f0 to bc5fa43 Compare May 18, 2020 16:27

kjbracey previously approved these changes May 19, 2020

View reviewed changes

evedon previously approved these changes May 19, 2020

View reviewed changes

OS timer uses LPTICKER by default, then USTICKER

a7c9601

This PR makes the bare-metal mode work automatically regardless of the tickless-from-us-ticker setting - in non RTOS mode - and in RTOS tickless mode

jeromecoutant force-pushed the PR_OS_TIMER branch from bc5fa43 to a7c9601 Compare May 19, 2020 10:34

jeromecoutant mentioned this pull request May 20, 2020

STM32 baremetal support step1 (F0/F1/F3/H7/L0) #12992

Merged

0xc0170 added ready for merge and removed needs: CI labels Jun 2, 2020

0xc0170 approved these changes Jun 2, 2020

View reviewed changes

kjbracey mentioned this pull request Jun 4, 2020

Documentation update on tickless mode porting guide ARMmbed/mbed-os-5-docs#1337

Merged

0xc0170 added the release-type: patch Indentifies a PR as containing just a patch label Jun 4, 2020

0xc0170 merged commit 7ae621e into ARMmbed:master Jun 4, 2020

mergify bot removed the ready for merge label Jun 4, 2020

mergify bot added the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label Jun 4, 2020

jeromecoutant deleted the PR_OS_TIMER branch June 5, 2020 14:53

adbridge added release-version: 6.1.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 24, 2020

0xc0170 removed the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label Aug 23, 2021

jeromecoutant mentioned this pull request Jan 3, 2022

Crashes on F030 Nucleo board ARMmbed/mbed-os-example-blinky-baremetal#65

Closed

OS timer uses LPTICKER by default, then USTICKER #12988

OS timer uses LPTICKER by default, then USTICKER #12988

Uh oh!

Conversation

jeromecoutant commented May 18, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

kjbracey May 18, 2020

Choose a reason for hiding this comment

Uh oh!

jeromecoutant May 18, 2020

Choose a reason for hiding this comment

Uh oh!

0xc0170 May 18, 2020

Choose a reason for hiding this comment

Uh oh!

kjbracey commented May 18, 2020

Uh oh!

jeromecoutant commented May 18, 2020

Uh oh!

kjbracey commented May 18, 2020

Uh oh!

kjbracey commented May 18, 2020

Uh oh!

jeromecoutant commented May 18, 2020

Uh oh!

kjbracey commented May 18, 2020

Uh oh!

hugueskamba May 18, 2020

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented May 18, 2020 • edited by kjbracey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromecoutant commented May 18, 2020

Uh oh!

kjbracey commented May 18, 2020

Uh oh!

0xc0170 commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromecoutant commented May 19, 2020

Uh oh!

jeromecoutant commented May 22, 2020

Uh oh!

0xc0170 commented May 25, 2020

Uh oh!

0xc0170 commented May 25, 2020

Uh oh!

mbed-ci commented May 25, 2020

Test run: SUCCESS

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jun 4, 2020

Uh oh!

Uh oh!

0xc0170 commented May 18, 2020 •

edited by kjbracey

Loading

0xc0170 commented May 19, 2020 •

edited

Loading