Skip to content

Refactor unnecessary functions from cellular driver #12265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 17, 2020
Merged

Refactor unnecessary functions from cellular driver #12265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e2cb180
Cellular: Refactor get_max_socket_count() into CellularProperty
Jan 15, 2020
ceea992
Cellular: Refactor is_protocol_supported() into CellularProperty
Jan 15, 2020
f9eef97
Cellular Refactor get_send_delay() into CellularProperty
Jan 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions UNITTESTS/features/cellular/framework/AT/at_cellularcontext/at_cellularcontexttest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,18 +77,10 @@ class TestAT_CellularContext : public testing::Test {
class my_stack : public AT_CellularStack {
public:
my_stack(ATHandler &atHandler, AT_CellularDevice &device) : AT_CellularStack(atHandler, 1, IPV4_STACK, device) {}
virtual int get_max_socket_count()
{
return 1;
}
virtual int get_max_packet_size()
{
return 200;
}
virtual bool is_protocol_supported(nsapi_protocol_t protocol)
{
return true;
}
virtual nsapi_error_t socket_close_impl(int sock_id)
{
return NSAPI_ERROR_OK;
Expand Down
7 changes: 0 additions & 7 deletions UNITTESTS/features/cellular/framework/AT/at_cellulardevice/at_cellulardevicetest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,6 @@ TEST_F(TestAT_CellularDevice, test_AT_CellularDevice_set_power_save_mode)
EXPECT_TRUE(NSAPI_ERROR_DEVICE_ERROR == dev.set_power_save_mode(0));
}

TEST_F(TestAT_CellularDevice, test_AT_CellularDevice_get_send_delay)
{
FileHandle_stub fh1;
AT_CellularDevice dev(&fh1);
EXPECT_TRUE(0 == dev.get_send_delay());
}

TEST_F(TestAT_CellularDevice, test_AT_CellularDevice_create_delete_context)
{
FileHandle_stub fh1;
Expand Down
38 changes: 15 additions & 23 deletions UNITTESTS/features/cellular/framework/AT/at_cellularstack/at_cellularstacktest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "ATHandler_stub.h"
#include "SocketAddress.h"
#include "CellularDevice_stub.h"
#include "AT_CellularDevice_stub.h"
#include "myCellularDevice.h"

using namespace mbed;
Expand All @@ -34,27 +35,15 @@ using namespace events;
class MyStack : public AT_CellularStack {
public:
bool bool_value;
bool max_sock_value;
nsapi_error_t create_error;
CellularSocket socket;

MyStack(ATHandler &atr, int cid, nsapi_ip_stack_t typ, AT_CellularDevice &device) : AT_CellularStack(atr, cid, typ, device)
{
bool_value = false;
max_sock_value = 0;
create_error = NSAPI_ERROR_OK;
}

virtual int get_max_socket_count()
{
return max_sock_value;
}

virtual bool is_protocol_supported(nsapi_protocol_t protocol)
{
return bool_value;
}

virtual nsapi_error_t socket_close_impl(int sock_id)
{
return NSAPI_ERROR_OK;
Expand Down Expand Up @@ -207,19 +196,22 @@ TEST_F(TestAT_CellularStack, test_AT_CellularStack_socket_open)
ATHandler at(&fh1, que, 0, ",");

MyStack st(at, 0, IPV6_STACK, *_dev);
st.bool_value = false;
AT_CellularDevice_stub::supported_bool = 0;
AT_CellularDevice_stub::max_sock_value = 1;
EXPECT_EQ(st.socket_open(NULL, NSAPI_TCP), NSAPI_ERROR_UNSUPPORTED);

st.bool_value = true;
st.max_sock_value = 0;
AT_CellularDevice_stub::supported_bool = 1;
nsapi_socket_t sock = &st.socket;
AT_CellularDevice_stub::max_sock_value = 0;
EXPECT_EQ(st.socket_open(&sock, NSAPI_TCP), NSAPI_ERROR_NO_SOCKET);

MyStack st2(at, 0, IPV6_STACK, *_dev);
st2.bool_value = true;
st2.max_sock_value = 1;
AT_CellularDevice_stub::max_sock_value = 1;
sock = &st2.socket;
EXPECT_EQ(st2.socket_open(&sock, NSAPI_TCP), NSAPI_ERROR_OK);

AT_CellularDevice_stub::max_sock_value = 1; // value must be the same as before the first open
}

TEST_F(TestAT_CellularStack, test_AT_CellularStack_socket_close)
Expand All @@ -233,13 +225,13 @@ TEST_F(TestAT_CellularStack, test_AT_CellularStack_socket_close)

nsapi_socket_t sock = &st.socket;
st.bool_value = true;
st.max_sock_value = 1;
AT_CellularDevice_stub::max_sock_value = 1;
EXPECT_EQ(st.socket_open(&sock, NSAPI_TCP), NSAPI_ERROR_OK);
st.max_sock_value = 0;
AT_CellularDevice_stub::max_sock_value = 0;
EXPECT_EQ(st.socket_close(sock), NSAPI_ERROR_DEVICE_ERROR);

MyStack st2(at, 0, IPV6_STACK, *_dev);
st2.max_sock_value = 1;
AT_CellularDevice_stub::max_sock_value = 1;
st2.bool_value = true;
sock = &st2.socket;
EXPECT_EQ(st2.socket_open(&sock, NSAPI_TCP), NSAPI_ERROR_OK);
Expand Down Expand Up @@ -306,7 +298,7 @@ TEST_F(TestAT_CellularStack, test_AT_CellularStack_socket_send)
EXPECT_EQ(st.socket_send(&st.socket, "addr", 4), NSAPI_ERROR_NO_CONNECTION);

SocketAddress addr("fc00::", 123);
st.max_sock_value = 1;
AT_CellularDevice_stub::max_sock_value = 1;
st.bool_value = true;
nsapi_socket_t sock = &st.socket;
st.socket_open(&sock, NSAPI_TCP);
Expand All @@ -325,7 +317,7 @@ TEST_F(TestAT_CellularStack, test_AT_CellularStack_socket_sendto)
SocketAddress addr("fc00::", 123);
EXPECT_EQ(st.socket_sendto(NULL, addr, "addr", 4), NSAPI_ERROR_NO_SOCKET);

st.max_sock_value = 1;
AT_CellularDevice_stub::max_sock_value = 1;
st.bool_value = true;
nsapi_socket_t sock = &st.socket;
st.socket_open(&sock, NSAPI_TCP);
Expand Down Expand Up @@ -359,7 +351,7 @@ TEST_F(TestAT_CellularStack, test_AT_CellularStack_socket_recvfrom)
EXPECT_EQ(st.socket_recvfrom(NULL, NULL, table, 4), NSAPI_ERROR_NO_SOCKET);

SocketAddress addr;
st.max_sock_value = 1;
AT_CellularDevice_stub::max_sock_value = 1;
st.bool_value = true;
nsapi_socket_t sock = &st.socket;
st.socket_open(&sock, NSAPI_TCP);
Expand All @@ -380,7 +372,7 @@ TEST_F(TestAT_CellularStack, test_AT_CellularStack_socket_attach)
MyStack st(at, 0, IPV6_STACK, *_dev);

st.socket_attach(NULL, NULL, NULL);
st.max_sock_value = 1;
AT_CellularDevice_stub::max_sock_value = 1;
st.bool_value = true;
nsapi_socket_t sock = &st.socket;
st.socket_open(&sock, NSAPI_TCP);
Expand Down
12 changes: 5 additions & 7 deletions UNITTESTS/stubs/AT_CellularDevice_stub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ int AT_CellularDevice_stub::set_pin_failure_count = 0;
int AT_CellularDevice_stub::get_sim_failure_count = 0;
bool AT_CellularDevice_stub::pin_needed = false;
bool AT_CellularDevice_stub::supported_bool = false;
int AT_CellularDevice_stub::max_sock_value = 0;

AT_CellularDevice::AT_CellularDevice(FileHandle *fh) : CellularDevice(fh),
#if MBED_CONF_CELLULAR_USE_SMS
Expand All @@ -48,7 +49,7 @@ AT_CellularDevice::~AT_CellularDevice()

ATHandler *AT_CellularDevice::get_at_handler(FileHandle *fileHandle)
{
return ATHandler::get_instance(fileHandle, _queue, _default_timeout, "\r", get_send_delay(), _modem_debug_on);
return ATHandler::get_instance(fileHandle, _queue, _default_timeout, "\r", get_property(AT_CellularDevice::PROPERTY_AT_SEND_DELAY), _modem_debug_on);
}

ATHandler *AT_CellularDevice::get_at_handler()
Expand Down Expand Up @@ -87,7 +88,7 @@ CellularNetwork *AT_CellularDevice::open_network(FileHandle *fh)
_queue,
_default_timeout,
"\r",
get_send_delay(),
get_property(AT_CellularDevice::PROPERTY_AT_SEND_DELAY),
_modem_debug_on), *this);
return _network;
}
Expand Down Expand Up @@ -163,11 +164,6 @@ void AT_CellularDevice::set_timeout(int timeout)
_default_timeout = timeout;
}

uint16_t AT_CellularDevice::get_send_delay() const
{
return 0;
}

void AT_CellularDevice::modem_debug_on(bool on)
{
_modem_debug_on = on;
Expand Down Expand Up @@ -293,6 +289,8 @@ intptr_t AT_CellularDevice::get_property(CellularProperty key)
return true;
} else if (key == PROPERTY_IPV4_PDP_TYPE) {
return true;
} else if (key == PROPERTY_SOCKET_COUNT) {
return AT_CellularDevice_stub::max_sock_value;
}

return AT_CellularDevice_stub::supported_bool;
Expand Down
1 change: 1 addition & 0 deletions UNITTESTS/stubs/AT_CellularDevice_stub.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ extern int set_pin_failure_count;
extern int get_sim_failure_count;
extern bool pin_needed;
extern bool supported_bool;
extern int max_sock_value;
}


Expand Down
5 changes: 0 additions & 5 deletions UNITTESTS/target_h/myCellularDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ class myCellularDevice : public AT_CellularDevice {

virtual void set_timeout(int timeout) {}

virtual uint16_t get_send_delay() const
{
return 0;
}

virtual void modem_debug_on(bool on) {}

virtual nsapi_error_t init()
Expand Down
9 changes: 2 additions & 7 deletions features/cellular/framework/AT/AT_CellularDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ void AT_CellularDevice::setup_at_handler()
{
set_at_urcs();

_at->set_send_delay(get_send_delay());
_at->set_send_delay(get_property(AT_CellularDevice::PROPERTY_AT_SEND_DELAY));
}

void AT_CellularDevice::urc_nw_deact()
Expand Down Expand Up @@ -188,7 +188,7 @@ ATHandler *AT_CellularDevice::get_at_handler(FileHandle *fileHandle)
}

