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.

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

UBLOX cellular api's for UDP and TCP #7619

Merged
merged 5 commits into from Aug 21, 2018

Conversation

Projects
None yet
8 participants
@mudassar-ublox
Contributor

mudassar-ublox commented Jul 27, 2018

Description

This PR updates the cellular API's for the u-blox targets. Some u-blox targets must use an internal IP stack while others can use both internal and external IP stacks. This PR introduces support for the internal IP stack; a new cellular target type, called UBLOX_AT, is added in "CellularTargets.h" and AT classes are added in targets/UBLOX. The API to the internal IP stack supports both UDP and TCP socket operations.

Supported Targets: UBLOX_C030_U201
Tool Chains: ARM, GCC_ARM, IAR
ARM_Build.txt
GCC_ARM_Build.txt
IAR_Build.txt

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[x] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Jul 27, 2018

@AriParkkila

This looks quite good. Description could tell TCP is client only.

@@ -98,6 +98,7 @@ class AT_CellularStack : public NetworkStack, public AT_CellularBase
bool started; // socket has been opened on modem stack
bool tx_ready; // socket is ready for sending on modem stack
bool rx_avail; // socket has data for reading on modem stack
volatile nsapi_size_t pending_bytes; // The number of received bytes pending

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

You could probably use rx_avail flag here just to indicate that some data is available because you always read how much data really is available when recveiving? If not then you could change type of rx_avail and use it instead of new pending_bytes variable.

There is no need to use volatile here.

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

If rx_avail is unused and nobody else is using it then we can change its type and use it as well.

@@ -0,0 +1,59 @@
/*
* Copyright (c) 2017, Arm Limited and affiliates.

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

Years are passing so fast, it's already 2018 :) See also the other files.

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

Updated.

{
public:
UBLOX_AT(events::EventQueue &queue);

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

Please run astyle.exe -n --options=.astylerc <file> for coding guidelines.

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

Updated all files.

bool UBLOX_AT_CellularNetwork::has_registration(RegistrationType reg_type)
{
return (reg_type == C_REG || reg_type == C_GREG);

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

LTE / NB-IoT should be C_EREG instead of C_GREG.

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

It is not LTE or NB-Iot module so no need to add 'C_EREG '.

nsapi_error_t UBLOX_AT_CellularNetwork::detach()
{
_at.lock();

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

Could call parent implementation AT_CellularNetwork::detach();?

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

After detach we are receiving two urc's, '+CGREG' and '+UUPSDD', to process these urc's we have added 50 ms wait here otherwise "CellularNetwork test detach" fails.

This comment has been minimized.

@AriParkkila

AriParkkila Aug 2, 2018

Contributor

Could you add wait just before when "test detach" is called?

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Aug 2, 2018

Contributor

Are you suggesting to add wait in CellularNetwork test detach test case?
if yes then I can remove my detach() function and will use parent implementation.

_at.resp_start();
_at.resp_stop();
_at.unlock();

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

get_last_error() before unlocking.

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

Updated.

return NSAPI_ERROR_DEVICE_ERROR;
}
_at.lock();

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

lock not needed due to socket_sendto_impl has locks.

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

Updated.

CellularSocket *socket = NULL;
for (unsigned int x = 0; (socket == NULL) && (x < UBLOX_MAX_SOCKET); x++) {
if (_socket) {

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

If _socket is NULL then we probably want to assert :)

_at.resp_start("+UDNSRN:");
if (_at.info_resp()) {
_at.read_string(ipAddress, sizeof(ipAddress));
_at.resp_stop();

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

It doesn't change anything here but logically resp_stop would be in the same scope with resp_start

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

Updated.

#endif
/* wait for modem to power off properly*/
wait(10);

This comment has been minimized.

@AriParkkila

AriParkkila Jul 27, 2018

Contributor

Is this really needed here?

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Jul 30, 2018

Contributor

Updated.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jul 27, 2018

@mudassar-ublox mudassar-ublox force-pushed the u-blox:cellular_ublox_udp_tcp_imp branch from 0fa9e3a to cd7dc50 Jul 30, 2018

