Optimise mbed_ticker_api.c #12897

kjbracey · 2020-04-30T08:21:31Z

Summary of changes

The generic code in mbed_ticker_api.c uses run-time polymorphism to handle different tickers, and has generic run-time calculations for different ticker widths and frequencies, with a single special-case for 1MHz.

Extend the run-time special casing to handle any conversion cases where either the multiply or divide can be done as a shift. This is a speed optimisation for certain platforms.

Add a new option target.custom-tickers. If turned off, it promises that only USTICKER and LPTICKER devices will be used. This then permits elimination and/or simplification of runtime calculations, saving size and speed. If either both USTICKER and LPTICKER have the same width, or the same period numerator or denominator, or only one of them exists, then operations can be hard-coded. This is a significant ROM space saving, and a minor speed and RAM saving.

We get to optimise all the calculations, but the run-time polymorphism is retained even if there is only one ticker, as it doesn't significantly affect code size versus direct calls, and the existence of
lp_ticker_wrapper and various us_ticker optimisations requires it, even if only LPTICKER is available.

Impact of changes

Targets should make an effort to define the new LP_TICKER_XXX defines to help the optimisation kick in.

Migration actions required

None

Documentation

TBD

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] 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

We'll also need a test that exercises the compile-time optimisation - I think building the common_tickers test with target.custom-tickers overridden to false should suffice.

Reviewers

jeromecoutant · 2020-04-30T08:41:06Z

* Targets should make an effort to define the new `LP_TICKER_XXX` defines to help the optimisation kick in.

Do you have an example ?

kjbracey · 2020-04-30T08:42:52Z

Do you have an example ?

See the comments in lp_ticker_api.h. I'll probably do at least one target in this PR myself, as I did for the US_TICKER ones.

Looking at the STM ones, it's a bit fiddly. One could make it easier by not attempting to get the fraction maximally simplified. That is not a requirement - you're allowed to just say "1000000/32768", you're not required to do "15625/512". More simplification should lead to more speed from early termination of multiply/divide, but it's sometimes hard to arrange with just the C preprocessor.

I'm just about to tweak it to optimise the cases where just the numerator or denominator matches between USTICKER and LPTICKER. That would be another reason to consider not fully-simplifying each of them, for some platforms maybe.

ciarmcom · 2020-04-30T09:00:35Z

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-test @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

jeromecoutant · 2020-04-30T09:20:52Z

Could it fix #12880 ?

kjbracey · 2020-04-30T09:23:10Z

Could it fix #12880 ?

I don't think so - there was already a fast path for the 1MHz case which it would have been going through on STM targets. Any non-C-library saving needs to be made in HAL_GetTick - I've made a suggestion in a comment on that PR.

0xc0170 · 2020-05-08T08:53:03Z

CI started

mbed-ci · 2020-05-08T10:10:40Z

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARM

kjbracey · 2020-05-11T06:49:48Z

CI started

mbed-ci · 2020-05-11T09:05:47Z

Test run: SUCCESS

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

adbridge · 2020-06-11T11:40:37Z

@evedon @donatieng I think we need someone from either Hal or Core to review this ?

kjbracey · 2020-10-23T08:42:54Z

Does it seem likely the greentea network test was a real failure?

0xc0170 · 2020-11-12T09:25:00Z

CI restarted

mbed-ci · 2020-11-12T18:53:23Z

Jenkins CI Test : ❌ FAILED

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

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	❌
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	❌

mbed-ci · 2020-11-18T12:32:59Z

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2020-11-18T13:50:31Z

@MarceloSalazar Please review

jeromecoutant · 2020-12-03T16:55:39Z

Hi

Note that with this PR, hal-tests-tests-mbed_hal-common_tickers becomes FAIL with NUCLEO_F072RB:

| NUCLEO_F072RB-ARMC6 | NUCLEO_F072RB | hal-tests-tests-mbed_hal-common_tickers | lp ticker speed test                               | 0      | 1      | FAIL   | 0.16               |

SibertEnovates · 2021-03-29T12:49:05Z

