Ticker: add fire interrupt now function #4644

0xc0170 · 2017-06-27T11:37:09Z

fire_interrupt function should be used for events in the past. As we have now
64bit timestamp, we can figure out what is in the past, and ask a target to invoke
an interrupt immediately. The previous attemps in the target HAL tickers were not ideal, as it can wrap around easily (16 or 32 bit counters). This new functionality should solve this problem.

The most important bit in here is at https://github.com/ARMmbed/mbed-os/pull/4644/files#diff-5103d7d35f905c001a8c673abbeb73f1. This is generic handling of events in the past, we here know that an event is already in the past, we need to invoke its handler now now, thus this removes any questions in the HAL tickers (is this already in the past).

Naming can be improved, I went ahead with fire_interrupt but set_pending_interrupt might be better? I can refactor.

set_interrupt for tickers in HAL code should not handle anything but the next match interrupt. If it was in the past is handled by the upper layer.

The specification for the fire_interrupts are:

should set pending bit for the ticker interrupt (as soon as possible),
the event we are scheduling is already in the past, and we do not want to skip
any events
no arguments are provided, neither return value, not needed
ticker should be initialized prior calling this function (no need to check if it is already initialized)

All our targets provide this new functionality, removing old misleading if (timestamp is in the past) checks.

This needs testing. We have already proposed ticker port, #4628 . I believe that PR could be updated, based on this requirement and test it.

@pan- @c1728p9 @sg- @bulislaw

Once we agree on the naming and that this is desired, I'll tag any vendor maintainer to review this that we cleaned the set_interrupt properly and fire_interrupt is implemented correctly (most of the implementation is SetPendingIRQ, but some need additional handling).

Some targets do delta calculation in set_interrupt, is that correct? I assume those timers do not have always counting up/down tickers, and thus use delta to set the next capture, I left it untouched, but shall be reviewed.

I expect to do some rebase based on the reviews and CI as this touches too many targets.

0xc0170 · 2017-06-29T06:29:12Z

/morph test

mbed-bot · 2017-06-29T06:57:51Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 682

Build failed!

0xc0170 · 2017-06-30T13:38:43Z

Rebased ! Waiting for review

0xc0170 · 2017-07-10T12:39:20Z

/morph test-nightly

mbed-bot · 2017-07-10T14:02:16Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 753

Test Prep failed!

0xc0170 · 2017-07-10T14:03:46Z

Jenkins java error, will need a restart

/morph test-nightly

theotherjimmy · 2017-07-10T15:58:22Z

@pan- @c1728p9 @sg- @bulislaw Could you review?

mbed-bot · 2017-07-10T20:18:15Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 756

Test failed!

0xc0170 · 2017-07-12T12:08:56Z

@pan- @c1728p9 Please review again

0xc0170 · 2017-07-12T14:01:36Z

/morph test

c1728p9

Thanks for the updates! This looks good to me. Sorry my review took so long.

mbed-bot · 2017-07-12T15:44:36Z

Result: ABORTED

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

/morph test

Output

mbed Build Number: 791

Test failed!

pan- · 2017-07-13T08:38:50Z

TESTS/mbed_hal/ticker/main.cpp

+    }
+} 
+
+static void test_set_interrupt_past_time()


Could you comment what the test do like it is done for other test. The documentation (and test) is supposed to act as a specification of the code behavior .

pan- · 2017-07-13T08:46:54Z

TESTS/mbed_hal/ticker/main.cpp

+    interface_stub.interface.read = ticker_interface_stub_read_interrupt_time;
+
+    ticker_event_t event = { 0 };
+    const timestamp_t event_timestamp = interface_stub.timestamp;


Could you add another test similar to this one where event_timestamp is more than interface_stub.timestamp.

Done, added

fire_interrupt function should be used for events in the past. As we have now 64bit timestamp, we can figure out what is in the past, and ask a target to invoke an interrupt immediately. The previous attemps in the target HAL tickers were not ideal, as it can wrap around easily (16 or 32 bit counters). This new functionality should solve this problem. set_interrupt for tickers in HAL code should not handle anything but the next match interrupt. If it was in the past is handled by the upper layer. It is possible that we are setting next event to the close future, so once it is set it is already in the past. Therefore we add a check after set interrupt to verify it is in future. If it is not, we fire interrupt immediately. This results in two events - first one immediate, correct one. The second one might be scheduled in far future (almost entire ticker range), that should be discarded. The specification for the fire_interrupts are: - should set pending bit for the ticker interrupt (as soon as possible), the event we are scheduling is already in the past, and we do not want to skip any events - no arguments are provided, neither return value, not needed - ticker should be initialized prior calling this function (no need to check if it is already initialized) All our targets provide this new functionality, removing old misleading if (timestamp is in the past) checks.

2 test cases added, one for event in the past, one for event in future but very close to the current time, thus once is set, it is already in the past, and we fire interrupt immediately.

0xc0170 · 2017-07-13T11:24:14Z

Rebased on top of the master, 2 test cases added to test both conditions that can happen for fire interrupt, tested with NRF51_DK, 32 OK tests cases now.

0xc0170 · 2017-07-13T11:24:31Z

/morph test

mbed-bot · 2017-07-13T13:00:32Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 796

All builds and test passed!

0xc0170 · 2017-07-13T13:47:48Z

@pan- Happy with this patch as it is now?

0xc0170 · 2017-07-17T06:32:16Z

/morph test-nightly

mbed-bot · 2017-07-17T14:38:53Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 819

All builds and test passed!

0xc0170 · 2017-07-17T15:47:37Z

Everyone happy with this proposal? All CI passed, proposed to go to 5.6

pan- · 2017-07-17T15:49:12Z

Pretty happy on my side if it goes in 5.6.

0xc0170 added the needs: review label Jun 27, 2017

0xc0170 mentioned this pull request Jun 27, 2017

Add port tests for ticker and lp ticker #4628

Closed

0xc0170 force-pushed the fix_ticker_delta_negative branch 2 times, most recently from 7178bd6 to d98abb5 Compare June 27, 2017 12:48

0xc0170 mentioned this pull request Jun 27, 2017

Fix beetle and lpc176x set ticker event in the past #4633

Closed

0xc0170 force-pushed the fix_ticker_delta_negative branch from d98abb5 to a7ef92f Compare June 27, 2017 15:00

0xc0170 added needs: work and removed needs: review labels Jun 27, 2017

0xc0170 force-pushed the fix_ticker_delta_negative branch 6 times, most recently from dc478cc to 36b33be Compare June 28, 2017 14:34

0xc0170 force-pushed the fix_ticker_delta_negative branch from 36b33be to c87b29b Compare June 29, 2017 07:29

theotherjimmy added needs: review and removed needs: work labels Jun 29, 2017

0xc0170 added needs: work and removed needs: review labels Jun 30, 2017

0xc0170 force-pushed the fix_ticker_delta_negative branch from c87b29b to 17eeb63 Compare June 30, 2017 13:38

0xc0170 added needs: review and removed needs: work labels Jun 30, 2017

c1728p9 approved these changes Jul 12, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Jul 12, 2017

0xc0170 force-pushed the fix_ticker_delta_negative branch from 76e4295 to c80df33 Compare July 12, 2017 15:43

pan- reviewed Jul 13, 2017

View reviewed changes

0xc0170 added 3 commits July 13, 2017 12:23

ticker test: add fire now stub

4ff4329

ticker test: add test for set interrupt with timestamp in the past

56cb158

2 test cases added, one for event in the past, one for event in future but very close to the current time, thus once is set, it is already in the past, and we fire interrupt immediately.

0xc0170 force-pushed the fix_ticker_delta_negative branch from 7f100b9 to 56cb158 Compare July 13, 2017 11:23

0xc0170 added the release-version: 5.6.0 label Jul 13, 2017

theotherjimmy added ready for merge and removed needs: CI labels Jul 17, 2017

theotherjimmy merged commit adfed0f into ARMmbed:master Jul 17, 2017

theotherjimmy removed the ready for merge label Jul 17, 2017

adbridge mentioned this pull request Aug 25, 2017

STM32: Align HAL & US tickers #4809

Merged

This was referenced Sep 4, 2017

Ticker test fails when run under Greentea on NUCLEO_F070RB #5004

Closed

STM32_us_ticker_16b: keep code to cope with past event #5012

Merged

c1728p9 mentioned this pull request Dec 13, 2017

Timing tests are unstable on the ARM_BEETLE_SOC #4605

Closed

Ticker: add fire interrupt now function #4644

Ticker: add fire interrupt now function #4644

Conversation

0xc0170 commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jun 29, 2017

Uh oh!

mbed-bot commented Jun 29, 2017

Output

Uh oh!

0xc0170 commented Jun 30, 2017

Uh oh!

0xc0170 commented Jul 10, 2017

Uh oh!

mbed-bot commented Jul 10, 2017

Output

Uh oh!

0xc0170 commented Jul 10, 2017

Uh oh!

theotherjimmy commented Jul 10, 2017

Uh oh!

mbed-bot commented Jul 10, 2017

Output

Uh oh!

0xc0170 commented Jul 12, 2017

Uh oh!

0xc0170 commented Jul 12, 2017

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

mbed-bot commented Jul 12, 2017

Output

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jul 13, 2017

Uh oh!

0xc0170 commented Jul 13, 2017

Uh oh!

mbed-bot commented Jul 13, 2017

Output

Uh oh!

0xc0170 commented Jul 13, 2017

Uh oh!

0xc0170 commented Jul 17, 2017

Uh oh!

mbed-bot commented Jul 17, 2017

Output

Uh oh!

0xc0170 commented Jul 17, 2017

Uh oh!

pan- commented Jul 17, 2017

Uh oh!

Uh oh!

0xc0170 commented Jun 27, 2017 •

edited

Loading