New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

equeue: add config option to use different timer classes #5328

Merged
merged 3 commits into from Nov 9, 2017

Conversation

Projects
None yet
7 participants
@MikeDK
Contributor

MikeDK commented Oct 17, 2017

…use LowPowerTimer, LowPowerTimeout and LowPowerTicker instead of Timer/Timeout/Ticker.

This way, on SiLabs boards the low power sleep states will be used when using event queue.

equeue: added config option which tells equeue_mbed.cpp if it shall u…
…se LowPowerTimer, LowPowerTimeout and LowPowerTicker instead of Timer/Timeout/Ticker.

This way, on SiLabs boards the low power sleep states will be used when using event queue.
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 17, 2017

This way, on SiLabs boards the low power sleep states will be used when using event queue.

Isn't this an issue about queue clean-up (destructor/destroy function) ? I glanced at the implementation, equeue_tick creates timer objects, but destroy does not destroy them, thus they are still active and blocking deep sleep?

cc @geky @kjbracey-arm

@pan-

This comment has been minimized.

Member

pan- commented Oct 17, 2017

@0xc0170 Sleep state which can be entered may also be managed by the MCU itself depending on the state of its peripherals. Outside sleep consideration, using low power timing primitives should help to reduce the power consumption footprint.

I've looked at the implementation and you're correct, the Timer and Ticker allocated dynamically allocated are not released. Thankfully they are allocated once and will be used by all event queue instantiated for the rest of the application lifetime.

@@ -21,6 +21,7 @@
"shared-highprio-eventsize": {
"help": "Event buffer size (bytes) for shared high-priority event queue",
"value": 256
}
},
"use-lowpower-timer-ticker": 0

This comment has been minimized.

@geky

geky Oct 17, 2017

Member

Could you expand this to have a "help" field as well. Something like:
"Enable use of low power timer and ticker classes. May reduce the accuracy of the event queue."

@geky

This comment has been minimized.

Member

geky commented Oct 17, 2017

This looks perfect 👍

Though quick question, what is the accuracy of the low power tickers? Are they less accurate than 1ms? It doesn't matter if this is added as a config option, but if it's more accurate than 1ms it may be worthwhile to use low power by default if it's available.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 18, 2017

If there's a "low power X", it must presumably have a downside compared to the default one, so there's some trade-off. If there is, not sure how this choice can be made globally. Wouldn't it have to be per-event-queue?

If it is acceptable to be the default for all event queues, why not for the entire system? Is it just an assumption that there are fewer event queue users, so switching all event queues at one once is less likely to break stuff than switching everything?

Afraid I'm not familiar with these LowPowerXXX classes - pointer welcome.

@MikeDK

This comment has been minimized.

Contributor

MikeDK commented Oct 18, 2017

@geky I updated to include a help text ... I took yours, as this seems accurate ;)

@kjbracey-arm When I do a grep DEVICE_LOWPOWERTIMER over all sources, I can see that several targets implement a lp_ticker.c file - these are:

  • TARGET_ONSEMI/TARGET_NCS36510
  • TARGET_STM
  • TARGET_Silicon_Labs/TARGET_EFM32
  • TARGET_NORDIC/TARGET_NRF5
  • TARGET_ARM_SSG/TARGET_BEETLE
  • TARGET_Freescale/TARGET_MCUXpresso_MCUS
  • TARGET_NUVOTON/TARGET_M480
  • TARGET_NUVOTON/TARGET_M451
  • TARGET_NUVOTON/TARGET_NUC472
  • TARGET_NUVOTON/TARGET_NANO100

So ... a rather small amount of targets actually claim to support this ;)

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 18, 2017

This is a bit vague: https://os.mbed.com/blog/entry/Using-the-new-mbed-power-management-API/

On Silicon Labs’ platforms, even though you’re still setting the timing value in microseconds, the resolution will be along the quarter millisecond line.

It would be nice to have some sort of proper cross-platform specification for that. Looking at STM and some other platforms, I see a fair few use 32.768kHz RTCs, which is 30µs. Maybe we can assume it's at least 1ms, so always suitable?

In which case I kind of feel that rather than being local and based on config, AliasTimer etc could become proper global classes like MillisecondTimer presenting only millisecond resolution, based on LowPowerTimer if DEVICE_LOWPOWERTIMER, else based on Timer. And the event queue could always use MillisecondTimer.

But staring at equeue a bit further, does it really need to use dedicated timers at all (in an RTOS)? Doesn't it just need millisecond precision to read current time (equeue_tick)? In which case it can just get that by reading osKernelGetTickCount. Would make sense as most of its delay is based on a (tick-based) semaphore wait.

And the other ticker is running infrequently to reset the first timer to avoid wrap - unnecessary with appropriate care with the kernel tick count wrap?

@pan-

This comment has been minimized.

Member

pan- commented Oct 18, 2017

I second @kjbracey-arm on using timer and ticker in the OS context. It seems unnecessary and from my perspective it is strange to use different time sources (us_ticker or lp_ticker and the systick clock) and interact with them as if they were equal.

@geky

This comment has been minimized.

Member

geky commented Oct 18, 2017

Ah yep, looks like osKernelGetTickCount fits perfectly in rtos context. It just wasn't around back when we were using cmsis-rtos v1.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 24, 2017

@MikeDK Could you please address the latest comments.

@geky

This comment has been minimized.

Member

geky commented Oct 24, 2017

I can't think of a reason we can't go ahead and merge this in? It's opt-in, the patch for okKernelGetTickCount can come in seperately, and the low-power option is still useful without the rtos.

@0xc0170 0xc0170 added the needs: CLA label Oct 24, 2017

@0xc0170

Coding style change request, the concept looks fine.

@@ -23,27 +23,36 @@
#include <stdbool.h>
#include "mbed.h"
#if MBED_CONF_EVENTS_USE_LOWPOWER_TIMER_TICKER
#define AliasTimer LowPowerTimer

This comment has been minimized.

@0xc0170

0xc0170 Oct 24, 2017

Member

macros should start at the line beginning and should use capital letters , see https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/

#if MBED_xxx
#define ALIAS_TIMER

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 24, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 24, 2017

@MikeDK Can you please sign https://os.mbed.com/contributor_agreement/ ?

I requested some styling changes.
@geky looks fine to me, I believe the rest of the team also liked this proposal to support low power tickers in the event queue.

the patch for okKernelGetTickCount can come in seperately, and the low-power option is still useful without the rtos.

@geky / @kjbracey-arm Can we capture this, to track it?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 25, 2017

And if the RTOS tick count change happens later, then this JSON option can remain but will no longer do anything with an RTOS (as there will be no tickers etc). Seems reasonable to me.

@MikeDK

This comment has been minimized.

Contributor

MikeDK commented Oct 30, 2017

@adbridge I was not sure about whether the comments were directed to me ;) My opinion is also that this change is opt-in, so it won't break anything if not explicitly enabled

@0xc0170 I already signed the agreement, and I just committed the styling changes ;)

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 30, 2017

@0xc0170 are you happy with the changes ?

@geky

geky approved these changes Oct 30, 2017

@MikeDK

This comment has been minimized.

Contributor

MikeDK commented Nov 6, 2017

Question: is there any further action required from my side? I'm not sure, because the bot tells me that there are still changes requested... I committed one change for geky's review, and one for 0xc0170...

@0xc0170

0xc0170 approved these changes Nov 6, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: CLA labels Nov 6, 2017

@0xc0170 0xc0170 removed the needs: work label Nov 6, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 6, 2017

Question: is there any further action required from my side? I'm not sure, because the bot tells me that there are still changes requested... I committed one change for geky's review, and one for 0xc0170...

To run CI here, will trigger the job (we are currently investigating one target failing)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 8, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 8, 2017

Build : SUCCESS

Build number : 465
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5328/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 9, 2017

@alzix Please trigger uvisor CI job for this PR (seems it is not active for us at the moment for some reason, testing right below)

/morph uvisor-test

@0xc0170 0xc0170 changed the title from equeue: added config option which tells equeue_mbed.cpp if it shall … to equeue: add config option to use different timer classes Nov 9, 2017

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Nov 9, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Nov 9, 2017

@MikeDK Could you please read https://chris.beams.io/posts/git-commit/#seven-rules and update this PR accordingly. Thanks

@0xc0170 0xc0170 merged commit 555ae12 into ARMmbed:master Nov 9, 2017

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@0xc0170 0xc0170 removed the ready for merge label Nov 9, 2017

@kjbracey-arm kjbracey-arm referenced this pull request Mar 22, 2018

Merged

Add an option to use LowPowerTimer for poll #6418

1 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment