Only schedule mbed_ticker interrupt if queue->head is changed #6515

marcemmers · 2018-03-30T10:35:18Z

Reverts change from commit 1057720

Description

It seems to me it is unneccesary to schedule a new interrupt the new event is further in the queue and head is still the same. This is essentially the same as is done in the remove_event function: only reschedule if the head is removed.

Doesn't really fix any issues but does result in better response time. Only tested on a STM32L072 in low power configuration.

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

pan-

The change looks good; it should improve insertion time for events the do not replace the head of the queue.

0xc0170 · 2018-04-06T15:25:52Z

/morph build

mbed-ci · 2018-04-06T16:15:00Z

Build : SUCCESS

Build number : 1678
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6515/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-06T18:23:25Z

Exporter Build : SUCCESS

Build number : 1310
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6515/

mbed-ci · 2018-04-06T21:23:31Z

Test : FAILURE

Build number : 1471
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6515/1471

0xc0170 · 2018-04-09T08:03:38Z

/morph test
/morph mbed2-build

marcemmers · 2018-04-09T08:04:46Z

I don't really know how the tests should work. It looks like this failure originates from a value from a previous test that is still in the interface_stub.

The value expected is 0xCCCCCCCC but actually is 0x7000000 which is equal to TIMESTAMP_MAX_DELTA used in the test_legacy_insert_event_head().

mbed-ci · 2018-04-09T10:52:44Z

Test : FAILURE

Build number : 1473
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6515/1473

0xc0170 · 2018-04-09T15:14:53Z

The value expected is 0xCCCCCCCC but actually is 0x7000000 which is equal to TIMESTAMP_MAX_DELTA used in the test_legacy_insert_event_head().

@pan- @mprse Can you help?

marcemmers · 2018-04-09T15:28:06Z

I am testing locally also and the change does seem to cause the issue but I don't really know why.
I ran it using the debugger with GREENTEA_SETUP(60, "default_auto"); commented and then it passed the test.

EDIT: It also fails using the debugger, forgot to actually use the changed code, whoops.

marcemmers · 2018-04-10T06:48:17Z

I have found the issue.
The test that fails has the following events before the changes in this PR:

Initialize Ticker -> interrupt set at max_delta
Add event with timestamp 0xCCCCCCCC -> interrupt set at max_delta
Update interface time
Add event with timestamp 0x10000000A -> interrupt set at 0xCCCCCCCC because it is the head

What is missing is the interrupt that would be caused by the interface time overflowing max_delta.
This would have caused the interrupt time to be updated to 0xCCCCCCCC and the following sequence with the changes in this PR:

Initialize Ticker -> interrupt set at max_delta
Add event with timestamp 0xCCCCCCCC -> interrupt set at max_delta
Update interface time
Interrupt -> interrupt set at 0xCCCCCCCC
Add event with timestamp 0x10000000A -> interrupt not changed because the first event is still at the head

I have added the changes for the test to this PR.

0xc0170 · 2018-04-10T06:54:51Z

@marcemmers Thanks for the update, we will review

mprse · 2018-04-10T08:21:05Z

@marcemmers I have analysed this with the same conclusion.
In the original version interrupt reschedule was performed every time a new event has been added to the list. Now it is only performed when HEAD changes (the closest interrupt to be fired).
In the previous version this was irrelevant since interrupt reschedule was performed every time the new event was added to the list. Now reschedule is not performed after inserting next events since HEAD does not changes.
Test scenario does not take into consideration that interrupt will fire while simulating time passing, this will force missing reschedule. This is a nice change. It should increase the performance.

0xc0170 · 2018-04-10T13:26:14Z

/morph build

mbed-ci · 2018-04-10T13:45:19Z

Build : SUCCESS

Build number : 1708
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6515/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-10T18:57:53Z

Exporter Build : SUCCESS

Build number : 1342
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6515/

mbed-ci · 2018-04-11T06:06:36Z

Test : FAILURE

Build number : 1502
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6515/1502

0xc0170 · 2018-05-30T09:21:30Z

@marcemmers Can you rebase on top of the latest master? Ticker improvements landed and we would like to test this fix.

Reverts change from commit 1057720

…e previous interrupt_timestamp would have triggered one anyway

marcemmers · 2018-05-30T09:44:45Z

@0xc0170 Should be ok now?

cmonr · 2018-05-31T16:28:18Z

@pan- @c1728p9 All good?

cmonr · 2018-05-31T20:19:43Z

/morph build

mbed-ci · 2018-06-01T02:12:16Z

Build : SUCCESS

Build number : 2212
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6515/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-06-01T15:41:01Z

Test : FAILURE

Build number : 2005
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6515/2005

mbed-ci · 2018-06-01T21:32:44Z

Exporter Build : ABORTED

Build number : 1839
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6515/

maciejbocianski · 2018-06-02T06:40:27Z

Test: tests-mbed_drivers-timer
Result: FAIL
Platform: NRF51_DK - Toolchain: IAR

