Add workaround for STM32F4 hardfault in sleep mode #12662

artokin · 2020-03-20T11:03:30Z

Summary of changes

This workaround is related to Mbed OS issue
#12294

We would like temporarily disable sleep from STM32F4. The hardfault occurs so often that it is preventing us to continue with large scale network testing.

This workaround can be removed once the root cause for hardfault is corrected.

Impact of changes

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

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

Reviewers

@TuomoHautamaki , @teetak01 , @mikter , @jeromecoutant

This workaround is related to Mbed OS issue ARMmbed#12294

jeromecoutant

Not in master branch

Issue is pyOCD side

MarceloSalazar · 2020-03-20T11:28:26Z

I agree with @jeromecoutant , we cannot reproduce this consistently and it seems caused by tools/infrastructure.
We need to continue to investigate to get to the bottom of the problem (I'm currently testing)

TuomoHautamaki · 2020-03-20T11:43:29Z

Not disagreeing but we need workaround untill problem is fixed

MarceloSalazar · 2020-03-20T11:49:38Z

@TuomoHautamaki as a workaround the team can create a patch to apply locally after cloning the repo. This will let you proceed with testing until we understand the root cause of the problem.

ciarmcom · 2020-03-20T12:00:32Z

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

TuomoHautamaki · 2020-03-20T12:01:08Z

We don't want to adjust our test jobs when there are issues in the SW under testing and this has been open quite long now.

I understand this may look a bit harsh when disabling the whole feature. However, there are also other users reporting the same issue and I doubt they are using our CI system. Disabling this feature works for them as well.

We can enable sleep feature again once fix is available.

MarceloSalazar

Workaround not suitable for unknown problem

jeromecoutant · 2020-03-20T12:43:42Z

You can disable DEVICE_HAS_SLEEP in your local mbed_app.json

juhhei01 · 2020-03-20T13:15:49Z

If every application must disable sleep it not sound a good idea. This problem have not new and it comes typically when binary size is updated. With workaround there is no problem so when this problem will be fixed ”properly”. App change hidden issue not fix that.

0xc0170 · 2020-03-23T13:29:40Z

You can disable DEVICE_HAS_SLEEP in your local mbed_app.json

@jeromecoutant reading the issue referenced, the problem was confirmed by various users. Shouldn't we disable sleep for specific targets? Not changing the driver code as in this PR at the moment. The problem is known - sleep is causing it but a root cause not yet (might be something else but that needs further investigation). We saw reports for L4, odin and nucleo f429 at least.

Issue is pyOCD side

It was reported with IAR or openOCD as well, this seems more to be in drivers than debug tools?

Is this only with debug profile, release is all good?

I think yes until this is fixed. Adding device_has_remove: ['SLEEP'] for affected targets. What targets are affected, all F4 (every target with label STM32F4 should have it?) ?

Let's do it other way around - remove sleep support for affected targets, have a known issue in the next release if its not fixed there (hopefully will be so not needed). Rather than keeping this enabled and known issue like "it might fail for you, just disable it in your app config".

0xc0170 · 2020-03-23T13:42:26Z

Set this to needs: review, more discussion ongoing how to proceed.

jamesbeyond · 2020-03-26T17:51:24Z

Hi @artokin,
I believe we found the cause of the issue,
please see my comments on #12294 (comment)

Also, could you try the suggested solutions? that is works for me and 2 other users. if the solution works for you, can you update your PR to use that as a fix

Pull request has been modified.

MarceloSalazar · 2020-03-27T12:02:55Z

@jeromecoutant we discussed and found this workaround of adding 3 x NOPs seems to be the best option as it's independent of tools and debug/release profiles.
Can you please check and confirm you're ok with this so we can proceed with CI testing? Thanks!

LMESTM · 2020-03-27T12:07:08Z

@jeromecoutant we discussed and found this workaround of adding 3 x NOPs seems to be the best option as it's independent of tools and debug/release profiles.
Can you please check and confirm you're ok with this so we can proceed with CI testing? Thanks!

Question: looks like a good way forward.
Extra question: Is the testing environment using DEBUG profile ? If so, can we have the NOP workarounds under MBED_DEBUG compilation switch so that the RELEASE builds aren't impacted ?

jeromecoutant · 2020-03-27T13:05:54Z

ST_INTERNAL_REF 83447

jeromecoutant

ST CI OK

jamesbeyond · 2020-03-27T16:55:23Z

Question: looks like a good way forward.
Extra question: Is the testing environment using DEBUG profile ? If so, can we have the NOP workarounds under MBED_DEBUG compilation switch so that the RELEASE builds aren't impacted ?

Hi @LMESTM, I was thinking about your suggestion. But I feel maybe we should leave it like this.

The reason is in the errata sheet, it didn't claim the crash have anything to do with the compiler debug options. So my theory is the app will crash regardless of what build profile user use. It just because the debug tools will most likely set the MUCDBG_CR flag when a debug build image being flashed. hence we seeing more frequent of this issue.

Also, our client team did report they randomly seeing similar crashes on develop/release profile build, which looks might be related.

Furthermore, I don't feel it is huge impact/compromise for adding 3xNOP in the code, even to the release profile. if you have other concerns, feel free to let us know.

jamesbeyond · 2020-03-27T16:57:11Z

targets/TARGET_STM/TARGET_STM32F4/device/stm32f4xx_hal_pwr.c

@@ -392,6 +392,9 @@ void HAL_PWR_EnterSLEEPMode(uint32_t Regulator, uint8_t SLEEPEntry)
  {   
    /* Request Wait For Interrupt */
    __WFI();
+    __NOP(); // Workaround for STM32F4 errata
+    __NOP(); // see chapter 2.1.3 - Debugging Sleep/Stop mode with WFE/WFI entry
+    __NOP(); // https://www.st.com/resource/en/errata_sheet/dm00037591-stm32f405-407xx-and-stm32f415-417xx-device-limitations-stmicroelectronics.pdf


What about the WFE() in below lines ? do we need them to be patched as well?

Mbed OS doesn't use that mode, so not required for us - would just be an unnecessary image size increase. But if the change is being upstreamed to the HAL, seems like you'd want them there.

As the upstream comes to our repo, this will be updated once driver is updated.

@jeromecoutant We fine as it is here now, correct?

I am not sure that ST driver team will accept this change :-(
But as it solves some mbed issues, we are ok to merge this wrkaround.

Is there a better alternative, given the errata description? That is the most universally-feasible workaround in your errata sheet.

As soon as confirmed, I'll merge this. It would be good to have this in today.

@LMESTM if you don't mind, we'll proceed with this change for both debug and release for now, as there are other random failures in release mode that we're seeing in CI and need to investigate and rule out for the limited time we have.
Then we can introduce optimizations once we have more information.

Ok. If RELEASE build is used in CI as well, then the same loading mechanism will apply (thru debug port) and the same issue may arise. So we can't put the proposed compilation switches - please forget my comment and feel free to proceed ...

There is a separate argument that something should be done to get that debug/sleep control register back into its default state anyway, otherwise the just-flashed release build in test is not representative of the deployed image in terms of sleep, whether that comes down to power measurements, timing, hitting bugs like this. Arguably release builds should be manually putting that back to its zero state. Flasher should also have taken more care...

We think it's the flasher role to do so. If you power cycle the product, the release build will work just fine ... I'm not sure we should add an explicit reset of those bits, because even the release build should be debugged and in this case the flasher or debugger (or flasher of the debugger) should set the bits.

I know bigger systems like Linux are intensely paranoid about chip state post-bootloader, so tend not to rely on any default state. But we can't really afford the ROM space for that level of paranoia in an embedded image.

And yes, in this specific case, I see your point that a debugger may obviously have deliberately set those bits before running us, for a good reason, so an image should not reset any bits there, regardless of the general "trust default state or not" philosphy.

0xc0170 · 2020-03-30T09:33:05Z

@artokin This should also be in 5.15.2 (please send another PR directly to the branch) ?

0xc0170 · 2020-03-30T13:28:13Z

@artokin This should also be in 5.15.2 (please send another PR directly to the branch) ?

I've got a branch, I'll create PR shortly

0xc0170 · 2020-03-30T13:31:48Z

#12717 Created

0xc0170 · 2020-03-30T15:28:12Z

@Mergifyio backport mbed-os-5.15

0xc0170 · 2020-03-31T06:34:06Z

That did not work as I assume I created PR (cant create duplicate). I'll test this on another PR

Disable sleep on STM32F4 as an workaround for stability issues.

40672c5

This workaround is related to Mbed OS issue ARMmbed#12294

artokin requested review from TuomoHautamaki, mikter, teetak01 and jeromecoutant March 20, 2020 11:07

jeromecoutant suggested changes Mar 20, 2020

View reviewed changes

adbridge mentioned this pull request Mar 20, 2020

Cordio: fix for issue #12625 #12637

Merged

adbridge mentioned this pull request Mar 20, 2020

Makefile: fix for overlong command line #12646

Merged

ciarmcom requested review from jeromecoutant and a team March 20, 2020 12:00

ciarmcom added the needs: review label Mar 20, 2020

TuomoHautamaki approved these changes Mar 20, 2020

View reviewed changes

MarceloSalazar self-requested a review March 20, 2020 12:04

MarceloSalazar previously requested changes Mar 20, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Mar 20, 2020

0xc0170 added needs: review and removed needs: work labels Mar 23, 2020

0xc0170 added needs: work and removed needs: review labels Mar 26, 2020

Add workaround for F429 hardfault

5aa66b5

MarceloSalazar changed the title ~~Disable sleep on STM32F4 as an workaround for stability issues.~~ Add workaround for STM32F4 hardfault in sleep mode Mar 27, 2020

mikter approved these changes Mar 27, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Mar 27, 2020

jeromecoutant approved these changes Mar 27, 2020

View reviewed changes

jamesbeyond reviewed Mar 27, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Mar 30, 2020

0xc0170 added the release-version: 6.0.0-beta-1 label Mar 30, 2020

MarceloSalazar self-requested a review March 30, 2020 12:07

MarceloSalazar approved these changes Mar 30, 2020

View reviewed changes

0xc0170 merged commit 92cdcfb into ARMmbed:master Mar 30, 2020

mergify bot removed the ready for merge label Mar 30, 2020

0xc0170 mentioned this pull request Mar 30, 2020

Add fix for STM32F4 hardfault in sleep mode #12717

Merged

artokin mentioned this pull request Mar 31, 2020

feature-wisun: Add fix for STM32F4 hardfault in sleep mode #12726

Merged

jeromecoutant mentioned this pull request May 5, 2020

mbed-cloud-client-example fatal run-time error on ST boards (SetSysClock failed) #12922

Closed

jeromecoutant mentioned this pull request May 25, 2020

STM32L0xx Hardfault after sleep while running the debugger #6138

Closed

Add workaround for STM32F4 hardfault in sleep mode #12662

Add workaround for STM32F4 hardfault in sleep mode #12662

Uh oh!

Conversation

artokin commented Mar 20, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

MarceloSalazar commented Mar 20, 2020

Uh oh!

TuomoHautamaki commented Mar 20, 2020

Uh oh!

MarceloSalazar commented Mar 20, 2020

Uh oh!

ciarmcom commented Mar 20, 2020

Uh oh!

TuomoHautamaki commented Mar 20, 2020

Uh oh!

MarceloSalazar left a comment

Choose a reason for hiding this comment

Uh oh!

jeromecoutant commented Mar 20, 2020

Uh oh!

juhhei01 commented Mar 20, 2020

Uh oh!

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

Uh oh!

Uh oh!

0xc0170 commented Mar 23, 2020

Uh oh!

jamesbeyond commented Mar 26, 2020

Uh oh!

MarceloSalazar commented Mar 27, 2020

Uh oh!

LMESTM commented Mar 27, 2020

Uh oh!

jeromecoutant commented Mar 27, 2020

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

jamesbeyond commented Mar 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 30, 2020

Uh oh!

0xc0170 commented Mar 30, 2020

Uh oh!

0xc0170 commented Mar 30, 2020

Uh oh!

0xc0170 commented Mar 30, 2020

0xc0170 commented Mar 23, 2020 •

edited

Loading

kjbracey Mar 30, 2020 •

edited

Loading