From fe5f97895be1e75f4a72a2d2aa438ee7aaa929a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teppo=20J=C3=A4rvelin?= Date: Mon, 8 Apr 2019 09:05:54 +0300 Subject: [PATCH] Cellular: Fix deleting of state machine to correct class --- .../cellularstatemachinetest.cpp | 22 ++++----- UNITTESTS/stubs/CellularStateMachine_stub.cpp | 4 +- .../framework/AT/AT_CellularDevice.cpp | 2 - .../framework/device/CellularDevice.cpp | 13 ++--- .../framework/device/CellularStateMachine.cpp | 47 +++++++------------ .../framework/device/CellularStateMachine.h | 5 +- 6 files changed, 39 insertions(+), 54 deletions(-) diff --git a/UNITTESTS/features/cellular/framework/device/cellularstatemachine/cellularstatemachinetest.cpp b/UNITTESTS/features/cellular/framework/device/cellularstatemachine/cellularstatemachinetest.cpp index b96a20f4bea..ae168e56661 100644 --- a/UNITTESTS/features/cellular/framework/device/cellularstatemachine/cellularstatemachinetest.cpp +++ b/UNITTESTS/features/cellular/framework/device/cellularstatemachine/cellularstatemachinetest.cpp @@ -73,9 +73,9 @@ class UT_CellularStateMachine { _state_machine = NULL; } - CellularStateMachine *create_state_machine(CellularDevice &device, events::EventQueue &queue) + CellularStateMachine *create_state_machine(CellularDevice &device, events::EventQueue &queue, CellularNetwork &nw) { - _state_machine = new CellularStateMachine(device, queue); + _state_machine = new CellularStateMachine(device, queue, nw); return _state_machine; } @@ -171,7 +171,7 @@ TEST_F(TestCellularStateMachine, test_create_delete) CellularDevice *dev = new myCellularDevice(&fh1); EXPECT_TRUE(dev); - CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue()); + CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); ut.delete_state_machine(); @@ -187,7 +187,7 @@ TEST_F(TestCellularStateMachine, test_setters) CellularDevice *dev = new myCellularDevice(&fh1); EXPECT_TRUE(dev); - CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue()); + CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); ut.set_cellular_callback(&cellular_callback); @@ -215,14 +215,14 @@ TEST_F(TestCellularStateMachine, test_start_dispatch) CellularDevice *dev = new myCellularDevice(&fh1); EXPECT_TRUE(dev); - CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue()); + CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); nsapi_error_t err = ut.start_dispatch(); ASSERT_EQ(NSAPI_ERROR_OK, err); ut.delete_state_machine(); Thread_stub::osStatus_value = osErrorNoMemory; - stm = ut.create_state_machine(*dev, *dev->get_queue()); + stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); err = ut.start_dispatch(); ASSERT_EQ(NSAPI_ERROR_NO_MEMORY, err); @@ -240,13 +240,13 @@ TEST_F(TestCellularStateMachine, test_stop) CellularDevice *dev = new AT_CellularDevice(&fh1); EXPECT_TRUE(dev); - CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue()); + CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); ut.stop(); // nothing created, run through ut.delete_state_machine(); - stm = ut.create_state_machine(*dev, *dev->get_queue()); + stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); nsapi_error_t err = ut.start_dispatch(); ASSERT_EQ(NSAPI_ERROR_OK, err); @@ -254,7 +254,7 @@ TEST_F(TestCellularStateMachine, test_stop) ut.stop(); // thread is created, now stop will delete it ut.delete_state_machine(); - stm = ut.create_state_machine(*dev, *dev->get_queue()); + stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); err = ut.start_dispatch(); ASSERT_EQ(NSAPI_ERROR_OK, err); @@ -270,7 +270,7 @@ TEST_F(TestCellularStateMachine, test_stop) ut.stop(); // thread and power are created, now stop will delete them ut.delete_state_machine(); - stm = ut.create_state_machine(*dev, *dev->get_queue()); + stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); err = ut.start_dispatch(); ASSERT_EQ(NSAPI_ERROR_OK, err); @@ -294,7 +294,7 @@ TEST_F(TestCellularStateMachine, test_run_to_state) CellularDevice *dev = new AT_CellularDevice(&fh1); EXPECT_TRUE(dev); - CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue()); + CellularStateMachine *stm = ut.create_state_machine(*dev, *dev->get_queue(), *dev->open_network()); EXPECT_TRUE(stm); nsapi_error_t err = ut.start_dispatch(); diff --git a/UNITTESTS/stubs/CellularStateMachine_stub.cpp b/UNITTESTS/stubs/CellularStateMachine_stub.cpp index 6e9d0ffe345..1c3c8cc10a2 100644 --- a/UNITTESTS/stubs/CellularStateMachine_stub.cpp +++ b/UNITTESTS/stubs/CellularStateMachine_stub.cpp @@ -25,8 +25,8 @@ CellularStubState CellularStateMachine_stub::get_current_target_state = STATE_IN CellularStubState CellularStateMachine_stub::get_current_current_state = STATE_INIT; bool CellularStateMachine_stub::bool_value = false; -CellularStateMachine::CellularStateMachine(CellularDevice &device, events::EventQueue &queue) : - _cellularDevice(device), _queue(queue) +CellularStateMachine::CellularStateMachine(CellularDevice &device, events::EventQueue &queue, CellularNetwork &nw) : + _cellularDevice(device), _network(nw), _queue(queue) { } diff --git a/features/cellular/framework/AT/AT_CellularDevice.cpp b/features/cellular/framework/AT/AT_CellularDevice.cpp index 2e358628843..5a92e3db52d 100644 --- a/features/cellular/framework/AT/AT_CellularDevice.cpp +++ b/features/cellular/framework/AT/AT_CellularDevice.cpp @@ -47,8 +47,6 @@ AT_CellularDevice::AT_CellularDevice(FileHandle *fh) : CellularDevice(fh), _netw AT_CellularDevice::~AT_CellularDevice() { - delete _state_machine; - // make sure that all is deleted even if somewhere close was not called and reference counting is messed up. _network_ref_count = 1; _sms_ref_count = 1; diff --git a/features/cellular/framework/device/CellularDevice.cpp b/features/cellular/framework/device/CellularDevice.cpp index 838a671f6ea..d79e2e2c898 100644 --- a/features/cellular/framework/device/CellularDevice.cpp +++ b/features/cellular/framework/device/CellularDevice.cpp @@ -44,6 +44,7 @@ CellularDevice::CellularDevice(FileHandle *fh) : _network_ref_count(0), _sms_ref CellularDevice::~CellularDevice() { tr_debug("CellularDevice destruct"); + delete _state_machine; } void CellularDevice::stop() @@ -118,7 +119,10 @@ nsapi_error_t CellularDevice::create_state_machine() { nsapi_error_t err = NSAPI_ERROR_OK; if (!_state_machine) { - _state_machine = new CellularStateMachine(*this, *get_queue()); + _nw = open_network(_fh); + // Attach to network so we can get update status from the network + _nw->attach(callback(this, &CellularDevice::stm_callback)); + _state_machine = new CellularStateMachine(*this, *get_queue(), *_nw); _state_machine->set_cellular_callback(callback(this, &CellularDevice::stm_callback)); err = _state_machine->start_dispatch(); if (err) { @@ -184,13 +188,6 @@ void CellularDevice::cellular_callback(nsapi_event_t ev, intptr_t ptr, CellularC // 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::stm_callback)); - } } else { tr_debug("callback: %d, ptr: %d", ev, ptr); if (ev == NSAPI_EVENT_CONNECTION_STATUS_CHANGE && ptr == NSAPI_STATUS_DISCONNECTED) { diff --git a/features/cellular/framework/device/CellularStateMachine.cpp b/features/cellular/framework/device/CellularStateMachine.cpp index 384aac1458b..acd2c798e4b 100644 --- a/features/cellular/framework/device/CellularStateMachine.cpp +++ b/features/cellular/framework/device/CellularStateMachine.cpp @@ -43,9 +43,9 @@ const int DEVICE_READY = 0x04; namespace mbed { -CellularStateMachine::CellularStateMachine(CellularDevice &device, events::EventQueue &queue) : +CellularStateMachine::CellularStateMachine(CellularDevice &device, events::EventQueue &queue, CellularNetwork &nw) : _cellularDevice(device), _state(STATE_INIT), _next_state(_state), _target_state(_state), - _event_status_cb(0), _network(0), _queue(queue), _queue_thread(0), _sim_pin(0), + _event_status_cb(0), _network(nw), _queue(queue), _queue_thread(0), _sim_pin(0), _retry_count(0), _event_timeout(-1), _event_id(-1), _plmn(0), _command_success(false), _is_retry(false), _cb_data(), _current_event(NSAPI_EVENT_CONNECTION_STATUS_CHANGE), _status(0) { @@ -99,11 +99,6 @@ void CellularStateMachine::stop() reset(); _event_id = STM_STOPPED; - - if (_network) { - _cellularDevice.close_network(); - _network = NULL; - } } bool CellularStateMachine::power_on() @@ -163,7 +158,7 @@ bool CellularStateMachine::open_sim() if (sim_ready) { // If plmn is set, we should it right after sim is opened so that registration is forced to correct network. if (_plmn && strlen(_plmn)) { - _cb_data.error = _network->set_registration(_plmn); + _cb_data.error = _network.set_registration(_plmn); tr_debug("STM: manual set_registration: %d, plmn: %s", _cb_data.error, _plmn); if (_cb_data.error) { return false; @@ -204,7 +199,7 @@ bool CellularStateMachine::get_network_registration(CellularNetwork::Registratio is_registered = false; bool is_roaming = false; CellularNetwork::registration_params_t reg_params; - _cb_data.error = _network->get_registration_params(type, reg_params); + _cb_data.error = _network.get_registration_params(type, reg_params); if (_cb_data.error != NSAPI_ERROR_OK) { if (_cb_data.error != NSAPI_ERROR_UNSUPPORTED) { @@ -329,14 +324,10 @@ bool CellularStateMachine::device_ready() { tr_info("Modem ready"); - if (!_network) { - _network = _cellularDevice.open_network(); - } - #ifdef MBED_CONF_CELLULAR_RADIO_ACCESS_TECHNOLOGY MBED_ASSERT(MBED_CONF_CELLULAR_RADIO_ACCESS_TECHNOLOGY >= CellularNetwork::RAT_GSM && MBED_CONF_CELLULAR_RADIO_ACCESS_TECHNOLOGY < CellularNetwork::RAT_UNKNOWN); - nsapi_error_t err = _network->set_access_technology((CellularNetwork::RadioAccessTechnology)MBED_CONF_CELLULAR_RADIO_ACCESS_TECHNOLOGY); + nsapi_error_t err = _network.set_access_technology((CellularNetwork::RadioAccessTechnology)MBED_CONF_CELLULAR_RADIO_ACCESS_TECHNOLOGY); if (err != NSAPI_ERROR_OK && err != NSAPI_ERROR_UNSUPPORTED) { tr_warning("Failed to set access technology to %d", MBED_CONF_CELLULAR_RADIO_ACCESS_TECHNOLOGY); return false; @@ -382,7 +373,7 @@ void CellularStateMachine::state_sim_pin() if (open_sim()) { bool success = false; for (int type = 0; type < CellularNetwork::C_MAX; type++) { - _cb_data.error = _network->set_registration_urc((CellularNetwork::RegistrationType)type, true); + _cb_data.error = _network.set_registration_urc((CellularNetwork::RegistrationType)type, true); if (!_cb_data.error && (type == CellularNetwork::C_EREG || type == CellularNetwork::C_GREG)) { success = true; } @@ -393,19 +384,19 @@ void CellularStateMachine::state_sim_pin() return; } - if (_network->is_active_context()) { // check if context was already activated + if (_network.is_active_context()) { // check if context was already activated tr_debug("Active context found."); _status |= ACTIVE_PDP_CONTEXT; } CellularNetwork::AttachStatus status = CellularNetwork::Detached; // check if modem is already attached to a network - if (_network->get_attach(status) == NSAPI_ERROR_OK && status == CellularNetwork::Attached) { + if (_network.get_attach(status) == NSAPI_ERROR_OK && status == CellularNetwork::Attached) { _status |= ATTACHED_TO_NETWORK; tr_debug("Cellular already attached."); } // if packet domain event reporting is not set it's not a stopper. We might lack some events when we are // dropped from the network. - _cb_data.error = _network->set_packet_domain_event_reporting(true); + _cb_data.error = _network.set_packet_domain_event_reporting(true); if (_cb_data.error == NSAPI_STATUS_ERROR_UNSUPPORTED) { tr_warning("Packet domain event reporting not supported!"); } else if (_cb_data.error) { @@ -434,7 +425,7 @@ void CellularStateMachine::state_registering() } else { _cellularDevice.set_timeout(TIMEOUT_REGISTRATION); if (!_command_success && !_plmn) { // don't call set_registration twice for manual registration - _cb_data.error = _network->set_registration(_plmn); + _cb_data.error = _network.set_registration(_plmn); _command_success = (_cb_data.error == NSAPI_ERROR_OK); } retry_state_or_fail(); @@ -446,7 +437,7 @@ void CellularStateMachine::state_attaching() _cellularDevice.set_timeout(TIMEOUT_CONNECT); tr_info("Attaching network (timeout %d s)", TIMEOUT_CONNECT / 1000); if (_status != ATTACHED_TO_NETWORK) { - _cb_data.error = _network->set_attach(); + _cb_data.error = _network.set_attach(); } if (_cb_data.error == NSAPI_ERROR_OK) { if (_event_status_cb) { @@ -534,14 +525,12 @@ bool CellularStateMachine::get_current_status(CellularStateMachine::CellularStat void CellularStateMachine::event() { #if MBED_CONF_MBED_TRACE_ENABLE - if (_network) { - int rssi; - if (_network->get_signal_quality(rssi) == NSAPI_ERROR_OK) { - if (rssi == CellularNetwork::SignalQualityUnknown) { - tr_info("RSSI unknown"); - } else { - tr_info("RSSI %d dBm", rssi); - } + int rssi; + if (_network.get_signal_quality(rssi) == NSAPI_ERROR_OK) { + if (rssi == CellularNetwork::SignalQualityUnknown) { + tr_info("RSSI unknown"); + } else { + tr_info("RSSI %d dBm", rssi); } } #endif @@ -643,7 +632,7 @@ void CellularStateMachine::cellular_event_changed(nsapi_event_t ev, intptr_t ptr if ((cellular_connection_status_t)ev == CellularRegistrationStatusChanged && _state == STATE_REGISTERING_NETWORK) { // expect packet data so only these states are valid CellularNetwork::registration_params_t reg_params; - nsapi_error_t err = _network->get_registration_params(reg_params); + nsapi_error_t err = _network.get_registration_params(reg_params); if (err == NSAPI_ERROR_OK && (reg_params._type == CellularNetwork::C_EREG || reg_params._type == CellularNetwork::C_GREG)) { if ((data->status_data == CellularNetwork::RegisteredHomeNetwork || diff --git a/features/cellular/framework/device/CellularStateMachine.h b/features/cellular/framework/device/CellularStateMachine.h index 4d311f9941b..095d426fe47 100644 --- a/features/cellular/framework/device/CellularStateMachine.h +++ b/features/cellular/framework/device/CellularStateMachine.h @@ -44,8 +44,9 @@ class CellularStateMachine { * * @param device reference to CellularDevice * @param queue reference to queue used in state transitions + * @param nw reference to CellularNetwork */ - CellularStateMachine(CellularDevice &device, events::EventQueue &queue); + CellularStateMachine(CellularDevice &device, events::EventQueue &queue, CellularNetwork &nw); ~CellularStateMachine(); /** Cellular connection states @@ -161,7 +162,7 @@ class CellularStateMachine { Callback _event_status_cb; - CellularNetwork *_network; + CellularNetwork &_network; events::EventQueue &_queue; rtos::Thread *_queue_thread;