Skip to content
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

Fix initialisation sequence of RTC #15184

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Conversation

mjh65
Copy link
Contributor

@mjh65 mjh65 commented Nov 29, 2021

Initialisation of RTC was wrong for boot from sysreset, and resulted
in RTOS with nothing to do in its main thread. This fixes the bug.

Summary of changes

Impact of changes

Migration actions required

Documentation


Pull request type

[] 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)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


Initialisation of RTC was wrong for boot from sysreset, and resulted
in RTOS with nothing to do in its main thread. This fixes the bug.
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is quite old platform, this should be functional as it was. What bug is this fixing?

@@ -127,10 +127,10 @@ time_t rtc_read(void)
//******************************************************************************
void lp_ticker_init(void)
{
init_rtc();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this order would matter? I would assume it should work either way (the 3 functions below are just setting nvic and interrupt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm developing software for a MAX32630, and I was finding that the software ran as expected from a POR/power-cycle, but would never reach main() when I was using the DAPlink debugger. So I spent some time investigating this, and found that the init_rtc() function was also never reached. When the NVIC IRQ was enabled the ISR would fire, and never returned.
(Although I didn't debug this part in detail, I assume that instead of a normal return from the IRQ, this is being used to pass control to the RTOS scheduler. But in this case the main() function has not yet been added to the queue, so there is nothing to do, and the system is deadlocked.)
The reason for the difference in behaviour between POR and DAPlink debugger appears to be that the debugger issues a SYSRESET, and the RTC survives the SYSRESET. The RTC continues to generate interrupt conditions while the debugger is flashing the device and starting up.
I was able to resolve the problem by moving the call to init_rtc() to the start of lp_ticker_init(), after which I found that the breakpoint on main() would be hit as expected when using the debugger. The setup in init_rtc() puts the module into a safe state so that the NVIC interrupt can be enabled without being immediately triggered. There is no difference to the POR behaviour.
Our project is using a fork of the Mbed repo. Unfortunately I don't have time to run all of the Mbed test suites, but I thought I would offer this fix to the main project. As mentioned, this is an old device, and the issue would only affect use with a debugger, or reset due to watchdog or software trigger. Maybe these scenarios are not covered in testing, and have not been used in real projects?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjh65 Thank you very much for your effort and contribution, As Maxim (Now part of ADI) we appreciate it.
Please let us think on it and run related test suites.
Regards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ozersa Any update, did the tests fail?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related test suites passed for GCC_ARM. The change request is fine.
GCC_ARM_TestResult_For_PR_15184.txt

@mjh65 thanks for your contribution.

@ciarmcom
Copy link
Member

@mjh65, thank you for your changes.
@ARMmbed/team-maximintegrated @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 7, 2021

CI started

@mergify mergify bot added needs: CI and removed needs: review labels Dec 7, 2021
@mbed-ci
Copy link

mbed-ci commented Dec 7, 2021

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit b129f6e into ARMmbed:master Dec 7, 2021
@mergify mergify bot added release version missing When PR does not contain release version, bot should label it and we fix it afterwards and removed ready for merge labels Dec 7, 2021
@mergify
Copy link

mergify bot commented Dec 7, 2021

This PR does not contain release version label after merging.

@0xc0170 0xc0170 added release-type: patch Indentifies a PR as containing just a patch and removed release version missing When PR does not contain release version, bot should label it and we fix it afterwards labels Dec 7, 2021
@mbedmain mbedmain added release-version: 6.16.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants