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 problem with low level lp_ticker STM wrapper #11385

Merged
merged 2 commits into from Sep 5, 2019

Conversation

@Tharazi97
Copy link
Contributor

commented Aug 30, 2019

Description

Fixes #11373

I have changed the STM implementation of low level lp ticker wrapper in way that it checks if event gets outdated while waiting for CMPOK flag. I have also added a solution of handling events that are close to ticker counter roll over. It changes the base of the timestamp so it can be easily compared with other values around max counter value.

/* Change the lp_delayed_counter buffor in that way so the value of (0xFFFF - LP_TIMER_SAFE_GUARD) is equal to 0.
             * By doing this it is easy to check if the value of timestamp get outdated by delaying its programming
             * For example if LP_TIMER_SAFE_GUARD is set to 5
             * (0xFFFA + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 0
             * (0xFFFF + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 5
             * (0x0000 + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 6
             * (0x0005 + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF = 11*/
            lp_delayed_counter = (timestamp + LP_TIMER_SAFE_GUARD + 1) & 0xFFFF;

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@maciejbocianski @mprse @jamesbeyond @LMESTM @0xc0170

Release Notes

@Tharazi97 Tharazi97 force-pushed the Tharazi97:lp_ticker_wrapper_change branch from 4551399 to 5cb7297 Aug 30, 2019
@ciarmcom ciarmcom requested review from 0xc0170, jamesbeyond, maciejbocianski, mprse and ARMmbed/mbed-os-maintainers Aug 30, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@LMESTM
LMESTM approved these changes Aug 30, 2019
Copy link
Contributor

left a comment

I think this change makes sense and is properly described ... thank you
2 minor remarks / questions

/* If this target timestamp is close to the roll over of the ticker counter
* and current tick is also close to the roll over, then we are in danger zone.*/
if(((0xFFFF - LP_TIMER_SAFE_GUARD < timestamp) || (timestamp < LP_TIMER_SAFE_GUARD))
&& ((0xFFFA < last_read_counter) || (last_read_counter < LP_TIMER_SAFE_GUARD)))

This comment has been minimized.

Copy link
@LMESTM

LMESTM Aug 30, 2019

Contributor

Why is the condition (last_read_counter < LP_TIMER_SAFE_GUARD) also needed ?
If this is the case, then the rollover already happened, right ?
I don't think it would cause an issue though ...

This comment has been minimized.

Copy link
@Tharazi97

Tharazi97 Aug 30, 2019

Author Contributor

You are right. It could do some mess if you want to set timestamp to 0xFFFF, or something close to, and current tick is at the beginning of the counter.

&& ((0xFFFA < last_read_counter) || (last_read_counter < LP_TIMER_SAFE_GUARD)))
{
roll_over_flag = true;
/* Change the lp_delayed_counter buffor in that way so the value of (0xFFFF - LP_TIMER_SAFE_GUARD) is equal to 0.

This comment has been minimized.

Copy link
@LMESTM

LMESTM Aug 30, 2019

Contributor

type buffor / buffer

This comment has been minimized.

Copy link
@Tharazi97

Tharazi97 Aug 30, 2019

Author Contributor

Done

@Tharazi97 Tharazi97 force-pushed the Tharazi97:lp_ticker_wrapper_change branch from 5cb7297 to 36dfbca Aug 30, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Sep 2, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

/* If this target timestamp is close to the roll over of the ticker counter
* and current tick is also close to the roll over, then we are in danger zone.*/
if(((0xFFFF - LP_TIMER_SAFE_GUARD < timestamp) || (timestamp < LP_TIMER_SAFE_GUARD)) && (0xFFFA < last_read_counter))
{

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 2, 2019

Member

please fix the style in the file (=consistency)

This comment has been minimized.

Copy link
@Tharazi97

Tharazi97 Sep 2, 2019

Author Contributor

Done

@0xc0170 0xc0170 added needs: work and removed needs: review labels Sep 2, 2019
@Tharazi97 Tharazi97 force-pushed the Tharazi97:lp_ticker_wrapper_change branch from 36dfbca to 3e1aa43 Sep 2, 2019
@Tharazi97 Tharazi97 force-pushed the Tharazi97:lp_ticker_wrapper_change branch from 3e1aa43 to a95450b Sep 2, 2019
Copy link
Contributor

left a comment

look good

@0xc0170
0xc0170 approved these changes Sep 4, 2019
@0xc0170 0xc0170 requested a review from ARMmbed/team-st-mcd Sep 4, 2019
@0xc0170 0xc0170 added needs: review and removed needs: work labels Sep 4, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Started CI

@LMESTM
LMESTM approved these changes Sep 4, 2019
Copy link
Contributor

left a comment

Thanks a lot !

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 4, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Sep 4, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

CI fail seems not related.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 5, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 merged commit b8ebee5 into ARMmbed:master Sep 5, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8654 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.