Skip to content
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

Mbed OS cellular connectivity #6082

Merged
merged 84 commits into from
Mar 2, 2018
Merged

Mbed OS cellular connectivity #6082

merged 84 commits into from
Mar 2, 2018

Conversation

AriParkkila
Copy link

Description

First implementation for Mbed OS cellular connectivity in the folder mbed-os/features/cellular

easy_cellular : simplified usage based on CellularBase.h

framework :
    API             Application Programming Interface for Cellular
    AT              AT implementation based on 3GPP TS 27.007 specification
    common    Common and utility sources
    mux           MUX implementation based on 3GPP TS 27.010 specification
    targets       Vendor specific cellular module adaptations

Status

READY

Migrations

NO

Related PRs

NO

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 13, 2018

Please look at jenkins CI failure, compiler error related. Also travis docs failed

@@ -0,0 +1,13 @@
coverage/
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this covered in root gitignore? thus this file could be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

FIle can't be removed because of unit tests. That one line can be removed.

* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "CppUTest/TestHarness.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a documentation how to run these tests (where cpputest icomes from, etc)?

Choose a reason for hiding this comment

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

Not yet. There is ongoing discussion around unittesting in general. CppUTest is one of the options, but we might switch to something else as well. When we have decided the mbed OS wide unittest tool, we need to document that for the whole mbed OS, not just for Cellular. There is a run_tests script in UNITTESTS folder and I think it is quite self-explanatory. Do you think we should add "readme" or similar under UNITTESTS folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

just want to understand why UNITTEST folder is also part of this PR . If it is added, then it needs to be documented how it is used, otherwise it can ber considered "dead" code - why it is here then?

Choose a reason for hiding this comment

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

Product decision. README.md updated with instructions


#include <UARTSerial.h>
#include <NetworkInterface.h>
#include <EventQueue.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

why are <> used here (how are they different from line 25/26)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed to double quotations. Brackets now used only for headers that come from toolchain.
AriParkkila@85f05f4

#define MBED_TRACE_MAX_LEVEL TRACE_LEVEL_INFO
#endif
#include "CellularLog.h"
/*#define log_debug printf
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed dead code.


/* PDP Context information */
struct pdpcontext_params_t {
char apn[100+1];
Copy link
Contributor

Choose a reason for hiding this comment

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

are these numbers defined anywhere (100 and 63) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

3gpp defines lengths, added defines

// *
// * @return zero for success
// */
// nsapi_error_t set_csms(int msg_service);
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code

Copy link
Contributor

Choose a reason for hiding this comment

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

removed


namespace mbed {

#define UART 1
Copy link
Contributor

Choose a reason for hiding this comment

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

very short names that might conflict with targets (UART or MUX). what is the use of these?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not used --> removed

@0xc0170 0xc0170 requested review from sg- and kjbracey February 13, 2018 15:06
if (socket->localAddress.get_port() == 0) {
socket->localAddress.set_port(get_dynamic_ip_port());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

bind() can independently set local port and local address. If either are zero, that's a no-op for that field. This should be more like "if address is non-zero, set address; if port is non-zero, set port". A call with port 0 shouldn't affect any already-chosen port.

Copy link
Contributor

Choose a reason for hiding this comment

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

if (!socket) {
return NSAPI_ERROR_DEVICE_ERROR;
}
socket->localAddress = addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

bind() sets address and port independently. If they're zero it's a no-op. This should be "if address is non-zero, set address; if port is non-zero, set port". If they're zero, don't change what's already been set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mirela fixed this : 5e04288

nsapi_size_or_error_t recv_len=0;
int port;
char ip_address[NSAPI_IP_SIZE];
char hexstr[BC95_MAX_PACKET_SIZE*2 + 1] = {0};
Copy link
Contributor

Choose a reason for hiding this comment

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

You're wasting a lot of CPU time setting this temporary array to zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

return;
}
#ifdef MBED_CONF_RTOS_PRESENT
rtos::Thread::yield();
Copy link
Contributor

Choose a reason for hiding this comment

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

Yield doesn't really achieve very much - you're still using 100% of CPU power and it won't let lower-priority threads run. You should use a timed poll() similar to the existing ATCmdParser or the PPP glue code.

Copy link
Contributor

Choose a reason for hiding this comment

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

internal ticket created: IOTCELL-556

void ATHandler::reset_buffer()
{
log_debug("%s", __func__);
memset(_recv_buff, 0, sizeof(_recv_buff));
Copy link
Contributor

Choose a reason for hiding this comment

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

Waste of CPU time/power? No need to set unused buffer space to anything.

Copy link
Contributor

Choose a reason for hiding this comment

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


nsapi_error_t AT_CellularNetwork::set_registration_urc(bool urc_on)
{
RegistrationType reg_types[] = {C_EREG, C_GREG, C_REG};
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be a bit more const, and possibly static - const RegistrationType & const char * const cmd_on

Copy link
Author

Choose a reason for hiding this comment

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

Added static const, commit
AriParkkila@9ec4cb8

return _at.get_last_error();
}

nsapi_error_t AT_CellularNetwork::set_registration(char *plmn)
Copy link
Contributor

Choose a reason for hiding this comment

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

const

Copy link
Author

Choose a reason for hiding this comment

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

virtual off_t seek(off_t offset, int whence = SEEK_SET);

/** Not supported by the implementation. */
virtual int close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if this internal file handle doesn't support blocking, it still should support poll() so that readable and writable work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Managed by IOTCELL-571

action was taken. */

tx_callback_pending_bit_set(dlci_id);
write_ret = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

-EAGAIN is the correct return value for "I'm not sending anything because buffers full". Write never normally returns 0 unless asked to send 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Managed by IOTCELL-567


ssize_t Mux::user_data_tx(uint8_t dlci_id, const void* buffer, size_t size)
{
MBED_ASSERT(size <=
Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to be providing the basic "unstructured octect stream" convergance layer, which means you should be acting as a stream file handle. You must accept any size here.

If we accept the non-blocking limitation (which will work with PPP and the AT Parser), I believe that means you just need to limit the size to your maximum, and return how much you actually managed to fit in 1 frame. Any user that knows they're using a non-blocking FileHandle will cope with that.

For blocking (eg if you just want to printf from the C library over the mux), you would need to construct a loop to write however many frames are necessary.

I would say blocking really must be implemented if this is to be generally useful, but that can wait until later. You do need to make sure poll() works though. Is the lack of poll why the ATHandler is spinning?

Copy link
Contributor

Choose a reason for hiding this comment

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

IOTCELL-566 created for possible future implementation change regarding this feedback.

Blocking mode is not supported by the current implementation and for my knowledge no plans to implement that in the future.

Regarding "ATHandler is spinning" feedback: Propably managed by IOTCELL-556: added comment there for tracking

Copy link
Contributor

Choose a reason for hiding this comment

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

The poll() functionality and write size functionality are clearly broken and not acceptable for merging to master as-is. This mux has one job to do, and that's to present itself as a functioning virtual serial port.

The limitation on no blocking support is something that could be documented, but the non-blocking support has to actually work correctly.

How does this even work with PPP as it is? I guess it doesn't?

Copy link
Contributor

Choose a reason for hiding this comment

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

On future blocking support, I think it would make sense to introduce some sort of "make blocking from non-blocking" helper to avoid having to implement extra logic for blocking in every non-blocking-capable FileHandle provider.

That's the way sockets work, and it's super convenient - you can just keep returning -EAGAIN and not worry whether it's blocking mode or not. FileHandle just lacks that because it historically was always blocking-only and non-blocking was bolted on as an extension afterwards.

I'll look into what this would look like. It would be continuing work from the "poll wake" stuff here: https://github.com/kjbracey-arm/mbed-os/commits/cv_cs

Copy link
Contributor

Choose a reason for hiding this comment

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

We have created JIRA tasks for future improvements and no implementation changes are planned at this point.

Do you consider that these are blocking the PR merge at this point?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. See my comment below.

Copy link
Contributor

@blind-owl blind-owl Feb 19, 2018

Choose a reason for hiding this comment

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

Fixed with commits:
1245c14
ee5d27f


void log_init(PinName tx, PinName rx, int baud)
{
#ifdef FEATURE_COMMON_PAL
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would just turn on FEATURE_COMMON_PAL, if you want to use mbed-trace. Any reason why not? (Anyone?)

Copy link
Author

Choose a reason for hiding this comment

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

mbed_trace was taken into use in cellular AriParkkila@5c8a2cb

mbed_trace_mutex_release_function_set(serial_unlock);
mbed_trace_init();
#else
serial_init(&stdio_uart, tx, rx);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit evil, touching these structures. I don't think you should be interfering with the console/stdio output settings. (You aren't in the mbed trace case).

(Besides, who's to say the console is even serial? mbed_trace just defaults to stdout, and debug() to stderr, whatever they are.).

Copy link
Author

Choose a reason for hiding this comment

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

File was removed in AriParkkila/mbed-os@5c8a2cb

serial_baud(&stdio_uart, baud);
stdio_uart_inited = 1;
#endif
log_info("\r\n\r\n**************************************");
Copy link
Contributor

Choose a reason for hiding this comment

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

If actually using mbed trace, each output should be a complete line, so if you want blank lines, you should technically do just two "" calls

Copy link
Author

Choose a reason for hiding this comment

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

File was removed in AriParkkila@5c8a2cb


#include <QUECTEL_BC95_CellularPower.h>

#include "onboard_modem_api.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not actually using this?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, removed include


namespace mbed {

class UBLOX_C027 : public AT_CellularDevice
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a misnomer? The C027 is the board, not the modem.

Indeed, is there even anything C027-specific here? Isn't this just the "generic onboard PPP modem, using onboard power APIs?"

Copy link
Author

Choose a reason for hiding this comment

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

As a placeholder, renamed AT module to LISA_U
AriParkkila@7392e73

#define EASY_CELLULAR_CONNECTION_H

#include "CellularConnectionUtil.h"
#ifdef CELLULAR_DEVICE
Copy link
Contributor

Choose a reason for hiding this comment

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

Having done this, you should be able to cross-link to "OnboardCellularInterface.h" so that it includes this and typedefs OnboardCellularInterface as this object (ifdef CELLULAR_DEVICE), which should(?) totally eliminate the need for any app changes.

The intent was always that people providing onboard modems should arrange for them to appear as OnboardCellularInterface.

Copy link
Author

Choose a reason for hiding this comment

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

If application has defined CELLULAR_DEVICE (in mbed_app.json or mbed target for onboard modem) then modem is selected from cellular/targets/ folder. If application wants to use OnboardCellularInterface.h then it needs to specifically instantiate some class implementing that. That choice has now been intentionally left to application developers.

Copy link
Contributor

@kjbracey kjbracey Feb 20, 2018

Choose a reason for hiding this comment

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

When designing the last round of cellular, what we ended up with was an attempt to achieve the "best of both worlds" for modem selection, while aligning with general design mbed OS architectural style.

For Ethernet, applications can usually just say EthernetInterface, because the target arranges for that to "do the right thing", accessing the Ethernet usually built into the main chip. For an add-on Wi-fi board, the application has to say XXXWiFi(pin, pin, pin).

The style in mbed OS so far has been for "on-main-chip" devices to be part of the "target", and hence automatically available to portable applications without explicit specification. You just specify the target. Plug-in things have to be explicitly specified, so applications doing that are not automatically portable.

Part of what I've been trying to work toward generally, and discussing with @sg-, is to allow the same driver (or a variant) to be both available for manual specification, and to be "just there" if it is actually built in to a target. The bulk of review (and redesign) last time was on this point.

The intent with OnboardCellularInterface was that someone could have a custom XXXModem(pin, pin, pin) that would work flexibly, and on any target. But if they actually built it into a target, the target would arrange for OnboardCellularInterface to be based on that general driver, with default constructor using all the correct pins and power controls, so that the application gets the same ease of use they get for EthernetInterface, due to it being target-specified.

Your framework would fit into that mechanism. If your target.json is defining CELLULAR_DEVICE to arrange for the right driver to be present because it's on the board, it makes sense for you to be pointing OnboardCellularInterface at that.

If you were talking about a non-target-specified modem, then the general style would be for the application to specify the modem by invoking its specific class, rather than JSON/define setting. I don't think applications should be setting CELLULAR_DEVICE (or its equivalent) - they should be giving it by name. That does mean you wouldn't have an equivalent to "easy cellular" for the offboard, but my understanding that that was out of immediate scope, and we were assuming on-board for now.

I'll spend some time discussing this with @sg- today or tomorrow to try to make sure we've got common architectural agreement here, then we can agree what to do on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, for reference and comparison with other non-cellular things, which are works in progress, see: #6108

There you see "OnboardModemInterface" as fitting in as one of the sub-defaults of the overall "default network interface".

It's not following the new pattern I'm establishing - that would be CellularBase::get_default_instance() - but it achieves a similar effect. Maybe it could be deprecated in favour of that sort of call?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, the decision is that we will be doing the CellularBase::get_default_instance() and its the equivalents for non-cellular for 5.9, but for 5.8 we just stick with the existing 5.7 abstraction design - use OnboardCellularInterface as the abstraction point.

Now that you have something which should be a fully-fledged replacement for the 4 in-tree targets defining MODEM_ON_BOARD_UART (Ublox C027 & C030 and the 2 Dragonfly boards) including that APN functionality, I think you can switch those to the new code by changing the structure of OnboardCellularInterface to flow something like this:

#include "EasyCellularConnection.h"
#ifdef CELLULAR_DEVICE
  typedef EasyCellularConnection OnboardCellularInterface;
#elif MODEM_ON_BOARD && MODEM_ON_BOARD_UART && NSAPI_PPP_AVAILABLE
  #include "UARTCellularInterface.h"
  class OnboardCellularInterface : public UARTCellularInterface;
#endif

That should mean that the client and cellular examples work without modification on those 4 targets and your new ones, as long as CELLULAR_DEVICE is being set automatically by the FEATURE_TARGET filtering. (Are there any remaining issues about necessary defines/configs?)

Any other targets we don't know about which set MODEM_ON_BOARD_UART will continue to use the 5.7 PPP driver. (In principle though, can't you're framework have a generic/default driver that would be able to act like UARTCellularInterface - I still think the Lisa U one is basically generic? Something to think about when removing that functionality).

In 5.9, we will have CellularBase::get_default_instance(), and that will give you more flexibility than that simple typedef - you would be able to construct something and maybe do more configuration. The OnboardCellularInterface point would be deprecated in favour of that.

We can also do CellularDevice::get_default_instance() for the full API then.

Annoyingly I see one error in mbed-os-example-cellular - it's calling iface.modem_debug_on, and that's not a CellularBase method, so would fail. Docs do say

Depending on the type of on-board modem, OnboardCellularInterface
could be derived from different implementation classes.
Portable applications should only rely on it being a CellularBase.

Adding that method to EasyCellular would avoid that compile failing, or we could remove the call from the app or make it compile time conditional somehow.

Copy link
Author

Choose a reason for hiding this comment

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

Typedef onboard cellular to easy cellular:
AriParkkila@a3d8783

Copy link
Author

Choose a reason for hiding this comment

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

Now after typedef https://github.com/ARMmbed/mbed-os-example-cellular can use this PR without any changes.

* @remark Must be called before any other methods
* @return see nsapi_error_t, 0 on success
*/
nsapi_error_t init();
Copy link
Contributor

@kjbracey kjbracey Feb 14, 2018

Choose a reason for hiding this comment

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

NetworkInterface objects are generally self-initialising. Initialisation that require power and/or cause errors, so you don't want to do them in your constructor, can normally be deferred to first connect.

(Ideally disconnect would be a full power shutdown, so whatever you do in init would always happen on every new connect. Most current drivers don't really close everything though - they leave serial ports open, etc, so those are first connect only).

Doing that would be part of making this a transparent OnboardCellularInterface provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

init is no longer needed to call from application


NetworkStack *EasyCellularConnection::get_stack()
{
#if NSAPI_PPP_AVAILABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this knowledge extend out here to a user of the framework? Why doesn't the underlying CellularDevice return the PPP stack transparently?

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed


static CellularConnectionUtil cellularConnection;
static rtos::Semaphore cellularSemaphore(0);
static UARTSerial cellularSerial(MDMTXD, MDMRXD, MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't just define these objects as static - they will be constructed and included in the image even if an EasyCellularConnection is never used.

Make them normal class members to avoid this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 or use SingletonPtr class

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed as class member variables


RegistrationType reg_types[] = { C_EREG, C_GREG, C_REG};
const char *cmd[] = { "AT+CEREG", "AT+CGREG", "AT+CREG"};
const char *rsp[] = { "+CEREG: ", "+CGREG: ", "+CREG: "};
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra whitespace after ":" is preventing matching the response, in case modem returns the response without the whitespace, like "+CEREG:". AT Handler is already looking for possible whitespaces when trying to match, so no need to have the whitespace in this definition.

Copy link
Contributor

Choose a reason for hiding this comment

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


memset(_apn, 0, MAX_ACCESSPOINT_NAME_LENGTH);

#ifdef MBED_CONF_APP_CELLULAR_APN
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an (hypothetical) application config setting - you can't access that from here.

If you want a default APN setting JSON it needs to be in your own cellular mbed_lib.json.

Copy link
Author

Choose a reason for hiding this comment

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

Changed APN initialization from CellularNetwork to application responsibility
AriParkkila@31c4e40

#define MUX_DLCI_INVALID_ID 0 /* Invalid DLCI ID. Used to invalidate MuxDataService object. */
#define MUX_CRC_TABLE_LEN 256u /* CRC table length in number of bytes. */

#ifndef MBED_CONF_MUX_DLCI_COUNT
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're adding mbed conf options, you need to actually have a mbed_lib.json, otherwise no-one can change them.

You can also put the defaults and help in there - no need to have a default here unless it needs to compile outside mbed OS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Managed by IOTCELL-570

@juhoeskeli
Copy link
Contributor

Implemented status callbacks introduced by PR #6032 to cellular connectivity. Added missing functionality to AT_CellularNetwork::disconnect()

@kimlep01
Copy link

Greentea tests are missing

Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

Okay, so I've provided quite a long list of comments so far.

My trickier "efficiency"-related issues can be deferred.

On the details of the internal framework design I don't have much to say, except that you better be pretty certain that your public APIs are solid, because I'm sure you will get pushback if you decide you want to start redesigning things in 5.9. I'm still not clear how much this is intended to be a "draft". I'd be fine with that, but if that is the case it should have some sort of big disclaimer that "as a first release, framework APIs are subject to change".

But for the places where you are implementing existing core APIs - they have to be solid and correct. By going into the tree, you become one of the few core implementers of those APIs, and people will end up copying your example, and apps using those APIs will expect them to work in the conventional fashion. So my comments on flaws in the FileHandle, NetworkStack, NetworkInterface and CellularBase implementations must be addressed. I don't think any of those comments are that hard to fix.

Major functional omissions (no mux blocking support, no TCP support) must be explicitly stated.


nsapi_error_t AT_CellularStack::socket_listen(nsapi_socket_t handle, int backlog)
{
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to return an "unsupported" error.

Copy link
Author

Choose a reason for hiding this comment

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


nsapi_error_t AT_CellularStack::socket_accept(void *server, void **socket, SocketAddress *addr)
{
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to return an "unsupported" error.

Copy link
Author

Choose a reason for hiding this comment

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

/**
* Class AT_CellularStack.
*
* Implements NetworkStack and introduces interface for modem specific stack implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should document that the current implementation supports only UDP here.

I see IPv6 code - is it functional?

Copy link
Contributor

Choose a reason for hiding this comment

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

ticket created: IOTCELL-592

@sg- sg- removed their request for review February 19, 2018 01:16
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 19, 2018

Please review travis failure , looks related to this changes


if (success) {
_ip_stack_type = tmp_stack;
_cid = cid;
Copy link
Contributor

Choose a reason for hiding this comment

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

If context created successfully it should be deleted if connect(CGACT for this context) fails.
Internal ticket: IOTCELL-538

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in: 55992a1

_mutex.lock();

const bool writable = (_tx_context.tx_state == TX_IDLE);
const bool readable = (_rx_context.rx_state == RX_SUSPEND);
Copy link
Contributor

@kjbracey kjbracey Feb 20, 2018

Choose a reason for hiding this comment

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

@this looks correct for writable, but I'm not sure about the readable. I think the condition should be _rx_context.is_user_ready - this should be the indication "read will now give you something, not just say -EAGAIN".

Copy link
Contributor

Choose a reason for hiding this comment

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

Using _state.is_user_rx_ready should be also fine but prefer doing it with like this at it maps to design description:
http://trac.oulu.arm.com/trac/nanoservice/wiki/NBIoT/Mux

  • ch Multiplexer frame RX statemachine - BASIC MODE

As thinking now _state.is_user_rx_ready could be removed completely as RX SM should have all required information available => tracked in IOTCELL-584

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not famililar with the structure, but the thing that prompted the comment was the fact the "I'm ready" condition is apparently different to that in the user_data_rx routine that implements read() above. Whichever you prefer, you should align the routines.

@kjbracey
Copy link
Contributor

Great work addressing the majority of my points. Think we're mostly there on the code.

I'm not seeing any JSON yet - whichever settings you're expecting targets/applications to set should be present and documented in one or more mbed_lib.jsons. That would include CELLULAR_DEVICE, and some of the mux settings I've already mentioned.

@AriParkkila
Copy link
Author

Added documentation about cellular with known limitations
https://github.com/AriParkkila/mbed-os/tree/master/features/cellular/README.md

AriParkkila pushed a commit to ARMmbed/easy-connect that referenced this pull request Feb 21, 2018
# Cellular Connectivity

## Provides a new network option in mbed_app.json configuration file:

        "network-interface": {
            "value": "CELLULAR"
        }

## Related PRs

Cellular implementation is coming to Mbed OS (5.8) in ARMmbed/mbed-os#6082
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 21, 2018

Please remove 92a872b commit (rebase instead)

const char* ATHandler::mem_str(const char* dest, size_t dest_len, const char* src, size_t src_len)
{
if (dest_len > src_len) {
for(size_t i = 0; i < dest_len-src_len; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparration stops too early. Fixed in 49cffc2

@jarvte
Copy link
Contributor

jarvte commented Feb 22, 2018

"Please remove 92a872b commit (rebase instead)"
I don't think there is need to rebase if commits are squashed when merging to master.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 22, 2018

"Please remove 92a872b commit (rebase instead)"
I don't think there is need to rebase if commits are squashed when merging to master.

We don't squash by default. And the aim for this PR is to be squashed into one commit? Based on the files changed (over 200), I do not think this is a good idea. To rebase (to clean up) the history, yes that would be nice.

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2018

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

Retrying
/morph test

@AriParkkila
Copy link
Author

Rebased to mbed-os/master commit 'a137444: Merge pull request #6142 from scartmell-arm/feature-deep-sleep-tracing'

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

/morph build

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

@kjbracey-arm You will need to re-approve :)

@kjbracey
Copy link
Contributor

kjbracey commented Mar 2, 2018

Ah, if this was testing merge, then maybe the "test over longer period" fix in 6225 is still insufficient. This time it got a 0.6% error on that clock test, greater than the demanded 0.5%. Don't believe there's any way this PR affects that test.

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

As @AriParkkila rebased the PR we will have to fully retest, fingers crossed it works this next time!

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2018

Build : FAILURE

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

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

Actual failure here http://mbed-os.s3-eu-west-1.amazonaws.com/builds/6082/FAIL/NUMAKER_PFM_M453/ARM/f91cc33fca6e25537db447b6b7aab66b8fcdc17c_build_log_NUMAKER_PFM_M453_ARM.txt

BUILD/tests/NUMAKER_PFM_M453/ARM/TESTS/mbed_drivers/timerevent/main.o ./TESTS/mbed_drivers/timerevent/main.cpp
[DEBUG] Return: 1
[DEBUG] Output: Fatal error: A1023E: File "/tmp/filejCBVGt" could not be opened: No such file or directory
[DEBUG] Output: 1 Error, 0 Warnings

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

Looks possibly like a CI failure rather than an issue ?

@kjbracey
Copy link
Contributor

kjbracey commented Mar 2, 2018

Yep, some sort of server glitch - out of disk space?

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

That's my thinking too!
Will ping @studavekar

@adbridge
Copy link
Contributor

adbridge commented Mar 2, 2018

Seems to be CI issue, re-triggering
/morph build

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2018

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2018

@cmonr
Copy link
Contributor

cmonr commented Mar 2, 2018

Last one.

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Mar 2, 2018

@adbridge adbridge merged commit a6e27b1 into ARMmbed:master Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.