This change seems to break UART on my STM32F030-based board (which to my knowledge doesn't have lpticker). Whenever I use a ticker, the UART stops properly receiving data. When I disable the ticker, UART works fine. I'm fairly certain ticker is the culprit, as the issue is not present in mbed 6.5 or lower and starts showing from mbed 6.6 and higher.
What could be causing this? What info do you need to look into this?

jeromecoutant · 2021-03-29T13:19:05Z

@donatieng please check

kjbracey · 2021-03-29T14:07:33Z

I'm fairly certain ticker is the culprit, as the issue is not present in mbed 6.5 or lower and starts showing from mbed 6.6 and higher.

Do you have anything more specific than that? I'm can't immediately see a connection between this particular PR and the UART. Have you tried specifically reverting this PR, or doing a bisect?

When you say "stops properly receiving data", are you talking occasional character drops, or severe/total loss?

You say "ticker" - do you mean a C++ Ticker object? What about if you just use a Timer - a bit simpler.

What happened about @jeromecoutant's reported lp ticker failure? Still failing? Any investigation?

SibertEnovates · 2021-03-29T14:18:47Z

Do you have anything more specific than that? I'm can't immediately see a connection between this particular PR and the UART. Have you tried specifically reverting this PR, or doing a bisect?

I'm going to check out how I can do this and report back.

When you say "stops properly receiving data", are you talking occasional character drops, or severe/total loss?

The loss is severe enough that I can't properly receive my frames (about 40 bytes) most of the time, breaking proper functionality of my applictation.

You say "ticker" - do you mean a C++ Ticker object? What about if you just use a Timer - a bit simpler.

Yes, using a C++ ticker object. I've replaced the ticker with a call_every on the event queue, which also resolved the issue but we would like to know the root cause of this issue.

SibertEnovates · 2021-03-29T14:38:01Z

I've reverted the 4 commits in this PR. This seems to remove the issue and thus UART works fine. I'd assume the issue is somewhere in this PR.

kjbracey · 2021-03-30T12:54:46Z

This is likely an interrupt latency issue, but if anything this PR should have decreased latency problems. Calculations are supposed to be simpler.

Please create a proper issue to track this.

Provide toolchain info there - also I'm interested to check if you're using microlib.

mergify bot added the needs: work label Apr 30, 2020

kjbracey force-pushed the tickeropt branch from ee0065e to d1cbeb0 Compare April 30, 2020 08:32

ciarmcom requested review from a team April 30, 2020 09:00

ciarmcom added the needs: review label Apr 30, 2020

mergify bot removed the needs: review label Apr 30, 2020

kjbracey force-pushed the tickeropt branch from d1cbeb0 to 516b12a Compare April 30, 2020 09:20

kjbracey force-pushed the tickeropt branch 3 times, most recently from 116ca5f to ac06508 Compare May 4, 2020 08:58

kjbracey mentioned this pull request May 6, 2020

Optimise HAL_GetTick API #12915

Merged

0xc0170 added needs: CI and removed needs: work labels May 7, 2020

mergify bot added needs: work and removed needs: CI labels May 8, 2020

kjbracey force-pushed the tickeropt branch from ac06508 to fcaabda Compare May 8, 2020 12:20

kjbracey added needs: review and removed needs: work labels May 27, 2020

adbridge added the release-type: feature label Jun 11, 2020

mergify bot removed the needs: review label Oct 15, 2020

0xc0170 added needs: CI and removed needs: work labels Nov 12, 2020

0xc0170 added needs: review and removed needs: CI labels Nov 18, 2020

ithinuel approved these changes Nov 26, 2020

View reviewed changes

0xc0170 approved these changes Nov 30, 2020

View reviewed changes

0xc0170 merged commit 4e0d07d into ARMmbed:master Nov 30, 2020

0xc0170 removed the needs: review label Nov 30, 2020

mbedmain added release-version: 6.6.0 Release-pending and removed release-type: feature Release-pending labels Dec 11, 2020

kjbracey deleted the tickeropt branch March 30, 2021 12:54

SibertEnovates mentioned this pull request Mar 30, 2021

Changes in Ticker cause UART to fail #14489

Closed

This was referenced Apr 12, 2021

Make ticker computation use shift-by-0 #14532

Merged

Timer.read_us takes 8.5µs, making wait_us unable to wait for less than that on Nucleo F103RB #7397

Closed

jeromecoutant mentioned this pull request Apr 13, 2021

STM32: Add LP_TICKER and US_TICKER optimisation macro #14536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise mbed_ticker_api.c #12897

Optimise mbed_ticker_api.c #12897

kjbracey commented Apr 30, 2020 •

edited

Loading

jeromecoutant commented Apr 30, 2020

kjbracey commented Apr 30, 2020 •

edited

Loading

ciarmcom commented Apr 30, 2020

jeromecoutant commented Apr 30, 2020

kjbracey commented Apr 30, 2020

0xc0170 commented May 8, 2020

mbed-ci commented May 8, 2020

kjbracey commented May 11, 2020

mbed-ci commented May 11, 2020

adbridge commented Jun 11, 2020

kjbracey commented Oct 23, 2020

0xc0170 commented Nov 12, 2020

mbed-ci commented Nov 12, 2020

mbed-ci commented Nov 18, 2020

0xc0170 commented Nov 18, 2020

jeromecoutant commented Dec 3, 2020

SibertEnovates commented Mar 29, 2021

jeromecoutant commented Mar 29, 2021

kjbracey commented Mar 29, 2021

SibertEnovates commented Mar 29, 2021

SibertEnovates commented Mar 29, 2021 •

edited

Loading

kjbracey commented Mar 30, 2021

Optimise mbed_ticker_api.c #12897

Optimise mbed_ticker_api.c #12897

Conversation

kjbracey commented Apr 30, 2020 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

jeromecoutant commented Apr 30, 2020

kjbracey commented Apr 30, 2020 • edited Loading

ciarmcom commented Apr 30, 2020

jeromecoutant commented Apr 30, 2020

kjbracey commented Apr 30, 2020

0xc0170 commented May 8, 2020

mbed-ci commented May 8, 2020

Test run: FAILED

kjbracey commented May 11, 2020

mbed-ci commented May 11, 2020

Test run: SUCCESS

adbridge commented Jun 11, 2020

kjbracey commented Oct 23, 2020

0xc0170 commented Nov 12, 2020

mbed-ci commented Nov 12, 2020

Jenkins CI Test : ❌ FAILED

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

mbed-ci commented Nov 18, 2020

Jenkins CI Test : ✔️ SUCCESS

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

0xc0170 commented Nov 18, 2020

jeromecoutant commented Dec 3, 2020

SibertEnovates commented Mar 29, 2021

jeromecoutant commented Mar 29, 2021

kjbracey commented Mar 29, 2021

SibertEnovates commented Mar 29, 2021

SibertEnovates commented Mar 29, 2021 • edited Loading

kjbracey commented Mar 30, 2021

kjbracey commented Apr 30, 2020 •

edited

Loading

kjbracey commented Apr 30, 2020 •

edited

Loading

SibertEnovates commented Mar 29, 2021 •

edited

Loading