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

Ticker: add fire interrupt now function #4644

Merged
merged 3 commits into from Jul 17, 2017

Conversation

Projects
None yet
5 participants
@0xc0170
Member

0xc0170 commented Jun 27, 2017

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 0xc0170 force-pushed the 0xc0170:fix_ticker_delta_negative branch 2 times, most recently Jun 27, 2017

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_ticker_delta_negative branch Jun 27, 2017

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

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_ticker_delta_negative branch 6 times, most recently Jun 28, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 29, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 29, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 682

Build failed!

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_ticker_delta_negative branch Jun 29, 2017

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

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_ticker_delta_negative branch Jun 30, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 30, 2017

Rebased ! Waiting for review

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 10, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 10, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 753

Test Prep failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 10, 2017

Jenkins java error, will need a restart

/morph test-nightly

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 10, 2017

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

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 10, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 756

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2017

/morph test

@c1728p9

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

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

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_ticker_delta_negative branch Jul 12, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 12, 2017

Result: ABORTED

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

/morph test

Output

mbed Build Number: 791

Test failed!

TESTS/mbed_hal/ticker/main.cpp Outdated
}
}
static void test_set_interrupt_past_time()

This comment has been minimized.

@pan-

pan- Jul 13, 2017

Member

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 .

TESTS/mbed_hal/ticker/main.cpp Outdated
interface_stub.interface.read = ticker_interface_stub_read_interrupt_time;
ticker_event_t event = { 0 };
const timestamp_t event_timestamp = interface_stub.timestamp;

This comment has been minimized.

@pan-

pan- Jul 13, 2017

Member

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

This comment has been minimized.

@0xc0170

0xc0170 Jul 13, 2017

Member

Done, added

0xc0170 added some commits Jun 27, 2017

Ticker: add fire interrupt now function
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.
ticker test: add test for set interrupt with timestamp in the past
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 0xc0170 force-pushed the 0xc0170:fix_ticker_delta_negative branch to 56cb158 Jul 13, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 13, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jul 13, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 13, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jul 13, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 17, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 17, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jul 17, 2017

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

@pan-

This comment has been minimized.

Member

pan- commented Jul 17, 2017

Pretty happy on my side if it goes in 5.6.

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

5 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
ci/morph-test-nightly 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