Skip to content

STM: Fix Hal_GetTick wrap around errors #14215

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 3, 2021
Merged

STM: Fix Hal_GetTick wrap around errors #14215

merged 1 commit into from
Feb 3, 2021

Conversation

flippy84
Copy link
Contributor

@flippy84 flippy84 commented Jan 29, 2021 •
edited
Loading

Summary of changes

This pull request changes the handling of the HAL_GetTick timer in the STM target.

  1. It makes sure the timer is only initialized once to prevent a wrap around at the start since HAL_InitTick is called twice.
  2. It prevents a wrap around error when the scheduler interrupts one HAL_GetTick and another HAL_GetTick is later called from another thread resulting in new_time < prev_time.

Both commits prevents a wrap around when new_time < prev_time which results in the wrong number of ticks added to total_ticks.

Impact of changes

Migration actions required

Documentation

None

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 

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

mbed-os-hal-gettick-tested

Tested and verified with J-Link Ozone execution counters.

  1. Initialization is only done once.
  2. There is no wrap arounds with two threads running Hal_GetTick in a while loop.

Reviewers

@flippy84 flippy84 mentioned this pull request Jan 29, 2021
Closed
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jan 29, 2021
@ciarmcom
Copy link
Member

@flippy84, thank you for your changes.
@ARMmbed/team-st-mcd @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from a team January 29, 2021 14:30
LMESTM
LMESTM reviewed Jan 29, 2021
View reviewed changes
targets/TARGET_STM/hal_tick_overrides.c Outdated

HAL_StatusTypeDef HAL_InitTick(uint32_t TickPriority)
{
if (initialized) {
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for your contribution
Shouldn't HAL_InitTick content also be under critical section if this can happen that HAL_InitTick is called from several threads ?

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 don't think that is needed since HAL_InitTick is called before main() is called and no user threads are running yet.

@flippy84
Copy link
Contributor Author

I forgot to add include "hal/critical_section_api.h". Will add that include so it compiles without warnings in another commit.

@jeromecoutant
Copy link
Collaborator

Hi
Tests become FAIL... :-(

ex:

$ mbed test -m NUCLEO_F429ZI -t ARM -v -n hal-tests-tests-mbed_hal-common_tickers_freq


[1611936075.23][CONN][RXD] >>> Running case #1: 'Microsecond ticker frequency test'...
[1611936075.28][CONN][INF] found KV pair in stream: {{__testcase_start;Microsecond ticker frequency test}}, queued...
[1611936075.33][CONN][INF] found KV pair in stream: {{timing_drift_check_start;0}}, queued...
[1611936075.34][SERI][TXD] {{base_time;0}}
[1611936075.39][CONN][INF] found KV pair in stream: {{base_time;439544}}, queued...
[1611936085.91][HTST][INF] Device base time 439544
[1611936085.91][HTST][INF] sleeping for 10.497550964355469 to measure drift accurately
[1611936085.92][SERI][TXD] {{final_time;0}}
[1611936085.95][CONN][INF] found KV pair in stream: {{final_time;55915088}}, queued...
[1611936085.95][HTST][INF] Device final time 55915088
[1611936085.95][HTST][INF] Compute host events
[1611936085.95][HTST][INF] Transport delay 0: 0.06871938705444336
[1611936085.95][HTST][INF] Transport delay 1: 0.04667854309082031
[1611936085.95][HTST][INF] DUT base time : 439544.0
[1611936085.95][HTST][INF] DUT end time  : 55915088.0
[1611936085.95][HTST][INF] min_pass : 10.096544301509857 , max_pass : 11.038170611858368 for 5.0%%
[1611936085.95][HTST][INF] min_inconclusive : 9.986916267871857 , max_inconclusive : 11.159338438510895
[1611936085.95][HTST][INF] Time reported by device: 55.475544
[1611936085.95][HTST][INF] Time outside of passing range. Timing drift seems to be present !!!
[1611936085.97][SERI][TXD] {{fail;0}}
mbedgt: :130::FAIL: Expected 'pass' Was 'fail'. Host side script reported a fail...
[1611936086.06][CONN][RXD] :130::FAIL: Expected 'pass' Was 'fail'. Host side script reported a fail...

LMESTM
LMESTM approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

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

LGTM - now must be tested by @jeromecoutant

@flippy84
Copy link
Contributor Author

flippy84 commented Feb 1, 2021

@jeromecoutant I will look into the failed test and see if i can get it working.

@flippy84
Copy link
Contributor Author

flippy84 commented Feb 2, 2021

Hi @jeromecoutant, I sorted out the issue with the failed tests. It had to do with the double initialization code. I've removed that check and tested on a Nucleo F413ZH board.
Changed to core_util_critical_section_enter to prevent a crash when running mbed.

Attached is two test runs, one with the test that failed and the other a full test run.

test_report.txt
full_test_report.txt

@flippy84
Copy link
Contributor Author

flippy84 commented Feb 3, 2021

Tidied up the commit history, let me know if there is something else.

@mbed-ci
Copy link

mbed-ci commented Feb 3, 2021

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_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-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️

@0xc0170 0xc0170 merged commit 7135c65 into ARMmbed:master Feb 3, 2021
@mergify mergify bot removed the ready for merge label Feb 3, 2021
@mbedmain mbedmain added Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

+2 more reviewers

@jeromecoutant jeromecoutant jeromecoutant approved these changes

@LMESTM LMESTM LMESTM approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
release-version: 6.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@flippy84 @ciarmcom @jeromecoutant @mbed-ci @0xc0170 @LMESTM @mbedmain