@mudassar-ublox mudassar-ublox force-pushed the u-blox:cellular_ublox_udp_tcp_imp branch from cd7dc50 to ffb4f92 Jul 30, 2018

@mudassar-ublox

This comment has been minimized.

Contributor

mudassar-ublox commented Aug 1, 2018

@AriParkkila please review again. Do you have any other queries?

uint8_t ch = 0;
int port = 0;
Timer timer;

This comment has been minimized.

@AriParkkila

AriParkkila Aug 2, 2018

Contributor

An application may call recvfrom even when there is no incoming data. In these cases function should return immediately with NSAPI_ERROR_WOULD_BLOCK (async) instead of waiting for timer.read_ms() < SOCKET_TIMEOUT).

_at.write_int(address.get_port());
_at.write_int(size);
_at.cmd_stop();
wait_ms(50);

This comment has been minimized.

@AriParkkila

AriParkkila Aug 2, 2018

Contributor

mbed_poll is a better way to wait for an input

nsapi_error_t UBLOX_AT_CellularNetwork::detach()
{
_at.lock();

This comment has been minimized.

@AriParkkila

AriParkkila Aug 2, 2018

Contributor

Could you add wait just before when "test detach" is called?

@NeilMacMullen

This comment has been minimized.

NeilMacMullen commented Aug 2, 2018

See #7679 and #7680 for issues that affect SARA-R4

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 9, 2018

@AriParkkila In reference to those linked issues, would it be possible for this PR to continue if those are left open, or are those issues blocking basic functionality and/or would they cause tests to fail?

@fahim-ublox

This comment has been minimized.

Contributor

fahim-ublox commented Aug 9, 2018

+UDNSRN will be supported in later modem firmware versions of R410M variant. This pull request was generated after all tests getting passed.

@AriParkkila

I think you should still fix async socket reading. If recv waits for an second that causes unnecessary unresponsiveness and battery consumption.

uint8_t ch = 0;
int port = 0;
Timer timer;

This comment has been minimized.

@AriParkkila

AriParkkila Aug 13, 2018

Contributor

You can always return NSAPI_ERROR_WOULD_BLOCK:

In blocking mode Mbed OS socket layer waits for a message until socket-timeout and if one is received via URC then socket layer calls recvfrom again to actually read the message.

In non-blocking mode control returns immediately to a calling application. When message is received via URC it issues callback on an application, then the application needs to call recvfrom again to actually read the message.

@@ -275,6 +273,13 @@ nsapi_size_or_error_t UBLOX_AT_CellularStack::socket_recvfrom_impl(CellularSocke
int port = 0;
Timer timer;
if (!socket->rx_avail) {

This comment has been minimized.

@AriParkkila

AriParkkila Aug 15, 2018

Contributor

I think rx_avail should not be cleared in sendto, you need to set it in URC and reset after all is read in recvfrom.

Because you are now updating pending_bytes in URC, you could use that as well and remove rx_avail completely, and then check for if (socket->pending_bytes > 0).

This comment has been minimized.

@mudassar-ublox

mudassar-ublox Aug 15, 2018

Contributor

Are you saying that remove rx_avail from my class completely and use pending_bytes only, for checking availability of data as well.

@AriParkkila

Please fix rx_avail and it looks good to me.

@mudassar-ublox

This comment has been minimized.

Contributor

mudassar-ublox commented Aug 15, 2018

Updated and removed rx_avail, using pending_bytes for availability of data. @AriParkkila Please review now.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 17, 2018

Build : SUCCESS

Build number : 2823
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7619/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

Export failure does not appear related to PR.
/morph export-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2018

/morph export-build
/morph uvisor-test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2018

Jenkins machine fail, restarting

/morph export-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2018

@ARMmbed/mbed-os-test Can you review the above exporter failure (IllegalStateException)

@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented Aug 21, 2018

@0xc0170 Seems to be jenkins failure. Please rerun export job

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 21, 2018

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 17a525c into ARMmbed:master Aug 21, 2018

15 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 584 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9204 cycles (-1087 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment