From f0ac234da7751e0876399348c16b79e4fb1e0f62 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 28 Sep 2017 16:20:15 -0500 Subject: [PATCH 1/4] Prevent DeepSleepLock from leaving sleep locked Add _lock_count to DeepSleepLock and use this to prevent deep sleep from staying locked when the DeepSleepLock objected is destroyed after an unbalanced number of calls to lock and unlock. --- platform/DeepSleepLock.h | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/platform/DeepSleepLock.h b/platform/DeepSleepLock.h index ff5149d2e37..17abe15614f 100644 --- a/platform/DeepSleepLock.h +++ b/platform/DeepSleepLock.h @@ -16,7 +16,9 @@ #ifndef MBED_DEEPSLEEPLOCK_H #define MBED_DEEPSLEEPLOCK_H +#include #include "platform/mbed_sleep.h" +#include "platform/mbed_critical.h" namespace mbed { @@ -36,29 +38,47 @@ namespace mbed { * @endcode */ class DeepSleepLock { +private: + uint16_t _lock_count; + public: - DeepSleepLock() + DeepSleepLock(): _lock_count(1) { sleep_manager_lock_deep_sleep(); } ~DeepSleepLock() { - sleep_manager_unlock_deep_sleep(); + if (_lock_count) { + sleep_manager_unlock_deep_sleep(); + } } /** Mark the start of a locked deep sleep section */ void lock() { - sleep_manager_lock_deep_sleep(); + uint16_t count = core_util_atomic_incr_u16(&_lock_count, 1); + if (1 == count) { + sleep_manager_lock_deep_sleep(); + } + if (0 == count) { + error("DeepSleepLock overflow (> USHRT_MAX)"); + } } /** Mark the end of a locked deep sleep section */ void unlock() { - sleep_manager_unlock_deep_sleep(); + uint16_t count = core_util_atomic_decr_u16(&_lock_count, 1); + if (count == 0) { + sleep_manager_unlock_deep_sleep(); + } + if (count == USHRT_MAX) { + core_util_critical_section_exit(); + error("DeepSleepLock underflow (< 0)"); + } } }; From ea468856c49714ffd655cd76e2e657d1de71e646 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 28 Sep 2017 16:30:57 -0500 Subject: [PATCH 2/4] Add a test to validate locking Test that DeepSleepLock and Timer lock deep sleep at the correct time and do no leave sleep locked after they are destroted. --- TESTS/mbed_drivers/sleep_lock/main.cpp | 132 +++++++++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 TESTS/mbed_drivers/sleep_lock/main.cpp diff --git a/TESTS/mbed_drivers/sleep_lock/main.cpp b/TESTS/mbed_drivers/sleep_lock/main.cpp new file mode 100644 index 00000000000..2e1bcbd80a2 --- /dev/null +++ b/TESTS/mbed_drivers/sleep_lock/main.cpp @@ -0,0 +1,132 @@ + +/* mbed Microcontroller Library + * Copyright (c) 2017 ARM Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#if !DEVICE_SLEEP + #error [NOT_SUPPORTED] Sleep not supported for this target +#endif + +#include "utest/utest.h" +#include "unity/unity.h" +#include "greentea-client/test_env.h" + +#include "mbed.h" + +using namespace utest::v1; + +void deep_sleep_lock_lock_test() +{ + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Check basic usage works + DeepSleepLock lock; + TEST_ASSERT_EQUAL(false, sleep_manager_can_deep_sleep()); + } + + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Check that unlock and lock change can deep sleep as expected + DeepSleepLock lock; + TEST_ASSERT_EQUAL(false, sleep_manager_can_deep_sleep()); + lock.unlock(); + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + lock.lock(); + TEST_ASSERT_EQUAL(false, sleep_manager_can_deep_sleep()); + } + + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Check that unlock releases sleep based on count + DeepSleepLock lock; + lock.lock(); + lock.lock(); + lock.unlock(); + TEST_ASSERT_EQUAL(false, sleep_manager_can_deep_sleep()); + } + + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Check that unbalanced locks do not leave deep sleep locked + DeepSleepLock lock; + lock.lock(); + TEST_ASSERT_EQUAL(false, sleep_manager_can_deep_sleep()); + } + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + +} + +void timer_lock_test() +{ + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Just creating a timer object does not lock sleep + Timer timer; + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + } + + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Starting a timer does lock sleep + Timer timer; + timer.start(); + TEST_ASSERT_EQUAL(false, sleep_manager_can_deep_sleep()); + } + + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Stopping a timer after starting it allows sleep + Timer timer; + timer.start(); + timer.stop(); + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + } + + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Starting a timer multiple times still lets you sleep + Timer timer; + timer.start(); + timer.start(); + } + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + { + // Stopping a timer multiple times still lets you sleep + Timer timer; + timer.start(); + timer.stop(); + timer.stop(); + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); + } + TEST_ASSERT_EQUAL(true, sleep_manager_can_deep_sleep()); +} + +Case cases[] = { + Case("DeepSleepLock lock test", deep_sleep_lock_lock_test), + Case("timer lock test", timer_lock_test), +}; + +utest::v1::status_t greentea_test_setup(const size_t number_of_cases) { + GREENTEA_SETUP(20, "default_auto"); + return greentea_test_setup_handler(number_of_cases); +} + +Specification specification(greentea_test_setup, cases, greentea_test_teardown_handler); + +int main() { + Harness::run(specification); +} From 87b151c8a02b58e4befb9d405991d30c267d05c7 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 28 Sep 2017 16:36:15 -0500 Subject: [PATCH 3/4] Protect Ticker attach with a critical section Add a critical section to attach_us so setting _function and locking deep sleep are done atomically. --- drivers/Ticker.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/Ticker.h b/drivers/Ticker.h index cc5e5dd9e04..c8f1f9deb33 100644 --- a/drivers/Ticker.h +++ b/drivers/Ticker.h @@ -22,6 +22,7 @@ #include "platform/NonCopyable.h" #include "platform/mbed_sleep.h" #include "hal/lp_ticker_api.h" +#include "platform/mbed_critical.h" namespace mbed { /** \addtogroup drivers */ @@ -113,12 +114,14 @@ class Ticker : public TimerEvent, private NonCopyable { * */ void attach_us(Callback func, us_timestamp_t t) { + core_util_critical_section_enter(); // lock only for the initial callback setup and this is not low power ticker if(!_function && _lock_deepsleep) { sleep_manager_lock_deep_sleep(); } _function = func; setup(t); + core_util_critical_section_exit(); } /** Attach a member function to be called by the Ticker, specifying the interval in micro-seconds From f854f3e6db95b4676e0aa1b1e7cc65d23d3019cd Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 28 Sep 2017 16:43:03 -0500 Subject: [PATCH 4/4] Properly unlock sleep in destructor of drivers Unlock sleep in CAN and SerialBase. This prevents deep sleep from staying locked if these objects are destroyed without first clearing the callbacks. --- drivers/CAN.cpp | 5 +++++ drivers/SerialBase.cpp | 10 ++++++++++ drivers/SerialBase.h | 3 +-- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/CAN.cpp b/drivers/CAN.cpp index e49b7adfcb7..b84d6082cee 100644 --- a/drivers/CAN.cpp +++ b/drivers/CAN.cpp @@ -46,6 +46,11 @@ CAN::CAN(PinName rd, PinName td, int hz) : _can(), _irq() { CAN::~CAN() { // No lock needed in destructor + + // Detaching interrupts releases the sleep lock if it was locked + for (int irq = 0; irq < IrqCnt; irq++) { + attach(NULL, (IrqType)irq); + } can_irq_free(&_can); can_free(&_can); } diff --git a/drivers/SerialBase.cpp b/drivers/SerialBase.cpp index e07a44149dd..5ec47ff83a6 100644 --- a/drivers/SerialBase.cpp +++ b/drivers/SerialBase.cpp @@ -133,6 +133,16 @@ void SerialBase:: unlock() { // Stub } +SerialBase::~SerialBase() +{ + // No lock needed in destructor + + // Detaching interrupts releases the sleep lock if it was locked + for (int irq = 0; irq < IrqCnt; irq++) { + attach(NULL, (IrqType)irq); + } +} + #if DEVICE_SERIAL_FC void SerialBase::set_flow_control(Flow type, PinName flow1, PinName flow2) { lock(); diff --git a/drivers/SerialBase.h b/drivers/SerialBase.h index 835e1a33b18..faf29a06998 100644 --- a/drivers/SerialBase.h +++ b/drivers/SerialBase.h @@ -241,8 +241,7 @@ class SerialBase : private NonCopyable { protected: SerialBase(PinName tx, PinName rx, int baud); - virtual ~SerialBase() { - } + virtual ~SerialBase(); int _base_getc(); int _base_putc(int c);