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

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

Merged
merged 2 commits into from Jun 5, 2018

Conversation

Projects
None yet
8 participants
@marcemmers
Contributor

marcemmers commented Mar 30, 2018

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

@0xc0170 0xc0170 requested a review from pan- Apr 3, 2018

@0xc0170 0xc0170 requested a review from c1728p9 Apr 3, 2018

@pan-

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 6, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 6, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 9, 2018

/morph test
/morph mbed2-build

@marcemmers

This comment has been minimized.

Contributor

marcemmers commented Apr 9, 2018

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

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 9, 2018

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?

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

@marcemmers

This comment has been minimized.

Contributor

marcemmers commented Apr 9, 2018

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 marcemmers dismissed stale reviews from c1728p9 and pan- via f88dce9 Apr 10, 2018

@marcemmers

This comment has been minimized.

Contributor

marcemmers commented Apr 10, 2018

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 0xc0170 added needs: review and removed needs: work labels Apr 10, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 10, 2018

@marcemmers Thanks for the update, we will review

@mprse

This comment has been minimized.

Member

mprse commented Apr 10, 2018

@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

This comment has been minimized.

Member

0xc0170 commented Apr 10, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 10, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 30, 2018

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

marcemmers added some commits Mar 30, 2018

Ticker test: Add ticker_irq_handler call because the time reaching th…
…e previous interrupt_timestamp would have triggered one anyway

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

@marcemmers

This comment has been minimized.

Contributor

marcemmers commented May 30, 2018

@0xc0170 Should be ok now?

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 31, 2018

@pan- @c1728p9 All good?

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 31, 2018

/morph build

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

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 1, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Jun 2, 2018

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

This comment has been minimized.

Contributor

cmonr commented Jun 2, 2018

@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

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 4, 2018

/morph export-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 4, 2018

/morph mbed2-build

@mbed-ci

This comment has been minimized.

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

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

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 918 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10162 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment