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

Add sleep manager API #4912

Merged
merged 9 commits into from Sep 7, 2017

Conversation

Projects
None yet
10 participants
@0xc0170
Member

0xc0170 commented Aug 15, 2017

This goes to feature-hal-spec, based on the sleep manager work that is here #4882 . Review only the last commit (at the moment SHA 34f9f43). This goes to feature-hal-spec branch = work in progress

To move further with sleep, I am sending this early to get feedback.

The goal is to have deepsleep locks for those drivers that depend on high speed clocks (it can be that some drivers for a target might support deep sleep but this is not usual, plus speed is limited often).

@bulislaw @c1728p9 @pan-

@@ -115,9 +115,11 @@ int CAN::filter(unsigned int id, unsigned int mask, CANFormat format, int handle
void CAN::attach(Callback<void()> func, IrqType type) {

This comment has been minimized.

@c1728p9

c1728p9 Aug 16, 2017

Contributor

Attach could also be called to replace a callback of the same type. If this is the case then deep sleep shouldn't be locked a second time.

drivers/SerialBase.cpp Outdated
@@ -73,9 +76,11 @@ void SerialBase::attach(Callback<void()> func, IrqType type) {
// Disable interrupts when attaching interrupt handler
core_util_critical_section_enter();
if (func) {
sleep_manager_lock_deep_sleep();

This comment has been minimized.

@c1728p9

c1728p9 Aug 16, 2017

Contributor

Attach could also be called to replace a callback of the same type. If this is the case then deep sleep shouldn't be locked a second time.

drivers/Ticker.cpp Outdated
@@ -26,6 +26,7 @@ void Ticker::detach() {
core_util_critical_section_enter();
remove();
_function = 0;
sleep_manager_unlock_deep_sleep();

This comment has been minimized.

@c1728p9

c1728p9 Aug 16, 2017

Contributor

Detach could be called while already detached (like during object destruction) which could cause an underflow.

drivers/Timer.cpp Outdated
@@ -41,6 +42,7 @@ void Timer::stop() {
core_util_critical_section_enter();
_time += slicetime();
_running = 0;
sleep_manager_unlock_deep_sleep();

This comment has been minimized.

@c1728p9

c1728p9 Aug 16, 2017

Contributor

Unlock should only be called if the timer was running.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 17, 2017

@c1728p9 Added fixes for the last review.

@0xc0170 0xc0170 force-pushed the 0xc0170:dev_sleep_drivers branch 2 times, most recently to 18ed12c Aug 17, 2017

@@ -125,6 +125,7 @@ void I2C::unlock() {
int I2C::transfer(int address, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length, const event_callback_t& callback, int event, bool repeated)
{
lock();
sleep_manager_lock_deep_sleep();
if (i2c_active(&_i2c)) {

This comment has been minimized.

@c1728p9

c1728p9 Aug 17, 2017

Contributor

If I2C is already active, you aren't starting a new transfer. You should probably either unlock before returning or add the lock until after the active check.

_irq[(CanIrqType)type] = func;
can_irq_set(&_can, (CanIrqType)type, 1);
} else {
sleep_manager_unlock_deep_sleep();

This comment has been minimized.

@c1728p9

c1728p9 Aug 17, 2017

Contributor

Same problem could occur with the else case - it is valid to call attach with null multiple times in a row. Maybe as a general strategy, each interrupt type could be locked when donothing is replaced, and unlocked when donothing is set. A similar change should also be made for SerialBase.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 18, 2017

@c1728p9 Fixed, I'll rebase once all approved to clean the history

@0xc0170 0xc0170 force-pushed the 0xc0170:dev_sleep_drivers branch to e0048cb Aug 20, 2017

@0xc0170 0xc0170 changed the title from Drivers: add sleep locks (feature-hal-spec branch) to Add sleep manager API Aug 21, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

This now should be feature complete (testing ongoing, plus reviews so expect me to rebase). I rebased it , so the first comment is not relevant anymore - this is now adding these:

  • add sleep manager API
  • deep sleep locks in drivers (Serial, SPI, etc)
  • rtos idle loop locks deep sleep (backward compability) for now

It should be good to go - being backward compatible plus adding new feature that we will use with upcoming tickless addition.

This is still against feature hal spec branch. I'll send tests as separate patch soon.

Quickly tested with rtos tests for K64F and GCC ARM - all OK

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

retest uvisor

@0xc0170 0xc0170 referenced this pull request Aug 21, 2017

Closed

Add tickless feature #4943

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 22, 2017

Result: FAILURE

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

/morph test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 22, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1077

Build Prep failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 22, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 22, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1078

All builds and test passed!

@0xc0170 0xc0170 force-pushed the 0xc0170:dev_sleep_drivers branch Aug 23, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2017

I added DeepSleepLock RAII object . One thing to consider - it is currently just lock/unlock on aquicistion/release. But to be usable also with async, we might need to add also lock/unlock methods. This however might require more additions to be completed ? For instance, should it track pair lock/unlock, thus when destroying, it would clean up ?

Please review

drivers/CAN.h Outdated
@@ -24,6 +24,7 @@
#include "platform/Callback.h"
#include "platform/PlatformMutex.h"
#include "platform/NonCopyable.h"
#include "platform/mbed_sleep.h"

This comment has been minimized.

@nvlsianpu

nvlsianpu Aug 28, 2017

Contributor

Is this intentionally propagation of "mbed_sleep.h" inclusion to any module which using the driver? If not this should be moved to the cpp file of the driver instead. This remark is up to all drivers headers in this patch.

This comment has been minimized.

@0xc0170

0xc0170 Aug 29, 2017

Member

@nvlsianpu thanks for the review, the PR updated , all should be fixed.

hal/mbed_sleep_manager.c Outdated
bool sleep_manager_can_deep_sleep(void)
{
core_util_critical_section_enter();

This comment has been minimized.

@nvlsianpu

nvlsianpu Aug 28, 2017

Contributor

Critical section is not required for read of natively supported integer (uint16). It only extend call execution time.

* void f() {
* // some code here
* {
* DeepSleepLock lock;

This comment has been minimized.

@nvlsianpu

nvlsianpu Aug 28, 2017

Contributor

Dose such a object will not been cutted out as unused object?

This comment has been minimized.

@nvlsianpu

nvlsianpu Aug 28, 2017

Contributor

It looks like it is guaranteed by cpp object life cycle behaviour. cpp reference

@0xc0170 0xc0170 force-pushed the 0xc0170:dev_sleep_drivers branch Aug 29, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 30, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 30, 2017

Result: ABORTED

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

/morph test

Output

mbed Build Number: 1142

Build failed!

0xc0170 added some commits Aug 18, 2017

drivers: fix attach sleep locking
attach/detach can be multiple invoked. Therefore lock/unlock deep sleep
only for the very first time it is invoked (when callbacks
are actually changed).
platform: add DeepSleepLock
RAII object for disabling, then restoring the deep sleep mode
test: add sleep manager tests
This commits contains two tests:
- race condition
- deep sleep locking/unlocking
idle loop: add deepsleep locks
For backward compability, we invoke just sleep mode. Adding critical section to complete
lock/unlock deepsleep section.
SerialBase and CAN: fix Callbacks comparision
As Callback currently does not have fully functional comparision (see #5017),
we workaround by doing null check.

@0xc0170 0xc0170 force-pushed the 0xc0170:dev_sleep_drivers branch to f6c34a2 Sep 7, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 7, 2017

Rebased (removed HAL docs changes, will come as separate PR), tested with nucloe f429, all good.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 7, 2017

/morph test

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 7, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 7, 2017

Awesome! LGTM.

@@ -102,6 +103,10 @@ class Ticker : public TimerEvent, private NonCopyable<Ticker> {
* @param t the time between calls in micro-seconds
*/
void attach_us(Callback<void()> func, us_timestamp_t t) {
// lock only for the initial callback setup
if (!_function) {
sleep_manager_lock_deep_sleep();

This comment has been minimized.

@c1728p9

c1728p9 Sep 7, 2017

Contributor

How does the sleep manager get unlocked?

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 7, 2017

Contributor

Generally, sleep_manager_unlock_deep_sleep, but you're asking for the specific unlock for this call to lock right?

@@ -40,6 +41,9 @@ void Timer::start() {
void Timer::stop() {
core_util_critical_section_enter();
_time += slicetime();
if (_running) {
sleep_manager_unlock_deep_sleep();

This comment has been minimized.

@c1728p9

c1728p9 Sep 7, 2017

Contributor

You might want to unlock this on object destruction.

/** Mark the start of a locked deep sleep section
*/
void lock()

This comment has been minimized.

@c1728p9

c1728p9 Sep 7, 2017

Contributor

Is there any reason to expose lock and unlock? This brings back the risks of forgetting to balance locks and unlocks.

@c1728p9

c1728p9 approved these changes Sep 7, 2017

Looks good to me. There are a couple of things that should be addressed after this is merged.

@theotherjimmy theotherjimmy merged commit e12f116 into ARMmbed:master Sep 7, 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
@sg-

This comment has been minimized.

Member

sg- commented Sep 8, 2017

Nice change removing the donothing. Looks like there is one straggler left in the codebase. https://github.com/ARMmbed/mbed-os/search?utf8=%E2%9C%93&q=callback%28donothing%29&type=

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 9, 2017

Nice change removing the donothing. Looks like there is one straggler left in the codebase. https://github.com/ARMmbed/mbed-os/search?utf8=%E2%9C%93&q=callback%28donothing%29&type=

It was left there as donothing replacement in this PR was required for making new API working (compare if a callback was attached or not). Thanks for checking ! Yes, I'll send PR shortly to align also InterruptIn

@0xc0170 0xc0170 deleted the 0xc0170:dev_sleep_drivers branch Sep 9, 2017

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Sep 20, 2017

Hi
In tests-mbed_drivers-lp_timeout and tests-mbed_hal-lp_ticker, we are still using deepsleep function which is deprecated.
Maybe you should update these tests ?
Thx

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

In tests-mbed_drivers-lp_timeout and tests-mbed_hal-lp_ticker, we are still using deepsleep function which is deprecated.
Maybe you should update these tests ?

Yes, I'll do

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Oct 11, 2017

Apparently looks like this path broke nrf52_Dk (nrf52840 as well) spi (tests-api-spi). Commit cb4e9b3 introduced such a change. Even if i comment off sleep instruction it still failed.

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Oct 11, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 11, 2017

@nvlsianpu I do not fully get even if you comment out sleep, then it still fails? Can you create an issue with details, I would like to reproduce it locally .

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Oct 11, 2017

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