From ec73c8a9c7944ef9c95d6d13485bb17891b2e1d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teppo=20J=C3=A4rvelin?= Date: Fri, 1 Mar 2019 09:08:03 +0200 Subject: [PATCH 1/2] Cellular: fix connect-disconnect sequence called many times Fix syncing back to at mode after ppp disconnect. Fix AT_CellularContext flags and states to allow new connect after disconnect. Fix that state machine is not reseted in disconnect is it's running (might be running because of another context or new connect already started). --- UNITTESTS/stubs/CellularDevice_stub.cpp | 2 +- .../cellular/framework/API/CellularContext.h | 2 + .../cellular/framework/API/CellularDevice.h | 8 +-- .../cellular/framework/API/CellularNetwork.h | 4 ++ features/cellular/framework/AT/ATHandler.cpp | 13 ++-- .../framework/AT/AT_CellularContext.cpp | 62 ++++++++++++------- .../framework/AT/AT_CellularNetwork.cpp | 9 +-- .../framework/device/CellularDevice.cpp | 44 ++++++++----- .../framework/device/CellularStateMachine.cpp | 1 + 9 files changed, 94 insertions(+), 51 deletions(-) diff --git a/UNITTESTS/stubs/CellularDevice_stub.cpp b/UNITTESTS/stubs/CellularDevice_stub.cpp index 7309837f06..e53e0d5873 100644 --- a/UNITTESTS/stubs/CellularDevice_stub.cpp +++ b/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) { } diff --git a/features/cellular/framework/API/CellularContext.h b/features/cellular/framework/API/CellularContext.h index 6779e7fdee..590b9aada7 100644 --- a/features/cellular/framework/API/CellularContext.h +++ b/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. */ diff --git a/features/cellular/framework/API/CellularDevice.h b/features/cellular/framework/API/CellularDevice.h index a67d226cee..dd5d7e8206 100644 --- a/features/cellular/framework/API/CellularDevice.h +++ b/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 - * same information. + * @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. */ @@ -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; diff --git a/features/cellular/framework/API/CellularNetwork.h b/features/cellular/framework/API/CellularNetwork.h index 0eb87366f4..e9ca4c2ec3 100644 --- a/features/cellular/framework/API/CellularNetwork.h +++ b/features/cellular/framework/API/CellularNetwork.h @@ -319,6 +319,10 @@ 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 status_cb) = 0; diff --git a/features/cellular/framework/AT/ATHandler.cpp b/features/cellular/framework/AT/ATHandler.cpp index 7ea0947655..7282eb19d3 100644 --- a/features/cellular/framework/AT/ATHandler.cpp +++ b/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; } diff --git a/features/cellular/framework/AT/AT_CellularContext.cpp b/features/cellular/framework/AT/AT_CellularContext.cpp index 1188979c1c..46b637925e 100644 --- a/features/cellular/framework/AT/AT_CellularContext.cpp +++ b/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 - // 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 + } else { + _cb_data.error = NSAPI_ERROR_OK; } +#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; - data.error = NSAPI_STATUS_DISCONNECTED; - _device->cellular_callback(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, (intptr_t)&data); + _device->cellular_callback(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, NSAPI_STATUS_DISCONNECTED, this); } diff --git a/features/cellular/framework/AT/AT_CellularNetwork.cpp b/features/cellular/framework/AT/AT_CellularNetwork.cpp index 538fb27f81..29295cd42f 100644 --- a/features/cellular/framework/AT/AT_CellularNetwork.cpp +++ b/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) { - _connect_status = status; - if (_connection_status_cb) { - _connection_status_cb(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, _connect_status); - } + if (_connection_status_cb) { + _connection_status_cb(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, _connect_status); } } diff --git a/features/cellular/framework/device/CellularDevice.cpp b/features/cellular/framework/device/CellularDevice.cpp index 8cc63e544c..751c040b60 100644 --- a/features/cellular/framework/device/CellularDevice.cpp +++ b/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 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)); - 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); - } + _nw->attach(callback(this, &CellularDevice::stm_callback)); } } 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); } diff --git a/features/cellular/framework/device/CellularStateMachine.cpp b/features/cellular/framework/device/CellularStateMachine.cpp index 58deb6c144..f4ad2bef65 100644 --- a/features/cellular/framework/device/CellularStateMachine.cpp +++ b/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; From 0905f014387e5ea1ec105507f5f0364d96cccde6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teppo=20J=C3=A4rvelin?= Date: Fri, 1 Mar 2019 14:51:05 +0200 Subject: [PATCH 2/2] Cellular: Removed API get_connection_status() from CellularNetwork This was left accidentally after refactoring. It wasn't giving correct states after refactoring. CellularContext::get_connection_status should be used instead. --- .../at_cellularnetwork/at_cellularnetworktest.cpp | 14 -------------- UNITTESTS/stubs/AT_CellularNetwork_stub.cpp | 5 ----- features/cellular/framework/API/CellularNetwork.h | 6 ------ .../cellular/framework/AT/AT_CellularNetwork.cpp | 5 ----- .../cellular/framework/AT/AT_CellularNetwork.h | 2 -- 5 files changed, 32 deletions(-) diff --git a/UNITTESTS/features/cellular/framework/AT/at_cellularnetwork/at_cellularnetworktest.cpp b/UNITTESTS/features/cellular/framework/AT/at_cellularnetwork/at_cellularnetworktest.cpp index d79bf9e5e9..f755e557ca 100644 --- a/UNITTESTS/features/cellular/framework/AT/at_cellularnetwork/at_cellularnetworktest.cpp +++ b/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; diff --git a/UNITTESTS/stubs/AT_CellularNetwork_stub.cpp b/UNITTESTS/stubs/AT_CellularNetwork_stub.cpp index 899be2fb4c..136539d830 100644 --- a/UNITTESTS/stubs/AT_CellularNetwork_stub.cpp +++ b/UNITTESTS/stubs/AT_CellularNetwork_stub.cpp @@ -44,11 +44,6 @@ void AT_CellularNetwork::attach(Callback 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) { diff --git a/features/cellular/framework/API/CellularNetwork.h b/features/cellular/framework/API/CellularNetwork.h index e9ca4c2ec3..31a9ea8e0b 100644 --- a/features/cellular/framework/API/CellularNetwork.h +++ b/features/cellular/framework/API/CellularNetwork.h @@ -327,12 +327,6 @@ public: */ virtual void attach(mbed::Callback 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. diff --git a/features/cellular/framework/AT/AT_CellularNetwork.cpp b/features/cellular/framework/AT/AT_CellularNetwork.cpp index 29295cd42f..23b88ab992 100644 --- a/features/cellular/framework/AT/AT_CellularNetwork.cpp +++ b/features/cellular/framework/AT/AT_CellularNetwork.cpp @@ -210,11 +210,6 @@ void AT_CellularNetwork::attach(Callback 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; diff --git a/features/cellular/framework/AT/AT_CellularNetwork.h b/features/cellular/framework/AT/AT_CellularNetwork.h index 044bc50c0d..0bc7566e21 100644 --- a/features/cellular/framework/AT/AT_CellularNetwork.h +++ b/features/cellular/framework/AT/AT_CellularNetwork.h @@ -63,8 +63,6 @@ public: // CellularNetwork virtual void attach(Callback 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);