[1527863410.40][CONN][RXD] >>> Running case #9: 'Test: Timer (based on os ticker) - float operator.'... [1527863410.40][CONN][INF] found KV pair in s4tream: {{__testcase_start;Test: Timer (based on os ticker) - float operator.}}, queued... 
[1527863410.48][CONN][RXD] :669::FAIL: Expected 0.010000 Was 0.010815 
[1527863410.58][CONN][INF] found KV pair in stream: {{__testcase_finish;Test: Timer (based on os ticker) - float operator.;0;1}}, queued...

fix is already being developed here #7070

cmonr · 2018-06-02T07:15:39Z

@maciejbocianski Thought that error looked familiar.

Gonna rerun the test CI on the off chance that it passes. Otherwise if it happens again we'll likely need to hold off for that PR to land.

/morph test

mbed-ci · 2018-06-02T13:37:52Z

Test : SUCCESS

Build number : 2017
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6515/2017

0xc0170 · 2018-06-04T13:36:04Z

/morph export-build

0xc0170 · 2018-06-04T13:36:31Z

/morph mbed2-build

mbed-ci · 2018-06-04T19:23:26Z

Exporter Build : SUCCESS

Build number : 1857
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6515/

0xc0170 requested a review from pan- April 3, 2018 12:09

0xc0170 added the needs: review label Apr 3, 2018

0xc0170 requested a review from c1728p9 April 3, 2018 12:09

pan- previously approved these changes Apr 4, 2018

View reviewed changes

c1728p9 previously approved these changes Apr 5, 2018

View reviewed changes

maciejbocianski mentioned this pull request Apr 6, 2018

Consecutive ticker callbacks result more timer interval interrupt requests than expected #6454

Closed

0xc0170 added needs: CI and removed needs: review labels Apr 6, 2018

0xc0170 added needs: work and removed needs: CI labels Apr 9, 2018

marcemmers dismissed stale reviews from c1728p9 and pan- via f88dce9 April 10, 2018 06:47

0xc0170 added needs: review and removed needs: work labels Apr 10, 2018

0xc0170 added needs: preceding PR and removed needs: work labels May 18, 2018

cmonr added the release-version: 5.9.1 label May 29, 2018

0xc0170 added needs: work and removed needs: preceding PR labels May 30, 2018

Marc Emmers added 2 commits May 30, 2018 11:36

Only schedule mbed_ticker interrupt if queue->head is changed

b01b173

Reverts change from commit 1057720

Ticker test: Add ticker_irq_handler call because the time reaching th…

046cf1d

…e previous interrupt_timestamp would have triggered one anyway

marcemmers force-pushed the mbed_ticker_api branch from f88dce9 to 046cf1d Compare May 30, 2018 09:43

0xc0170 added needs: review and removed needs: work labels May 30, 2018

c1728p9 approved these changes May 31, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels May 31, 2018

cmonr added ready for merge and removed needs: CI labels Jun 4, 2018

0xc0170 merged commit eddaa8b into ARMmbed:master Jun 5, 2018

0xc0170 removed the ready for merge label Jun 5, 2018

Only schedule mbed_ticker interrupt if queue->head is changed #6515

Only schedule mbed_ticker interrupt if queue->head is changed #6515

Uh oh!

Conversation

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

Uh oh!

Description

Pull request type

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 6, 2018

Uh oh!

mbed-ci commented Apr 6, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 6, 2018

Uh oh!

mbed-ci commented Apr 6, 2018

Uh oh!

0xc0170 commented Apr 9, 2018

Uh oh!

marcemmers commented Apr 9, 2018

Uh oh!

mbed-ci commented Apr 9, 2018

Uh oh!

0xc0170 commented Apr 9, 2018

Uh oh!

marcemmers commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcemmers commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 10, 2018

Uh oh!

mprse commented Apr 10, 2018

Uh oh!

0xc0170 commented Apr 10, 2018

Uh oh!

mbed-ci commented Apr 10, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 10, 2018

Uh oh!

mbed-ci commented Apr 11, 2018

Uh oh!

0xc0170 commented May 30, 2018

Uh oh!

marcemmers commented May 30, 2018

Uh oh!

cmonr commented May 31, 2018

Uh oh!

cmonr commented May 31, 2018

Uh oh!

mbed-ci commented Jun 1, 2018

Triggering tests

Uh oh!

mbed-ci commented Jun 1, 2018

Uh oh!

mbed-ci commented Jun 1, 2018

Uh oh!

maciejbocianski commented Jun 2, 2018

Uh oh!

cmonr commented Jun 2, 2018

Uh oh!

mbed-ci commented Jun 2, 2018

Uh oh!

0xc0170 commented Jun 4, 2018

Uh oh!

0xc0170 commented Jun 4, 2018

Uh oh!

mbed-ci commented Jun 4, 2018

Uh oh!

Uh oh!

marcemmers commented Mar 30, 2018 •

edited

Loading

marcemmers commented Apr 9, 2018 •

edited

Loading

marcemmers commented Apr 10, 2018 •

edited

Loading