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

STM32_us_ticker_16b: keep code to cope with past event #5012

Merged
merged 1 commit into from Sep 9, 2017

Conversation

Projects
None yet
6 participants
@LMESTM
Contributor

LMESTM commented Sep 4, 2017

Description

Fixes test suggested in #5004.

Fixing regression introduced in commit
"Ticker: add fire interrupt now function".

In above mentioned commit, the management of timestamp being in the past
has been moved to higher layer (hal/mbed_ticker_api.c), but the reset of
oc_int was missing when implementing the new us_ticker_fire_interrupt
function - which is fixed now.

Status

READY

Todos

  • Tests
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

cc @pan-

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

What is this fixing? What regression was introduced ?

I would keep the else part, but if delta is negative - should be handled by ticker implementation in the above layer ?

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 4, 2017

@0xc0170 As explained in description, this fixes #5004. The test of #5004 works fine before merge of #4644 and fails just after => this is the regression.

In us_ticker_set_interrupt we need to read timestamp again, so delta can be positive when read from above layer, then negative when reading again in us_ticker_set_interrupt. Therefore I think that we cannot get rid of this check.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 4, 2017

As this fixes a regression already introduced in a PR marked for 5.6, we need to also get this one in for 5.6.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@0xc0170 As explained in description, this fixes #5004. The test of #5004 works fine before merge of #4644 and fails just after => this is the regression.

That reading is fine, but the first condtion could be removed. have you tested it with only else there? when delta is positive. I believe it should work

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 4, 2017

@0xc0170 Before reverting the full function, I did the test with only the else part and the test fails.
My understanding of the situation is as said: In us_ticker_set_interrupt we need to read timestamp again, so delta can be positive when read from above layer, but still negative when reading again in us_ticker_set_interrupt

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 4, 2017

@0xc0170 actually this makes me think that maybe the same applies to us_ticker_32b.c except the probability is just very low ...

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 5, 2017

@0xc0170 Before reverting the full function, I did the test with only the else part and the test fails.
My understanding of the situation is as said: In us_ticker_set_interrupt we need to read timestamp again, so delta can be positive when read from above layer, but still negative when reading again in us_ticker_set_interrupt

as delta is only int (very short range), we do not know if it was in the past or in the future (it could have wrapped around by the time). We cleaned all targets , to remove this check, the upper layer does it and knows better as using 64bit timestamp.

See https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_ticker_api.c#L120 . The set interrupt function should only receive positive deltas (new match timestamp is in the future, even very near future).

Aren't we missing something else here? Fire interrupt does not touch oc int part, should it?

@pan- What is your view on this one?

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 5, 2017

See https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_ticker_api.c#L120 . The set interrupt function should only receive positive deltas (new match timestamp is in the future, even very near future).

Actually the condition of event being in the future in line 120 can have changed when line 129 is reached and the call is done, or even inside the set_interrupt function. BUT there is another sanity check few lines later here https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_ticker_api.c#L130 which does the job indeed. I first thought this may be buggy in some way, but ...

Aren't we missing something else here? Fire interrupt does not touch oc int part, should it?

Good catch. oc_int part is the counter of 16bits wrap-ups until next interrupt must be triggered. When event is in the past or when we fire interrupt, this needs to be reset to 0. When fixing this in fire_intterrupt, the tests are PASSED again. I will update my PR accordingly.

STM32_us_ticker_16b: reset counter when fire interrupt
Fixing regression introduced in commit
"Ticker: add fire interrupt now function".

In above mentioned commit, the management of timestamp being in the past
has been moved to higher layer (hal/mbed_ticker_api.c), but the reset of
oc_int was missing when implementing the new us_ticker_fire_interrupt
function - which is fixed now.

@LMESTM LMESTM force-pushed the LMESTM:issue5004_ticker_16b branch to f2eb77a Sep 5, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 5, 2017

@0xc0170 I just pushed the new version of the patch

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 5, 2017

And Ticker tests passed ok

@0xc0170

0xc0170 approved these changes Sep 5, 2017

LGTM

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 5, 2017

@LMESTM 👍 Should be all OK, in case not, we should write a test (however mbed ticker API has extensive testing via mocks), and tick classes are now getting also test updates, teh coverage should increase soon

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 5, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 5, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 5, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1223

Example Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 7, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 7, 2017

Result: ABORTED

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

/morph test

Output

mbed Build Number: 1240

Build Prep failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 8, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 9, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1262

All builds and test passed!

@theotherjimmy theotherjimmy merged commit 3f2e986 into ARMmbed:master Sep 9, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment