Merge pull request #9898 from jarvte/connect_disconnect_fix

Cellular: fix connect-disconnect sequence called many times

UNITTESTS/features/cellular/framework/AT/at_cellularnetwork/at_cellularnetworktest.cpp

@@ -653,20 +653,6 @@ TEST_F(TestAT_CellularNetwork, test_AT_CellularNetwork_attach)
     cn.attach(&network_cb);
 }
 
-TEST_F(TestAT_CellularNetwork, test_get_connection_status)
-{
-    EventQueue que;
-    FileHandle_stub fh1;
-    ATHandler at(&fh1, que, 0, ",");
-
-    AT_CellularNetwork cn(at);
-
-    ATHandler_stub::nsapi_error_value = NSAPI_ERROR_OK;
-    network_cb_count = 0;
-    cn.attach(&network_cb);
-    EXPECT_TRUE(NSAPI_STATUS_DISCONNECTED == cn.get_connection_status());
-}
-
 TEST_F(TestAT_CellularNetwork, test_AT_CellularNetwork_set_receive_period)
 {
     EventQueue que;

UNITTESTS/stubs/AT_CellularNetwork_stub.cpp

@@ -44,11 +44,6 @@ void AT_CellularNetwork::attach(Callback<void(nsapi_event_t, intptr_t)> status_c
 {
 }
 
-nsapi_connection_status_t AT_CellularNetwork::get_connection_status() const
-{
-    return NSAPI_STATUS_LOCAL_UP;
-}
-
 nsapi_error_t AT_CellularNetwork::set_registration_urc(RegistrationType type, bool urc_on)
 {
     if (AT_CellularNetwork_stub::set_registration_urc_fail_counter) {

UNITTESTS/stubs/CellularDevice_stub.cpp

@@ -103,6 +103,6 @@ nsapi_error_t CellularDevice::shutdown()
     return NSAPI_ERROR_OK;
 }
 
-void CellularDevice::cellular_callback(nsapi_event_t ev, intptr_t ptr)
+void CellularDevice::cellular_callback(nsapi_event_t ev, intptr_t ptr, CellularContext *ctx)
 {
 }

features/cellular/framework/API/CellularContext.h

@@ -133,6 +133,8 @@ public: // from NetworkInterface
      *  The parameters on the callback are the event type and event type dependent reason parameter.
      *
      *  @remark  deleting CellularDevice/CellularContext in callback is not allowed.
+     *  @remark  Allocating/adding lots of traces not recommended as callback is called mostly from State machines thread which
+     *           is now 2048. You can change to main thread for example via EventQueue.
      *
      *  @param status_cb The callback for status changes.
      */

features/cellular/framework/API/CellularDevice.h

@@ -283,8 +283,8 @@ public:
      *  The parameters on the callback are the event type and event-type dependent reason parameter.
      *
      *  @remark  deleting CellularDevice/CellularContext in callback not allowed.
-     *  @remark  application should not attach to this function if using CellularContext::attach as it will contain the
+     *  @remark  Allocating/adding lots of traces not recommended as callback is called mostly from State machines thread which
-     *           same information.
+     *           is now 2048. You can change to main thread for example via EventQueue.
      *
      *  @param status_cb The callback for status changes.
      */
@@ -420,8 +420,8 @@ protected:
      *  This method will broadcast to every interested classes:
      *  CellularContext (might be many) and CellularStateMachine if available.
      */
-    void cellular_callback(nsapi_event_t ev, intptr_t ptr);
+    void cellular_callback(nsapi_event_t ev, intptr_t ptr, CellularContext *ctx = NULL);
-
+    void stm_callback(nsapi_event_t ev, intptr_t ptr);
     int _network_ref_count;
     int _sms_ref_count;
     int _info_ref_count;

features/cellular/framework/API/CellularNetwork.h

@@ -319,16 +319,14 @@ public:
      *  on the network. The parameters on the callback are the event type and
      *  event-type dependent reason parameter.
      *
+     *  @remark Application should not call attach if using CellularContext class. Call instead CellularContext::attach
+     *          as CellularDevice is dependent of this attach if CellularContext/CellularDevice is used to get
+     *          device/sim ready, registered, attached, connected.
+     *
      *  @param status_cb The callback for status changes
      */
     virtual void attach(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb) = 0;
 
-    /** Get the connection status
-     *
-     *  @return         The connection status according to ConnectionStatusType
-     */
-    virtual nsapi_connection_status_t get_connection_status() const = 0;
-
     /** Read operator names
      *
      *  @param op_names      on successful return contains linked list of operator names.

features/cellular/framework/AT/ATHandler.cpp

@@ -1256,22 +1256,27 @@ void ATHandler::debug_print(const char *p, int len)
 bool ATHandler::sync(int timeout_ms)
 {
     tr_debug("AT sync");
+    lock();
+    uint32_t timeout = _at_timeout;
+    _at_timeout = timeout_ms;
     // poll for 10 seconds
     for (int i = 0; i < 10; i++) {
-        lock();
-        set_at_timeout(timeout_ms, false);
         // For sync use an AT command that is supported by all modems and likely not used frequently,
         // especially a common response like OK could be response to previous request.
+        clear_error();
+        _start_time = rtos::Kernel::get_ms_count();
         cmd_start("AT+CMEE?");
         cmd_stop();
         resp_start("+CMEE:");
         resp_stop();
-        restore_at_timeout();
-        unlock();
         if (!_last_err) {
+            _at_timeout = timeout;
+            unlock();
             return true;
         }
     }
     tr_error("AT sync failed");
+    _at_timeout = timeout;
+    unlock();
     return false;
 }

features/cellular/framework/AT/AT_CellularContext.cpp

@@ -498,7 +498,6 @@ nsapi_error_t AT_CellularContext::activate_context()
     if (err != NSAPI_ERROR_OK) {
         _at.unlock();
         tr_error("Failed to activate network context! (%d)", err);
-        call_network_cb(NSAPI_STATUS_DISCONNECTED);
         return err;
     }
 
@@ -551,16 +550,18 @@ void AT_CellularContext::do_connect()
 {
     if (!_is_context_active) {
         _cb_data.error = do_activate_context();
-#if !NSAPI_PPP_AVAILABLE
+    } else {
-        // in PPP mode we did not activate any context, just searched the correct _cid
+        _cb_data.error = NSAPI_ERROR_OK;
-        if (_status_cb) {
-            _status_cb((nsapi_event_t)CellularActivatePDPContext, (intptr_t)&_cb_data);
-        }
-#endif // !NSAPI_PPP_AVAILABLE
     }
 
+#if !NSAPI_PPP_AVAILABLE
+    // in PPP mode we did not activate any context, just searched the correct _cid
+    if (_status_cb) {
+        _status_cb((nsapi_event_t)CellularActivatePDPContext, (intptr_t)&_cb_data);
+    }
+#endif // !NSAPI_PPP_AVAILABLE
+
     if (_cb_data.error != NSAPI_ERROR_OK) {
-        call_network_cb(NSAPI_STATUS_DISCONNECTED);
         _is_connected = false;
         return;
     }
@@ -630,16 +631,23 @@ void AT_CellularContext::ppp_status_cb(nsapi_event_t ev, intptr_t ptr)
     tr_debug("ppp_status_cb: event %d, ptr %d", ev, ptr);
     if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_GLOBAL_UP) {
         _is_connected = true;
-    } else if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_DISCONNECTED) {
-        ppp_disconnected();
     } else {
         _is_connected = false;
     }
 
     _connect_status = (nsapi_connection_status_t)ptr;
 
+    // catch all NSAPI_STATUS_DISCONNECTED events but send to device only when we did not ask for disconnect.
+    if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_DISCONNECTED) {
+        if (_is_connected) {
+            ppp_disconnected();
+            _device->cellular_callback(ev, ptr, this);
+        }
+        return;
+    }
+
     // call device's callback, it will broadcast this to here (cellular_callback)
-    _device->cellular_callback(ev, ptr);
+    _device->cellular_callback(ev, ptr, this);
 }
 
 void AT_CellularContext::ppp_disconnected()
@@ -660,10 +668,13 @@ void AT_CellularContext::ppp_disconnected()
 
 nsapi_error_t AT_CellularContext::disconnect()
 {
-    tr_info("CellularContext disconnect");
+    tr_info("CellularContext disconnect()");
     if (!_nw || !_is_connected) {
         return NSAPI_ERROR_NO_CONNECTION;
     }
+
+    // set false here so callbacks know that we are not connected and so should not send DISCONNECTED
+    _is_connected = false;
 #if NSAPI_PPP_AVAILABLE
     nsapi_error_t err = nsapi_ppp_disconnect(_at.get_file_handle());
     if (err != NSAPI_ERROR_OK) {
@@ -681,11 +692,12 @@ nsapi_error_t AT_CellularContext::disconnect()
         } else {
             deactivate_ip_context();
         }
-    } else {
-        call_network_cb(NSAPI_STATUS_DISCONNECTED);
     }
+    _is_context_active = false;
+    _connect_status = NSAPI_STATUS_DISCONNECTED;
 
-    _is_connected = false;
+    // call device's callback, it will broadcast this to here (cellular_callback)
+    _device->cellular_callback(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, NSAPI_STATUS_DISCONNECTED, this);
 
     return _at.unlock_return_error();
 }
@@ -928,20 +940,24 @@ void AT_CellularContext::cellular_callback(nsapi_event_t ev, intptr_t ptr)
         if (_is_blocking) {
             if (data->error != NSAPI_ERROR_OK) {
                 // operation failed, release semaphore
+                _current_op = OP_INVALID;
                 _semaphore.release();
             } else {
                 if ((st == CellularDeviceReady && _current_op == OP_DEVICE_READY) ||
                         (st == CellularSIMStatusChanged && _current_op == OP_SIM_READY &&
                          data->status_data == CellularDevice::SimStateReady)) {
                     // target reached, release semaphore
+                    _current_op = OP_INVALID;
                     _semaphore.release();
                 } else if (st == CellularRegistrationStatusChanged && (data->status_data == CellularNetwork::RegisteredHomeNetwork ||
                                                                        data->status_data == CellularNetwork::RegisteredRoaming || data->status_data == CellularNetwork::AlreadyRegistered) && _current_op == OP_REGISTER) {
                     // target reached, release semaphore
+                    _current_op = OP_INVALID;
                     _semaphore.release();
                 } else if (st == CellularAttachNetwork && (_current_op == OP_ATTACH || _current_op == OP_CONNECT) &&
                            data->status_data == CellularNetwork::Attached) {
                     // target reached, release semaphore
+                    _current_op = OP_INVALID;
                     _semaphore.release();
                 }
             }
@@ -949,6 +965,7 @@ void AT_CellularContext::cellular_callback(nsapi_event_t ev, intptr_t ptr)
             // non blocking
             if (st == CellularAttachNetwork && _current_op == OP_CONNECT && data->error == NSAPI_ERROR_OK &&
                     data->status_data == CellularNetwork::Attached) {
+                _current_op = OP_INVALID;
                 // forward to application
                 if (_status_cb) {
                     _status_cb(ev, ptr);
@@ -963,14 +980,18 @@ void AT_CellularContext::cellular_callback(nsapi_event_t ev, intptr_t ptr)
             if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_GLOBAL_UP) {
                 tr_info("CellularContext IP %s", get_ip_address());
                 _cb_data.error = NSAPI_ERROR_OK;
-                _semaphore.release();
             } else if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_DISCONNECTED) {
                 tr_info("PPP disconnected");
                 _cb_data.error = NSAPI_ERROR_NO_CONNECTION;
-                _semaphore.release();
             }
         }
-#endif
+#else
+#if MBED_CONF_MBED_TRACE_ENABLE
+        if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_DISCONNECTED) {
+            tr_info("cb: CellularContext disconnected");
+        }
+#endif // MBED_CONF_MBED_TRACE_ENABLE
+#endif // NSAPI_PPP_AVAILABLE
     }
 
     // forward to application
@@ -1044,8 +1065,7 @@ void AT_CellularContext::ciot_opt_cb(mbed::CellularNetwork::CIoT_Supported_Opt
 
 void AT_CellularContext::set_disconnect()
 {
+    tr_debug("AT_CellularContext::set_disconnect()");
     _is_connected = false;
-    cell_callback_data_t data;
+    _device->cellular_callback(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, NSAPI_STATUS_DISCONNECTED, this);
-    data.error = NSAPI_STATUS_DISCONNECTED;
-    _device->cellular_callback(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, (intptr_t)&data);
 }

features/cellular/framework/AT/AT_CellularNetwork.cpp

@@ -171,7 +171,7 @@ void AT_CellularNetwork::read_reg_params_and_compare(RegistrationType type)
                     reg_params._status == RegisteredRoaming)) {
                 if (previous_registration_status == RegisteredHomeNetwork ||
                         previous_registration_status == RegisteredRoaming) {
-                    _connection_status_cb(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, NSAPI_STATUS_DISCONNECTED);
+                    call_network_cb(NSAPI_STATUS_DISCONNECTED);
                 }
             }
         }
@@ -200,11 +200,8 @@ void AT_CellularNetwork::urc_cgreg()
 
 void AT_CellularNetwork::call_network_cb(nsapi_connection_status_t status)
 {
-    if (_connect_status != status) {
+    if (_connection_status_cb) {
-        _connect_status = status;
+        _connection_status_cb(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, _connect_status);
-        if (_connection_status_cb) {
-            _connection_status_cb(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, _connect_status);
-        }
     }
 }
 
@@ -213,11 +210,6 @@ void AT_CellularNetwork::attach(Callback<void(nsapi_event_t, intptr_t)> status_c
     _connection_status_cb = status_cb;
 }
 
-nsapi_connection_status_t AT_CellularNetwork::get_connection_status() const
-{
-    return _connect_status;
-}
-
 nsapi_error_t AT_CellularNetwork::set_registration_urc(RegistrationType type, bool urc_on)
 {
     int index = (int)type;

features/cellular/framework/AT/AT_CellularNetwork.h

@@ -63,8 +63,6 @@ public: // CellularNetwork
 
     virtual void attach(Callback<void(nsapi_event_t, intptr_t)> status_cb);
 
-    virtual nsapi_connection_status_t get_connection_status() const;
-
     virtual nsapi_error_t set_access_technology(RadioAccessTechnology rat);
 
     virtual nsapi_error_t scan_plmn(operList_t &operators, int &ops_count);

features/cellular/framework/device/CellularDevice.cpp

@@ -113,13 +113,20 @@ nsapi_error_t CellularDevice::create_state_machine()
     nsapi_error_t err = NSAPI_ERROR_OK;
     if (!_state_machine) {
         _state_machine = new CellularStateMachine(*this, *get_queue());
-        _state_machine->set_cellular_callback(callback(this, &CellularDevice::cellular_callback));
+        _state_machine->set_cellular_callback(callback(this, &CellularDevice::stm_callback));
         err = _state_machine->start_dispatch();
         if (err) {
             tr_error("Start state machine failed.");
             delete _state_machine;
             _state_machine = NULL;
         }
+
+        if (strlen(_plmn)) {
+            _state_machine->set_plmn(_plmn);
+        }
+        if (strlen(_sim_pin)) {
+            _state_machine->set_sim_pin(_sim_pin);
+        }
     }
     return err;
 }
@@ -156,7 +163,12 @@ void CellularDevice::attach(Callback<void(nsapi_event_t, intptr_t)> status_cb)
     _status_cb = status_cb;
 }
 
-void CellularDevice::cellular_callback(nsapi_event_t ev, intptr_t ptr)
+void CellularDevice::stm_callback(nsapi_event_t ev, intptr_t ptr)
+{
+    cellular_callback(ev, ptr);
+}
+
+void CellularDevice::cellular_callback(nsapi_event_t ev, intptr_t ptr, CellularContext *ctx)
 {
     if (ev >= NSAPI_EVENT_CELLULAR_STATUS_BASE && ev <= NSAPI_EVENT_CELLULAR_STATUS_END) {
         cell_callback_data_t *ptr_data = (cell_callback_data_t *)ptr;
@@ -166,28 +178,23 @@ void CellularDevice::cellular_callback(nsapi_event_t ev, intptr_t ptr)
             // broadcast only network registration changes to state machine
             _state_machine->cellular_event_changed(ev, ptr);
         }
-
         if (cell_ev == CellularDeviceReady && ptr_data->error == NSAPI_ERROR_OK) {
             // Here we can create mux and give new filehandles as mux reserves the one what was in use.
             // if mux we would need to set new filehandle:_state_machine->set_filehandle( get fh from mux);
             _nw = open_network(_fh);
             // Attach to network so we can get update status from the network
-            _nw->attach(callback(this, &CellularDevice::cellular_callback));
+            _nw->attach(callback(this, &CellularDevice::stm_callback));
-            if (strlen(_plmn)) {
-                _state_machine->set_plmn(_plmn);
-            }
-        } else if (cell_ev == CellularSIMStatusChanged && ptr_data->error == NSAPI_ERROR_OK &&
-                   ptr_data->status_data == SimStatePinNeeded) {
-            if (strlen(_sim_pin)) {
-                _state_machine->set_sim_pin(_sim_pin);
-            }
         }
     } else {
         tr_debug("callback: %d, ptr: %d", ev, ptr);
         if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_DISCONNECTED) {
             // we have been disconnected, reset state machine so that application can start connect sequence again
             if (_state_machine) {
-                _state_machine->reset();
+                CellularStateMachine::CellularState current_state, targeted_state;
+                bool is_running = _state_machine->get_current_status(current_state, targeted_state);
+                if (!is_running) {
+                    _state_machine->reset();
+                }
             }
         }
     }
@@ -195,11 +202,18 @@ void CellularDevice::cellular_callback(nsapi_event_t ev, intptr_t ptr)
     // broadcast network and cellular changes to state machine and CellularContext.
     CellularContext *curr = get_context_list();
     while (curr) {
-        curr->cellular_callback(ev, ptr);
+        if (ctx) {
+            if (ctx == curr) {
+                curr->cellular_callback(ev, ptr);
+                break;
+            }
+        } else {
+            curr->cellular_callback(ev, ptr);
+        }
         curr = curr->_next;
     }
 
-    // forward to callback function if set by attach(...)
+    // forward to callback function if set by attach(...).
     if (_status_cb) {
         _status_cb(ev, ptr);
     }

features/cellular/framework/device/CellularStateMachine.cpp

@@ -320,6 +320,7 @@ void CellularStateMachine::retry_state_or_fail()
         tr_debug("%s: retry %d/%d", get_state_string(_state), _retry_count, RETRY_ARRAY_SIZE);
         _event_timeout = _retry_timeout_array[_retry_count];
         _is_retry = true;
+        _cb_data.error = NSAPI_ERROR_OK;
     } else {
         report_failure(get_state_string(_state));
         return;