cellular: AT Handler API changes after review

UNITTESTS/features/cellular/framework/AT/at_cellularbase/unittest.cmake

@@ -16,6 +16,7 @@ set(unittest-includes ${unittest-includes}
 # Source files
 set(unittest-sources
   ../features/cellular/framework/AT/AT_CellularBase.cpp
+  ../features/cellular/framework/AT/ATHandler_factory.cpp
 )
 
 # Test files

UNITTESTS/features/cellular/framework/AT/at_cellularcontext/unittest.cmake

@@ -14,6 +14,7 @@ set(unittest-includes ${unittest-includes}
 # Source files
 set(unittest-sources
   ../features/cellular/framework/AT/AT_CellularContext.cpp
+  ../features/cellular/framework/AT/ATHandler_factory.cpp
   ../features/cellular/framework/common/CellularUtil.cpp
 )
 

UNITTESTS/features/cellular/framework/AT/at_cellulardevice/unittest.cmake

@@ -18,6 +18,7 @@ set(unittest-includes ${unittest-includes}
 set(unittest-sources
   stubs/randLIB_stub.c
   ../features/cellular/framework/AT/AT_CellularDevice.cpp
+  ../features/cellular/framework/AT/ATHandler_factory.cpp
 )
 
 # Test files

UNITTESTS/features/cellular/framework/AT/athandler/athandlertest.cpp

@@ -86,23 +86,36 @@ TEST_F(TestATHandler, test_ATHandler_set_file_handle)
     at.set_file_handle(&fh2);
 }
 
-TEST_F(TestATHandler, test_ATHandler_get_release)
+TEST_F(TestATHandler, test_ATHandler_list)
 {
     EventQueue que;
     FileHandle_stub fh1;
 
-    ATHandler *at1 = ATHandler::get(&fh1, que, 0, ",", 0, 0);
+    ATHandler::set_at_timeout_list(1000, false);
-    EXPECT_TRUE(at1->get_ref_count() == 1);
+    ATHandler::set_debug_list(false);
-    EXPECT_TRUE(ATHandler::get(NULL, que, 0, ",", 0, 0) == NULL);
 
-    ATHandler *at2 = ATHandler::get(&fh1, que, 0, ",", 0, 0);
+    ATHandler *at1 = ATHandler::get_instance(&fh1, que, 0, ",", 0, 0);
+    EXPECT_TRUE(at1->get_ref_count() == 1);
+
+    ATHandler::set_at_timeout_list(1000, false);
+    ATHandler::set_debug_list(true);
+
+    EXPECT_TRUE(ATHandler::get_instance(NULL, que, 0, ",", 0, 0) == NULL);
+
+    ATHandler *at2 = ATHandler::get_instance(&fh1, que, 0, ",", 0, 0);
     EXPECT_TRUE(at1->get_ref_count() == 2);
     EXPECT_TRUE(at2->get_ref_count() == 2);
 
-    EXPECT_TRUE(ATHandler::release(at1) == NSAPI_ERROR_OK);
+    ATHandler::set_at_timeout_list(2000, true);
+    ATHandler::set_debug_list(false);
+
+    EXPECT_TRUE(at1->close() == NSAPI_ERROR_OK);
     EXPECT_TRUE(at2->get_ref_count() == 1);
-    EXPECT_TRUE(ATHandler::release(at2) == NSAPI_ERROR_OK);
+    EXPECT_TRUE(at2->close() == NSAPI_ERROR_OK);
-    EXPECT_TRUE(ATHandler::release(NULL) == NSAPI_ERROR_PARAMETER);
+    EXPECT_TRUE(at1->close() == NSAPI_ERROR_PARAMETER);
+
+    ATHandler::set_at_timeout_list(1000, false);
+    ATHandler::set_debug_list(false);
 }
 
 TEST_F(TestATHandler, test_ATHandler_lock)

UNITTESTS/features/cellular/framework/AT/athandler/unittest.cmake

@@ -14,6 +14,7 @@ set(unittest-includes ${unittest-includes}
 # Source files
 set(unittest-sources
   ../features/cellular/framework/AT/ATHandler.cpp
+  ../features/cellular/framework/AT/ATHandler_factory.cpp
   ../features/cellular/framework/common/CellularUtil.cpp
 )
 

UNITTESTS/stubs/ATHandler_stub.cpp

@@ -57,7 +57,7 @@ int ATHandler_stub::urc_amount = 0;
 mbed::Callback<void()> ATHandler_stub::callback[kATHandler_urc_table_max_size];
 char *ATHandler_stub::urc_string_table[kATHandler_urc_table_max_size];
 
-ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, int timeout, const char *output_delimiter, uint16_t send_delay) :
+ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay) :
     _nextATHandler(0),
     _fileHandle(fh),
     _queue(queue),
@@ -73,8 +73,6 @@ ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, int timeout, const char
         ATHandler_stub::urc_string_table[i++] = NULL;
     }
 }
-ATHandler *ATHandler::_atHandlers = NULL;
-PlatformMutex ATHandler::_getReleaseMutex;
 
 void ATHandler::set_debug(bool debug_on)
 {
@@ -123,68 +121,6 @@ void ATHandler::set_file_handle(FileHandle *fh)
 {
 }
 
-// each parser is associated with one filehandle (that is UART)
-ATHandler *ATHandler::get(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
-                          const char *delimiter, uint16_t send_delay, bool debug_on)
-{
-    if (!fileHandle) {
-        return NULL;
-    }
-
-    _getReleaseMutex.lock();
-    ATHandler *atHandler = _atHandlers;
-    while (atHandler) {
-        if (atHandler->get_file_handle() == fileHandle) {
-            atHandler->inc_ref_count();
-            _getReleaseMutex.unlock();
-            return atHandler;
-        }
-        atHandler = atHandler->_nextATHandler;
-    }
-
-    atHandler = new ATHandler(fileHandle, queue, timeout, delimiter, send_delay);
-    if (debug_on) {
-        atHandler->set_debug(debug_on);
-    }
-    atHandler->_nextATHandler = _atHandlers;
-    _atHandlers = atHandler;
-
-    _getReleaseMutex.unlock();
-    return atHandler;
-}
-
-nsapi_error_t ATHandler::release(ATHandler *at_handler)
-{
-    if (!at_handler || at_handler->get_ref_count() == 0) {
-        return NSAPI_ERROR_PARAMETER;
-    }
-
-    _getReleaseMutex.lock();
-    at_handler->dec_ref_count();
-    if (at_handler->get_ref_count() == 0) {
-        // we can delete this at_handler
-        ATHandler *atHandler = _atHandlers;
-        ATHandler *prev = NULL;
-        while (atHandler) {
-            if (atHandler == at_handler) {
-                if (prev == NULL) {
-                    _atHandlers = _atHandlers->_nextATHandler;
-                } else {
-                    prev->_nextATHandler = atHandler->_nextATHandler;
-                }
-                delete atHandler;
-                at_handler = NULL;
-                break;
-            } else {
-                prev = atHandler;
-                atHandler = atHandler->_nextATHandler;
-            }
-        }
-    }
-    _getReleaseMutex.unlock();
-    return NSAPI_ERROR_OK;
-}
-
 nsapi_error_t ATHandler::set_urc_handler(const char *urc, mbed::Callback<void()> cb)
 {
     if (ATHandler_stub::urc_amount < kATHandler_urc_table_max_size) {

UNITTESTS/stubs/AT_CellularDevice_stub.cpp

@@ -35,7 +35,7 @@ AT_CellularDevice::~AT_CellularDevice()
 
 ATHandler *AT_CellularDevice::get_at_handler(FileHandle *fileHandle)
 {
-    return ATHandler::get(fileHandle, _queue, _default_timeout, "\r", get_send_delay(), _modem_debug_on);
+    return ATHandler::get_instance(fileHandle, _queue, _default_timeout, "\r", get_send_delay(), _modem_debug_on);
 }
 
 ATHandler *AT_CellularDevice::get_at_handler()
@@ -45,7 +45,11 @@ ATHandler *AT_CellularDevice::get_at_handler()
 
 nsapi_error_t AT_CellularDevice::release_at_handler(ATHandler *at_handler)
 {
-    return ATHandler::release(at_handler);
+    if (at_handler) {
+        return at_handler->close();
+    } else {
+        return NSAPI_ERROR_PARAMETER;
+    }
 }
 
 CellularContext *create_context(FileHandle *fh = NULL, const char *apn = MBED_CONF_NSAPI_DEFAULT_CELLULAR_APN)
@@ -60,7 +64,7 @@ void delete_context(CellularContext *context)
 
 CellularNetwork *AT_CellularDevice::open_network(FileHandle *fh)
 {
-    return new AT_CellularNetwork(*ATHandler::get(fh, _queue, _default_timeout, "\r", get_send_delay(), _modem_debug_on));
+    return new AT_CellularNetwork(*ATHandler::get_instance(fh, _queue, _default_timeout, "\r", get_send_delay(), _modem_debug_on));
 }
 
 CellularSMS *AT_CellularDevice::open_sms(FileHandle *fh)

features/cellular/framework/AT/ATHandler.cpp

@@ -60,7 +60,7 @@ static const uint8_t map_3gpp_errors[][2] =  {
     { 146, 46 }, { 178, 65 }, { 179, 66 }, { 180, 48 }, { 181, 83 }, { 171, 49 },
 };
 
-ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, int timeout, const char *output_delimiter, uint16_t send_delay) :
+ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay) :
     _nextATHandler(0),
     _fileHandle(fh),
     _queue(queue),
@@ -110,8 +110,6 @@ ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, int timeout, const char
 
     set_file_handle(fh);
 }
-ATHandler *ATHandler::_atHandlers = NULL;
-PlatformMutex ATHandler::_getReleaseMutex;
 
 void ATHandler::set_debug(bool debug_on)
 {
@@ -162,69 +160,6 @@ void ATHandler::set_is_filehandle_usable(bool usable)
     _is_fh_usable = usable;
 }
 
-
-// each parser is associated with one filehandle (that is UART)
-ATHandler *ATHandler::get(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
-                          const char *delimiter, uint16_t send_delay, bool debug_on)
-{
-    if (!fileHandle) {
-        return NULL;
-    }
-
-    _getReleaseMutex.lock();
-    ATHandler *atHandler = _atHandlers;
-    while (atHandler) {
-        if (atHandler->get_file_handle() == fileHandle) {
-            atHandler->inc_ref_count();
-            _getReleaseMutex.unlock();
-            return atHandler;
-        }
-        atHandler = atHandler->_nextATHandler;
-    }
-
-    atHandler = new ATHandler(fileHandle, queue, timeout, delimiter, send_delay);
-    if (debug_on) {
-        atHandler->set_debug(debug_on);
-    }
-    atHandler->_nextATHandler = _atHandlers;
-    _atHandlers = atHandler;
-
-    _getReleaseMutex.unlock();
-    return atHandler;
-}
-
-nsapi_error_t ATHandler::release(ATHandler *at_handler)
-{
-    if (!at_handler || at_handler->get_ref_count() == 0) {
-        return NSAPI_ERROR_PARAMETER;
-    }
-
-    _getReleaseMutex.lock();
-    at_handler->dec_ref_count();
-    if (at_handler->get_ref_count() == 0) {
-        // we can delete this at_handler
-        ATHandler *atHandler = _atHandlers;
-        ATHandler *prev = NULL;
-        while (atHandler) {
-            if (atHandler == at_handler) {
-                if (prev == NULL) {
-                    _atHandlers = _atHandlers->_nextATHandler;
-                } else {
-                    prev->_nextATHandler = atHandler->_nextATHandler;
-                }
-                delete atHandler;
-                at_handler = NULL;
-                break;
-            } else {
-                prev = atHandler;
-                atHandler = atHandler->_nextATHandler;
-            }
-        }
-    }
-    _getReleaseMutex.unlock();
-    return NSAPI_ERROR_OK;
-}
-
 nsapi_error_t ATHandler::set_urc_handler(const char *prefix, mbed::Callback<void()> callback)
 {
     if (find_urc_handler(prefix)) {

features/cellular/framework/AT/ATHandler.h

@@ -74,7 +74,7 @@ public:
      *  @param output_delimiter delimiter used when parsing at responses, "\r" should be used as output_delimiter
      *  @param send_delay       the minimum delay in ms between the end of last response and the beginning of a new command
      */
-    ATHandler(FileHandle *fh, events::EventQueue &queue, int timeout, const char *output_delimiter, uint16_t send_delay = 0);
+    ATHandler(FileHandle *fh, events::EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay = 0);
     virtual ~ATHandler();
 
     /** Return used file handle.
@@ -83,9 +83,14 @@ public:
      */
     FileHandle *get_file_handle();
 
-    /** Get a new ATHandler instance, and update the linked list
+    /** Get a new ATHandler instance, and update the linked list. Once the use of the ATHandler
+     *  has finished, call to close() has to be made
      *
-     *  @param fileHandle       filehandle used for reading AT responses and writing AT commands
+     *  @param fileHandle       filehandle used for reading AT responses and writing AT commands.
+     *                          If there is already an ATHandler with the same fileHandle pointer,
+     *                          then a pointer to that ATHandler instance will be returned with
+     *                          that ATHandler's queue, timeout, delimiter, send_delay and debug_on
+     *                          values
      *  @param queue            Event queue used to transfer sigio events to this thread
      *  @param timeout          Timeout when reading for AT response
      *  @param delimiter        delimiter used when parsing at responses, "\r" should be used as output_delimiter
@@ -94,15 +99,15 @@ public:
      *  @return                 NULL, if fileHandle is not set, or a pointer to an existing ATHandler, if the fileHandle is
      *                          already in use. Otherwise a pointer to a new ATHandler instance is returned
      */
-    static ATHandler *get(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
+    static ATHandler *get_instance(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
                                    const char *delimiter, uint16_t send_delay, bool debug_on);
 
-    /** Release an ATHandler instance, and update the linked list
+    /** Close and delete the current ATHandler instance, if the reference count to it is 0.
+     *  Close() can be only called, if the ATHandler was opened with get_instance()
      *
-     *  @param at_handler       Pointer to the ATHandler
      *  @return NSAPI_ERROR_OK on success, NSAPI_ERROR_PARAMETER on failure
      */
-    static nsapi_error_t release(ATHandler *at_handler);
+    nsapi_error_t close();
 
     /** Locks the mutex for file handle if AT_HANDLER_MUTEX is defined.
      */
@@ -148,10 +153,14 @@ public:
     device_err_t get_last_device_error() const;
 
     /** Increase reference count. Used for counting references to this instance.
+     *  Note that this should be used with care, if the ATHandler was taken into use
+     *  with get_instance()
      */
     void inc_ref_count();
 
     /** Decrease reference count. Used for counting references to this instance.
+     *  Note that this should be used with care, if the ATHandler was taken into use
+     *  with get_instance()
      */
     void dec_ref_count();
 
@@ -168,6 +177,13 @@ public:
      */
     void set_at_timeout(uint32_t timeout_milliseconds, bool default_timeout = false);
 
+    /** Set timeout in milliseconds for all ATHandlers in the _atHandlers list
+     *
+     *  @param timeout_milliseconds  Timeout in milliseconds
+     *  @param default_timeout       Store as default timeout
+     */
+    static void set_at_timeout_list(uint32_t timeout_milliseconds, bool default_timeout = false);
+
     /** Restore timeout to previous timeout. Handy if there is a need to change timeout temporarily.
      */
     void restore_at_timeout();
@@ -244,7 +260,6 @@ private:
     bool _is_fh_usable;
 
     static ATHandler *_atHandlers;
-    static PlatformMutex _getReleaseMutex;
 
     //*************************************
 public:
@@ -422,6 +437,12 @@ public: // just for debugging
      */
     void set_debug(bool debug_on);
 
+    /** Set debug_on for all ATHandlers in the _atHandlers list
+     *
+     *  @param debug_on Set true to enable debug traces
+     */
+    static void set_debug_list(bool debug_on);
+
 private:
 
     // should fit any prefix and int

features/cellular/framework/AT/ATHandler_factory.cpp

@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) 2018, Arm Limited and affiliates.
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * 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.
+ */
+
+#include "ATHandler.h"
+#include "SingletonPtr.h"
+
+using namespace mbed;
+
+ATHandler *ATHandler::_atHandlers = NULL;
+
+// each parser is associated with one filehandle (that is UART)
+ATHandler *ATHandler::get_instance(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
+                                   const char *delimiter, uint16_t send_delay, bool debug_on)
+{
+    if (!fileHandle) {
+        return NULL;
+    }
+
+    singleton_lock();
+    ATHandler *atHandler = _atHandlers;
+    while (atHandler) {
+        if (atHandler->get_file_handle() == fileHandle) {
+            atHandler->inc_ref_count();
+            singleton_unlock();
+            return atHandler;
+        }
+        atHandler = atHandler->_nextATHandler;
+    }
+
+    atHandler = new ATHandler(fileHandle, queue, timeout, delimiter, send_delay);
+    if (debug_on) {
+        atHandler->set_debug(debug_on);
+    }
+    atHandler->_nextATHandler = _atHandlers;
+    _atHandlers = atHandler;
+
+    singleton_unlock();
+    return atHandler;
+}
+
+nsapi_error_t ATHandler::close()
+{
+    if (get_ref_count() == 0) {
+        return NSAPI_ERROR_PARAMETER;
+    }
+
+    singleton_lock();
+    dec_ref_count();
+    if (get_ref_count() == 0) {
+        // we can delete this at_handler
+        ATHandler *atHandler = _atHandlers;
+        ATHandler *prev = NULL;
+        while (atHandler) {
+            if (atHandler == this) {
+                if (prev == NULL) {
+                    _atHandlers = _atHandlers->_nextATHandler;
+                } else {
+                    prev->_nextATHandler = atHandler->_nextATHandler;
+                }
+                delete this;
+                break;
+            } else {
+                prev = atHandler;
+                atHandler = atHandler->_nextATHandler;
+            }
+        }
+    }
+    singleton_unlock();
+    return NSAPI_ERROR_OK;
+}
+
+void ATHandler::set_at_timeout_list(uint32_t timeout_milliseconds, bool default_timeout)
+{
+    ATHandler *atHandler = _atHandlers;
+    singleton_lock();
+    while (atHandler) {
+        atHandler->set_at_timeout(timeout_milliseconds, default_timeout);
+        atHandler = atHandler->_nextATHandler;
+    }
+    singleton_unlock();
+}
+
+void ATHandler::set_debug_list(bool debug_on)
+{
+    ATHandler *atHandler = _atHandlers;
+    singleton_lock();
+    while (atHandler) {
+        atHandler->set_debug(debug_on);
+        atHandler = atHandler->_nextATHandler;
+    }
+    singleton_unlock();
+}

features/cellular/framework/AT/AT_CellularDevice.cpp

@@ -74,7 +74,7 @@ ATHandler *AT_CellularDevice::get_at_handler(FileHandle *fileHandle)
         fileHandle = _fh;
     }
 
-    return ATHandler::get(fileHandle, _queue, _default_timeout,
+    return ATHandler::get_instance(fileHandle, _queue, _default_timeout,
                                    "\r", get_send_delay(), _modem_debug_on);
 }
 
@@ -85,7 +85,11 @@ ATHandler *AT_CellularDevice::get_at_handler()
 
 nsapi_error_t AT_CellularDevice::release_at_handler(ATHandler *at_handler)
 {
-    return ATHandler::release(at_handler);
+    if (at_handler) {
+        return at_handler->close();
+    } else {
+        return NSAPI_ERROR_PARAMETER;
+    }
 }
 
 CellularContext *AT_CellularDevice::get_context_list() const
@@ -310,21 +314,7 @@ void AT_CellularDevice::set_timeout(int timeout)
 {
     _default_timeout = timeout;
 
-    if (_network) {
+    ATHandler::set_at_timeout_list(_default_timeout, true);
-        _network->get_at_handler().set_at_timeout(_default_timeout, true);
-    }
-    if (_sms) {
-        _sms->get_at_handler().set_at_timeout(_default_timeout, true);
-    }
-    if (_power) {
-        _power->get_at_handler().set_at_timeout(_default_timeout, true);
-    }
-    if (_sim) {
-        _sim->get_at_handler().set_at_timeout(_default_timeout, true);
-    }
-    if (_information) {
-        _information->get_at_handler().set_at_timeout(_default_timeout, true);
-    }
 }
 
 uint16_t AT_CellularDevice::get_send_delay() const
@@ -336,21 +326,7 @@ void AT_CellularDevice::modem_debug_on(bool on)
 {
     _modem_debug_on = on;
 
-    if (_network) {
+    ATHandler::set_debug_list(_modem_debug_on);
-        _network->get_at_handler().set_debug(_modem_debug_on);
-    }
-    if (_sms) {
-        _sms->get_at_handler().set_debug(_modem_debug_on);
-    }
-    if (_power) {
-        _power->get_at_handler().set_debug(_modem_debug_on);
-    }
-    if (_sim) {
-        _sim->get_at_handler().set_debug(_modem_debug_on);
-    }
-    if (_information) {
-        _information->get_at_handler().set_debug(_modem_debug_on);
-    }
 }
 
 nsapi_error_t AT_CellularDevice::init_module()