return ATHandler::get_instance(fileHandle, _queue, _default_timeout,
"\r", get_send_delay(), _modem_debug_on);
"\r", get_property(AT_CellularDevice::PROPERTY_AT_SEND_DELAY), _modem_debug_on);
}

ATHandler *AT_CellularDevice::get_at_handler()
Expand Down Expand Up @@ -445,11 +445,6 @@ void AT_CellularDevice::set_timeout(int timeout)
}
}

uint16_t AT_CellularDevice::get_send_delay() const
{
return 0;
}

void AT_CellularDevice::modem_debug_on(bool on)
{
_modem_debug_on = on;
Expand Down
7 changes: 4 additions & 3 deletions features/cellular/framework/AT/AT_CellularDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ class AT_CellularDevice : public CellularDevice {
PROPERTY_NON_IP_PDP_TYPE, // 0 = not supported, 1 = supported. Does modem support Non-IP?
PROPERTY_AT_CGEREP, // 0 = not supported, 1 = supported. Does modem support AT command AT+CGEREP.
PROPERTY_AT_COPS_FALLBACK_AUTO, // 0 = not supported, 1 = supported. Does modem support mode 4 of AT+COPS= ?

PROPERTY_SOCKET_COUNT, // The number of sockets of modem IP stack
PROPERTY_IP_TCP, // 0 = not supported, 1 = supported. Modem IP stack has support for TCP
PROPERTY_IP_UDP, // 0 = not supported, 1 = supported. Modem IP stack has support for TCP
PROPERTY_AT_SEND_DELAY, // Sending delay between AT commands in ms
PROPERTY_MAX
};

Expand Down Expand Up @@ -98,8 +101,6 @@ class AT_CellularDevice : public CellularDevice {

virtual void set_timeout(int timeout);

virtual uint16_t get_send_delay() const;

virtual void modem_debug_on(bool on);

virtual nsapi_error_t init();
Expand Down
52 changes: 33 additions & 19 deletions features/cellular/framework/AT/AT_CellularStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,30 +25,32 @@ using namespace mbed_cellular_util;
using namespace mbed;

AT_CellularStack::AT_CellularStack(ATHandler &at, int cid, nsapi_ip_stack_t stack_type, AT_CellularDevice &device) :
_socket(NULL), _socket_count(0), _cid(cid),
_socket(NULL), _cid(cid),
_stack_type(stack_type), _ip_ver_sendto(NSAPI_UNSPEC), _at(at), _device(device)
{
memset(_ip, 0, PDP_IPV6_SIZE);
}

AT_CellularStack::~AT_CellularStack()
{
for (int i = 0; i < _socket_count; i++) {
if (_socket[i]) {
delete _socket[i];
_socket[i] = NULL;
if (_socket) {
for (int i = 0; i < _device.get_property(AT_CellularDevice::PROPERTY_SOCKET_COUNT); i++) {
if (_socket[i]) {
delete _socket[i];
_socket[i] = NULL;
}
}
delete [] _socket;
_socket = NULL;
}
_socket_count = 0;

delete [] _socket;
_socket = NULL;
}

int AT_CellularStack::find_socket_index(nsapi_socket_t handle)
{
int max_socket_count = get_max_socket_count();
for (int i = 0; i < max_socket_count; i++) {
if (!_socket) {
return -1;
}
for (int i = 0; i < _device.get_property(AT_CellularDevice::PROPERTY_SOCKET_COUNT); i++) {
if (_socket[i] == handle) {
return i;
}
Expand Down Expand Up @@ -158,23 +160,36 @@ nsapi_error_t AT_CellularStack::socket_stack_init()

nsapi_error_t AT_CellularStack::socket_open(nsapi_socket_t *handle, nsapi_protocol_t proto)
{
if (!is_protocol_supported(proto) || !handle) {
if (!handle) {
return NSAPI_ERROR_UNSUPPORTED;
}

int max_socket_count = get_max_socket_count();
if (proto == NSAPI_UDP) {
if (!_device.get_property(AT_CellularDevice::PROPERTY_IP_UDP)) {
Copy link
Contributor

@kivaisan kivaisan Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic, but maybe these could be combined into one if -sentence, like:
if (proto == NSAPI_UDP && !_device.get_property(AT_CellularDevice::PROPERTY_IP_UDP)) {

return NSAPI_ERROR_UNSUPPORTED;
}
} else if (proto == NSAPI_TCP) {
if (!_device.get_property(AT_CellularDevice::PROPERTY_IP_TCP)) {
return NSAPI_ERROR_UNSUPPORTED;
}
} else {
return NSAPI_ERROR_UNSUPPORTED;
}

_socket_mutex.lock();

if (!_socket) {
if (_device.get_property(AT_CellularDevice::PROPERTY_SOCKET_COUNT) == 0) {
_socket_mutex.unlock();
return NSAPI_ERROR_NO_SOCKET;
}
if (socket_stack_init() != NSAPI_ERROR_OK) {
_socket_mutex.unlock();
return NSAPI_ERROR_NO_SOCKET;
}

_socket = new CellularSocket*[max_socket_count];
_socket_count = max_socket_count;
for (int i = 0; i < max_socket_count; i++) {
_socket = new CellularSocket*[_device.get_property(AT_CellularDevice::PROPERTY_SOCKET_COUNT)];
for (int i = 0; i < _device.get_property(AT_CellularDevice::PROPERTY_SOCKET_COUNT); i++) {
_socket[i] = 0;
}
}
Expand Down Expand Up @@ -435,8 +450,7 @@ void AT_CellularStack::socket_attach(nsapi_socket_t handle, void (*callback)(voi

int AT_CellularStack::get_socket_index_by_port(uint16_t port)
{
int max_socket_count = get_max_socket_count();
for (int i = 0; i < max_socket_count; i++) {
for (int i = 0; i < _device.get_property(AT_CellularDevice::PROPERTY_SOCKET_COUNT); i++) {
if (_socket[i]->localAddress.get_port() == port) {
return i;
}
Expand All @@ -447,7 +461,7 @@ int AT_CellularStack::get_socket_index_by_port(uint16_t port)
AT_CellularStack::CellularSocket *AT_CellularStack::find_socket(int sock_id)
{
CellularSocket *sock = NULL;
for (int i = 0; i < _socket_count; i++) {
for (int i = 0; i < _device.get_property(AT_CellularDevice::PROPERTY_SOCKET_COUNT); i++) {
if (_socket[i] && _socket[i]->id == sock_id) {
sock = _socket[i];
break;
Expand Down
Loading