Add LOWPOWERTIMER for NUCLEO_F401RE and DISCO_F429ZI #3319

jeromecoutant · 2016-11-24T08:34:39Z

Description

Add LOWPOWERTIMER for NUCLEO_F401RE and DISCO_F429ZI

Status

READY

Tests

Tests done on top of #3316

Thx

adbridge · 2016-11-24T10:50:24Z

LGTM 👍

adbridge · 2016-11-24T10:55:04Z

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

adbridge · 2016-11-24T10:55:13Z

/morph test

bridadan · 2016-11-28T03:58:39Z

@adbridge Perhaps we should wait on this PR until #3316 is merged?

0xc0170 · 2016-11-28T14:58:39Z

/morph test

0xc0170 · 2016-11-28T15:00:04Z

@adbridge Perhaps we should wait on this PR until #3316 is merged?

That's refactoring. It should be fine to merge this one in as adding new extension, and refactor will come later. (if these PR are not connect, assuming there are not)

@jeromecoutant happy with this one coming in first, then refactor?

jeromecoutant · 2016-11-28T15:37:57Z

Yes, you can merge. Thx

mbed-bot · 2016-11-28T15:43:05Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1175

All builds and test passed!

bridadan · 2016-11-29T01:22:09Z

The normal tests won't run the low power timer tests. I'll kick off a nightly.

/morph test-nightly

mbed-bot · 2016-11-29T03:32:21Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1176

Test failed!

bridadan · 2016-11-29T05:41:21Z

@jeromecoutant The test tests-mbed_drivers-lp_timeout failed for the NUCLEO_F401RE for the GCC_ARM toolchain. Can you reproduce this failure?

jeromecoutant · 2016-11-30T10:42:30Z

Rebase done.

I agree that tests-mbed_drivers-lp_timeout test is now failed...

We found issue is due to #3213 merge
New patch is available in #3345

Thx

bridadan · 2016-12-01T02:37:33Z

Thanks for the update @jeromecoutant, let's get #3345 merged in before we rerun the bots on this.

0xc0170 · 2016-12-02T14:28:42Z

#3345 merged, lets rerun CI

0xc0170 · 2016-12-02T14:28:52Z

/morph test-nightly

mbed-bot · 2016-12-02T16:18:01Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1193

Test failed!

0xc0170 · 2016-12-05T13:39:18Z

@jeromecoutant Still lp ticker 500us fails , for nucleo 411re ?

Same as reported above:

`@jeromecoutant The test tests-mbed_drivers-lp_timeout failed for the NUCLEO_F401RE for the GCC_ARM toolchain. Can you reproduce this failure?

jeromecoutant · 2016-12-06T09:32:00Z

@0xc0170
In my current branch, test is OK with nucleo 411re... I am going to rebase to the master to check again.
But anyway, you could merge this PR as there is no related to this nucleo 411RE... ?
Thx

adbridge · 2016-12-12T16:45:35Z

@jeromecoutant Could you please retry with GCC version 4.9.3 as we don't currently support v5.x.

bridadan · 2016-12-12T19:23:08Z

If you can reproduce the failure on your machine with that version of the compiler (available here) then I'm curious what about the STM32F4 code is causing this issue (since it works on other targets just fine).

jeromecoutant · 2016-12-14T10:51:47Z

Hi

...
[DEBUG] Compile: C:\Program Files (x86)\GNU Tools ARM Embedded\4.9 2015q3\bin\arm-none-eabi-g++ ...
...

| NUCLEO_F401RE-GCC_ARM | NUCLEO_F401RE | tests-mbed_drivers-lp_timeout | 1ms LowPowerTimeout | 1 | 0 | OK | 0.05 |
| NUCLEO_F401RE-GCC_ARM | NUCLEO_F401RE | tests-mbed_drivers-lp_timeout | 1sec LowPowerTimeout | 1 | 0 | OK | 1.06 |
| NUCLEO_F401RE-GCC_ARM | NUCLEO_F401RE | tests-mbed_drivers-lp_timeout | 1sec LowPowerTimeout from deepsleep | 1 | 0 | OK | 1.07 |
| NUCLEO_F401RE-GCC_ARM | NUCLEO_F401RE | tests-mbed_drivers-lp_timeout | 1sec LowPowerTimeout from sleep | 1 | 0 | OK | 1.07 |
| NUCLEO_F401RE-GCC_ARM | NUCLEO_F401RE | tests-mbed_drivers-lp_timeout | 500us LowPowerTimeout | 1 | 0 | OK | 0.05 |
| NUCLEO_F401RE-GCC_ARM | NUCLEO_F401RE | tests-mbed_drivers-lp_timeout | 5sec LowPowerTimeout | 1 | 0 | OK | 5.07 |

jeromecoutant · 2016-12-14T14:07:41Z

Does Ci use the same compilation options....?

0xc0170 · 2016-12-15T16:08:46Z

Does Ci use the same compilation options....?

Yes, it does. It uses the default profile. We will try to reproduce this locally , will report back

bridadan · 2016-12-15T18:46:41Z

Ok @jeromecoutant I think I know how you can reproduce this.

Download and use the release of GCC from here: https://launchpad.net/gcc-arm-embedded/4.9/4.9-2015-q3-update
Checkout to mbed-os master branch (I tested with commit 899c542)
Merge your branch

Run the following

mbed test -m NUCLEO_F401RE -t GCC_ARM -n tests-mbed_drivers-lp_timeout -DMBED_HEAP_STATS_ENABLED=1 --clean --compile
mbed test -m NUCLEO_F401RE -t GCC_ARM -n tests-mbed_drivers-lp_timeout --run -v

This SHOULD cause the failure we're seeing. The missing bit was the heap stat tracking that we enable in CI (we also enable stack stat tracking with -DMBED_STACK_STATS_ENABLED=1, but I was able to reproduce the problem with just that macro).

If you remove the heap stack tracking, it'll start passing again, which is what you're currently seeing.

FYI @c1728p9

bridadan · 2016-12-19T15:38:04Z

Have you had any luck reproducing this @jeromecoutant? Thanks for your patience.

0xc0170 · 2016-12-21T14:51:41Z

@jeromecoutant Have you been able to reproduce it?

0xc0170 · 2016-12-30T11:39:47Z

@jeromecoutant bump

jeromecoutant · 2017-01-09T09:51:04Z

Rebase and fix applied

Note about the fix:
Issue seems to be in mktime function which is calling at the first time __wrap__free_r

Only when MBED_HEAP_STATS_ENABLED is enabled, __wrap__free_r needs to initialize Singleton. This can be done only when IT are enabled...
rtc_read is called at each ticker setup which call core_util_critical_section_enter, which disables IT...

So to be sure that mktime is called at the first time while IT are enabled, rtc_read is called once once during init.

0xc0170 · 2017-01-09T11:29:58Z

Only when MBED_HEAP_STATS_ENABLED is enabled, __wrap__free_r needs to initialize Singleton. This can be done only when IT are enabled...
rtc_read is called at each ticker setup which call core_util_critical_section_enter, which disables IT...

So to be sure that mktime is called at the first time while IT are enabled, rtc_read is called once once during init.

Can you provide this info to the commit message. To me it's not clear this workaround, why it fails only when heap stats is enabled. Can't this be reproduced during a regular use ?

bridadan · 2017-01-09T16:02:48Z

@jeromecoutant Does this need to be wrapped in #ifdef or can rtc_read just always be called without any adverse effects during the init? It'd be nice to try and reduce the amount of conditional behavior if we can I think.

jeromecoutant · 2017-01-09T16:18:22Z

Goal is to keep the code size unchanged for user, as this patch is only needed for CI test.

bridadan · 2017-01-09T16:40:01Z

@jeromecoutant Ah good point! Can you fix the merge conflict please?

c1728p9 · 2017-01-09T21:36:18Z

targets/TARGET_STM/lp_ticker.c

+    /* __wrap__free_r needs to initialize Singleton not in ISR */
+    /* rtc_read is called at each ticker setup which call core_util_critical_section_enter */
+    /* so rtc_read is called once once before other calls */
+    rtc_read();


If malloc or free get called on first call to rtc_read then this is a problem regardless of whether MBED_HEAP_STATS_ENABLED is set, and could lead to a crash. To make this implementation of lp ticker truly interrupt safe then the call to mktime needs to be removed.

But malloc is not called if MBED_HEAP_STATS_ENABLED is not set...

I am with @c1728p9 . That function call should not be removed if stats are enabled as it has nothing in common with lp ticker or rtc read function. rtc_read is the one that causing the problems, that one should be changed.

@jeromecoutant MBED_HEAP_STATS_ENABLED does not effect the number of calls to free or malloc, it just hooks those functions to record the number of calls.

0xc0170 · 2017-01-12T11:39:09Z

@jeromecoutant Can you remove/replace mktime invocation in the rtc api ?

Add a workaround in lp_ticker_init function for CI tests

jeromecoutant · 2017-01-13T14:42:09Z

Rebase done

0xc0170 · 2017-01-13T16:26:01Z

To replace mktime ?

jeromecoutant · 2017-01-16T12:14:14Z

Hi
How can we replace mktime and still get a UNIX timestamp?
I can see several mktime call in rtc_read function from several platform vendors...

0xc0170 · 2017-02-02T10:30:18Z

Back to this one. Having the code conditionally compiled based on stats flag is not acceptable. The root cause should be found. I do not fully understand why rtc_read() is causing problems and why is it placed in the lp_ticker init. There are platforms that use RTC peripheral for RTC and lp ticker and did not hit this problem.

We shall pull this code and run it locally to see the failure.

adbridge added the needs: CI label Nov 24, 2016

0xc0170 removed the needs: CI label Nov 28, 2016

0xc0170 added the needs: CI label Nov 28, 2016

0xc0170 added needs: work and removed needs: CI labels Nov 29, 2016

jeromecoutant force-pushed the PR_LPT_F4 branch from 21b0cfd to e686100 Compare November 30, 2016 08:19

0xc0170 added needs: CI needs: work and removed needs: work labels Dec 2, 2016

0xc0170 removed the needs: work label Dec 2, 2016

0xc0170 added needs: work and removed needs: CI labels Dec 5, 2016

bridadan mentioned this pull request Dec 13, 2016

NUCLEO_F429ZI Test Failures #3264

Closed

0xc0170 added the needs: work label Dec 15, 2016

jeromecoutant force-pushed the PR_LPT_F4 branch from e756fc1 to d1d2500 Compare January 9, 2017 09:34

c1728p9 suggested changes Jan 9, 2017

View reviewed changes

bridadan mentioned this pull request Jan 12, 2017

Add uVisor support for the DISCO_F429ZI #3397

Merged

jeromecoutant added 2 commits January 13, 2017 15:38

Add LOWPOWERTIMER for NUCLEO_F401RE and DISCO_F429ZI

8adb393

Fix issue in case of MBED_HEAP_STATS_ENABLED enabled

63c5c7d

Add a workaround in lp_ticker_init function for CI tests

jeromecoutant force-pushed the PR_LPT_F4 branch from d1d2500 to 63c5c7d Compare January 13, 2017 14:39

sg- removed the needs: CI label Feb 9, 2017

jeromecoutant closed this Mar 6, 2017

jeromecoutant deleted the PR_LPT_F4 branch March 6, 2017 10:25

Add LOWPOWERTIMER for NUCLEO_F401RE and DISCO_F429ZI #3319

Add LOWPOWERTIMER for NUCLEO_F401RE and DISCO_F429ZI #3319

Conversation

jeromecoutant commented Nov 24, 2016

Description

Status

Tests

adbridge commented Nov 24, 2016

adbridge commented Nov 24, 2016

adbridge commented Nov 24, 2016

bridadan commented Nov 28, 2016

0xc0170 commented Nov 28, 2016

0xc0170 commented Nov 28, 2016

jeromecoutant commented Nov 28, 2016

mbed-bot commented Nov 28, 2016

Result: SUCCESS

Output

bridadan commented Nov 29, 2016

mbed-bot commented Nov 29, 2016

Result: FAILURE

Output

bridadan commented Nov 29, 2016

jeromecoutant commented Nov 30, 2016

bridadan commented Dec 1, 2016

0xc0170 commented Dec 2, 2016

0xc0170 commented Dec 2, 2016

mbed-bot commented Dec 2, 2016

Result: FAILURE

Output

0xc0170 commented Dec 5, 2016 • edited Loading

jeromecoutant commented Dec 6, 2016

adbridge commented Dec 12, 2016

bridadan commented Dec 12, 2016

jeromecoutant commented Dec 14, 2016

jeromecoutant commented Dec 14, 2016

0xc0170 commented Dec 15, 2016

bridadan commented Dec 15, 2016

bridadan commented Dec 19, 2016

0xc0170 commented Dec 21, 2016

0xc0170 commented Dec 30, 2016

jeromecoutant commented Jan 9, 2017

0xc0170 commented Jan 9, 2017

bridadan commented Jan 9, 2017

jeromecoutant commented Jan 9, 2017

bridadan commented Jan 9, 2017

c1728p9 Jan 9, 2017

Choose a reason for hiding this comment

jeromecoutant Jan 10, 2017

Choose a reason for hiding this comment

0xc0170 Jan 11, 2017

Choose a reason for hiding this comment

c1728p9 Jan 11, 2017

Choose a reason for hiding this comment

0xc0170 commented Jan 12, 2017

jeromecoutant commented Jan 13, 2017

0xc0170 commented Jan 13, 2017

jeromecoutant commented Jan 16, 2017

0xc0170 commented Feb 2, 2017

0xc0170 commented Dec 5, 2016 •

edited

Loading