Skip to content

Cellular feature br #4119

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

Merged
merged 29 commits into from
Jun 1, 2017
Merged

Cellular feature br #4119

merged 29 commits into from
Jun 1, 2017

Conversation

hasnainvirk
Copy link
Contributor

Notes:

  • Pull requests will not be accepted until the submitter has agreed to the contributer agreement.
  • This is just a template, so feel free to use/remove the unnecessary things

Description

Adding cellular feature to mbed-os.

Salient features:

  • Introduction of FileHandle
  • Introducing BufferedSerial as a FileHandle and moving it in mbed-os tree
  • Improved AT parser moves in mbed-os tree
  • Introducing mbed-os - lwip glue layer for PPP interface
  • Reference drivers for two different cellular platforms

Status

READY

Migrations

NO

Related PRs

https://github.com/ARMmbed/mbed-os-cliapp/pull/264
https://github.com/ARMmbed/mbed-clitest/pull/769

@hasnainvirk hasnainvirk force-pushed the cellular_feature_br branch 2 times, most recently from b837721 to 96da967 Compare April 5, 2017 15:11
@geky
Copy link
Contributor

geky commented Apr 5, 2017

Steady on the line, we've caught a big one! Thanks for putting this pr up.

cc @sg-, @bulislaw, @sarahmarshy, @kjbracey-arm
possible conflicts #4079 #4087

Also, I'm for bringing in the ATParser (related issue #4040), but this will break most of the external network interfaces that rely on it. Just something to be aware of. This must be on a minor release.

@bulislaw
Copy link
Member

bulislaw commented Apr 5, 2017

Yup, we were discussing it with @kjbracey-arm and I've also sent an email warning about that coming.

@@ -104,7 +104,7 @@ void File::rewind()
return _fs->file_rewind(_file);
}

size_t File::size()
off_t File::size()
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for this tweak

*/
bool send(const char *command, ...)
#if defined(__GNUC__) || defined(__CC_ARM)
__attribute__ ((__format__(__printf__, 2, 3)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be moved out to mbed_toolchain.h?

* @note out-of-band data is only processed during a scanf call
*/
template <typename T, typename M>
void oob(const char *prefix, T *obj, M method) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This callback style should probably be deprecated (issues with cv-correctness)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you suggest an alternative for our particular case ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The definition above takes in a callback:
https://github.com/ARMmbed/mbed-os/pull/4119/files#diff-feeda9e18e6dcf0b25b0c69eae598e78R230

We also provide a function that converts object+member function into a callback:

class Thing { void doit() { printf("hi"); } };
Thing thing;

atparser.oob("AT!", callback(&thing, &Thing::doit));

The reason we do this is to keep the mess that is Callback.h from spreading.

@@ -69,11 +69,15 @@
#endif

// The stack space occupied is mainly dependent on the underling C standard library
#ifdef OS_MAINSTKSIZE
#define WORDS_STACK_SIZE OS_MAINSTKSIZE
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change needed for this pr? I believe there is quite a bit of discussion around the main stack size already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this is needed. Only 32K RAM is actually available for use in UBLOX_C027.
We must reduce main stack size and do all kind of stack reduction hacks. Otherwise we don't fit in.
Even after these drastic measures, we barely fit in.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is 32k for application and 16k*2 on other buses. I think it would be worth allowing all 64k for application and unifying the name of regions such that tools like GCC could do a proper placement but ARM and IAR tools can take advantage of multi-region placement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there is 16K allocated for Ethernet in LWIP's case and 16K for USB. It would have been possible for us to use 16K reserved for USB for BufferedSerial and AT parser but that seemed to be an ugly solution with toolchain related ifdefs and might not have been portable. May be we need to rethink this. @kjbracey-arm

Copy link
Contributor

Choose a reason for hiding this comment

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

Any case where we can do multi-region placement we really, really should. Would save a lot of grief.

Not totally sure whether this "infrastructure" PR should also be including memory tuning stuff though. Unlike the other bits, there's no dependency between them.

* @param events bitmask of poll events we're interested in - POLLIN/POLLOUT etc.
* @return bitmask of poll events that have occurred.
*/
virtual short poll(short events) const {
Copy link
Contributor

@geky geky Apr 5, 2017

Choose a reason for hiding this comment

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

The poll function is growing on me as an extensible approach to handling the state of non-blocking classes, but it is a shift for mbed. I'm thinking this is fine as long as non-blocking behaviour is completely optional. Both implementors and users should be able to ignore these functions.

A few other concerns:

  1. We should move the non-class functions and defines out into a seperate file, thoughts on mbed_poll.h? alternatively they could live in mbed_retarget.h.

  2. Can we use uint16_t here (and why not uint32_t? or maybe even a typedef for poll_t? maybe). This is especially important as a bitfield.

  3. I would describe the purpose of the events parameter a bit better, maybe:

    The input parameter is a bit mask specifying the events the application is interested in. The input parameter can be ignored, but may be used to avoid the cost of determining event state.

  4. There was talk earlier of providing a default implementation of the sigio behaviour with a ticker. Is that possible to accomplish here in the attach function as default behaviour?

  5. Can we drop the _poll_change function? It would seem simpler for implementors to take care of storing the attached callback themselves.

Nits:

  1. Can you make the comments all share the same style, complete with indention and spacing?
  2. Can we provide a default 0xffff for the argument?

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 functions that work on FileHandle and are fundamental to its operation can reasonably be in FileHandle.h. But if/when we have a POSIX API, we would presumably want the real poll() from <poll.h> too. So maybe mbed_poll.h?

As the FileHandle version is inherently C++ maybe this could just be an overload of poll()?

It's short because that's the standard parameter for poll().

I think the comments should be as explicit as possible that this is trying to be POSIX poll(). Not sure if the phrasing was copied from the POSIX spec?

This poll API doesn't quite line up with SIGIO - SIGIO doesn't provide definite answers. poll does. So it's a tougher requirement to implement - still not convinced its possible on all stacks.

Maybe the poll change callback could be turned into just SIGIO - non-definite. So apps could just use the SIGIO and be compatible with limited-functionality network stacks? And maybe apps could be written to tolerate false positives from poll()?

I am nervous about how we integrate this with sockets - at present this assumes reliable poll information, whereas I believe sockets won't always provide that. We potentially end up with a worse set of confusion (cf mbed client PAL, which provides a "select", but doesn't provide reliable answers on all platforms, but apps assume that it does).

The _poll_change is (or will be) required to make top-level poll() work. It covers "release the block in poll" as well as the application callback. (Although the block mechanism is not yet implemented). I'm thinking maybe it shouldn't be a member though.

That member poll() function is potentially confusing, just from a naming point of view. Wondering if it should be called something else...

Copy link
Contributor

@geky geky Apr 6, 2017

Choose a reason for hiding this comment

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

Quick notes:

  • mbed_poll.h seems like the best path forward

  • I'm not sure about mbed::poll vs mbed_poll, so I'll leave that up to you (we do already have mbed::callback).

  • I would still push for uint16_t (or int16_t if you want), it will be the same underlying type as short, just with an explicit width

  • I copied chunks of that description from the linux manual I think. It never hurts to document more than necessary. Related thought: should we just add links to the open group docs on poll/sigio?


My thinking is that the "state fetching" of FileHandle::poll can turn a non-definite sigio into a definite sigio.

FileHandle::poll is a harder requirement but I believe this is necessary for the "definite" functionality users want (poll, event driven). I think at some point it will need to be added to the socket API.

I'm fairly confident you can move _poll_change out of the FileHandle class. Just let the implementors handle the callback. Here's a quick poll based around this idea for sanity:
https://gist.github.com/geky/7eee6937f3d55552d4e365bf02de114f

Copy link
Contributor

Choose a reason for hiding this comment

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

I was actually thinking an overload of ::poll. Modern C++ style is to add overloads in the main namespace for functions to take your types (eg std::swap(mbed::foo &, mbed::foo &)).

People do want that definite poll functionality, but I don't think they're ever going to get it from sockets, as long as we are planning to support off-chip modules, which we definitely are. It would be a trap. Anyone writing code to rely on poll being accurate would be writing fragile unportable code. (Albeit many are managing that with sigio too...)

Given that people haven't been able to figure out the totally standard behaviour of SIGIO, I don't know how we would make them understand non-standard false positives from poll().

Even if we could make poll reliable, I fear it might be incredibly inefficient on some modules. Making a socket call to find out if you should make a socket call doesn't seem brilliant.

I agree that _poll_change shouldn't be a member - (I thought I'd already brought up that thought)

Will ponder your poll.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, continuing to push against int16_t. It's not the same underlying type necessarily, and in POSIX there is no explicit width for short. It's allowed to be bigger than 16 bits. int16_t is not the same type as short.

Yes, in this platform it happens to be, but that's not a reason to change the prototype.

Turning it around - why use off_t and size_t, when we know they're really int32_t and uint32_t? We could make the size explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit on this line: Could we add a default argument?

virtual short poll(short events = 0x7fff) const {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting a default event/events doesn't make sense here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be SHORT_MAX though, technically :)

The member poll was really intended only for the system poll().

On the system poll, I don't think a default would make sense, even if it was possible - it wouldn't be sensible to wake the poll to report a state you don't know about. So there, you always have explicit app-requested bits specified. The parameter was just to pass that required information through as a potential optimisation hint. ("Don't bother filling in these bits if it will cost you time").

We do also have to think about the EINVAL I mentioned - that poll() doesn't allow for that. Would it need a separate "not available" bit?

My feeling is I'd prefer to mark FileHandle::poll() as protected, and a friend of mbed::poll(), to make people use the real API. I don't think there's a valid application use of FileHandle::poll - it's just a non-blocking check of 1 handle, and it's less error-prone (and more portable and efficient) to just attempt non-blocking read/write calls to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just thinking of the default in the context of the FileHandle::poll. Calling poll to get all flags seems like a reasonable default to me. Was just a thought though.

Do you think we should add an extra bit for POLLNAVAIL or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

My feeling is that I don't really think FileHandle::poll should be viewed as public API at all. Putting the default in smells like giving it "easy-to-use-for-the-public" approval. Anyone other than ::poll using it shouldn't be.

I'd rather do something to mark it as "don't use this, use poll()". Make it _poll? protected and a friend of ::poll?

*
* @param blocking true for blocking mode, false for non-blocking mode.
*/
virtual int set_blocking(bool blocking) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should document the return value

Also thoughts on set_timeout?

Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing timeout would be an extra feature. POSIX doesn't provide that - the mechanism is to use the timeout on poll() if necessary. I'd like to keep complexity down, given that we're expecting FileHandle implementers to do it, unlike NSAPI where it's the common code.

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 right, that makes sense

*
* @param func Function to call on state change
*/
int attach(Callback<void(short events)> func);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be named sigio? Also should document the return value.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be sigio, yes. At the minute, it is a reliable callback, unlike sigio, but perhaps it shouldn't be documented that way. That would ease socket integration. In that case remove the parameter too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would push for sigio, if it's not how could you provide a sensible default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by sensible default ? By default it is always ready to read and write.

Copy link
Contributor

@kjbracey kjbracey Apr 7, 2017

Choose a reason for hiding this comment

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

In this form, the default is it's never called. The state never changes. It's explicitly marked as "when the state changes", and the default is to be always readable and writable, and it doesn't support non-blocking.

Note that the default here is "a normal file", and files behave differently from sockets from the point of view of poll.

Copy link
Contributor

@geky geky Apr 7, 2017

Choose a reason for hiding this comment

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

Ah! I didn't think about the state change from that point of view. That makes perfect sense.

I would still vote for sigio to leave the function open ended for implementors. But an empty sigio makes sense to me, thanks for explaining it.


#define MBED_POLLERR 0x1000
#define MBED_POLLHUP 0x2000
#define MBED_POLLNVAL 0x4000
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use the standard names like we've used the errno and open constants?

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 so, yes. I initially assumed no, but having seen what we're doing with errno, it would fit.

Copy link
Contributor Author

@hasnainvirk hasnainvirk Apr 6, 2017

Choose a reason for hiding this comment

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

These are standard names as per posix poll. We added a MBED prefix following mbed conventions.

Copy link
Contributor

Choose a reason for hiding this comment

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

MBED prefixes are for mbed constants. I would just use the POSIX names directly, that would make it clear these aren't just an mbed concept.

#include "ATParser.h"

// Forward declaration
class NetworkStack;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this forward declaration of NetworkStack? The DragonFly interface should inherit from it and implement all of the socket operations, right?

Copy link
Contributor Author

@hasnainvirk hasnainvirk Apr 6, 2017

Choose a reason for hiding this comment

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

This implementation uses PPP to talk to modem and an external IP stack. So you need a forward declaration of NetworkStack. We will use external network stacks's socket operations, e.g., LWIP will provide those. LWIP is a NetworkStack that provides all required socket operations. we could use here any NetworkStack implementation, although currently it is LWIP as Nanostack doesn't have PPP implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you just include NetworkStack.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Including header files prolongs the compilation time. Anyway, the useer of reference drivers do not need NetworkStack, get_stack() is protected and that's why it doesn't make a whole lot of sense to include the header file. Forward declaration is a harmless incomplete type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say "can't you just declare it rather than pull in unneeded headers"?

Anyone actually needing/using the network stack must be getting it from another file - one of the Socket.h, I imagine.

(There's certainly some code messing with interfaces but not using sockets or the network stack - that easy connect thing in the mbed client examples.)

* Application must abort if NSAPI_ERROR_AUTH_ERROR is returned.
* For APN setup, default behaviour is to use 'internet' as APN string and assuming no authentication
* is required, i.e., username and password are no set. Optionally, a database lookup can be requested
* by turning on the APN database lookup feature. In order to do so, add 'MBED_CONF_MTS_DRAGONFLY_APN_LOOKUP'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be added as a parameter to the CellularInterface connect method rather than a config option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems alright to be a config option, as that APN db is not at all exhaustive, It was a customer demand so they can have it if they want to use it. Otherwise, we have our mbed-os API to set proper credentials.

#include "ATParser.h"

// Forward declaration
class NetworkStack;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing as DragonflyInterface. Where are the socket operations defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation uses PPP to talk to modem and an external IP stack. So you need a forward declaration of NetworkStack. We will use external network stacks's socket operations, e.g., LWIP will provide those.

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

Could you address my concerns with your targets.json changes?

@@ -1276,7 +1276,7 @@
"inherits": ["Target"],
"core": "Cortex-M4F",
"supported_toolchains": ["ARM", "uARM", "GCC_ARM", "IAR"],
"extra_labels": ["STM", "STM32F4", "STM32F411RE"],
"extra_labels": ["STM", "STM32F4", "STM32F411RE", "DRAGONFLY_MODEM"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need another TARGET_ directory here? Could you just put it in the TARGET_MTS_DRAGONFLY_F411RE directory?

Copy link
Contributor Author

@hasnainvirk hasnainvirk Apr 6, 2017

Choose a reason for hiding this comment

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

Cellular is a feature. A single Cellular driver serves many different variants. However, a modem can be common in all of those variants. Mostly in mbed environment, the modem is driven by an external MCU. TARGET_UBLOX_C027 or TARGET_MTS_DRAGONFLY_F411RE are sub-directories under NXP and STM families providing board specific initialization. It has nothing to do with the modem specific settings. We have tried to make PinNames uniform across all cellular platforms (will be done for any future porting as well) so that a modem driver can handle any board configuration. We are following here mbed conventions, as its STM family -> a specific STM class of MCUs -> a specific MCU -> attached to a specific modem.
If for example you don't have this structure then, we need to have multiple copies of the same driver in different target defined directories which is a massive maintainance headache.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fine to me

@theotherjimmy Happy with this?

@@ -256,7 +256,7 @@
"supported_form_factors": ["ARDUINO"],
"core": "Cortex-M3",
"supported_toolchains": ["ARM", "uARM", "GCC_ARM", "GCC_CR", "IAR"],
"extra_labels": ["NXP", "LPC176X"],
"extra_labels": ["NXP", "LPC176X", "UBLOX_MODEM"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need another TARGET_ directory here? Could you just put it in the TARGET_UBLOX_C027 directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check the comment below. We need this extra label. We are not voilating any rule here, its exctly like LWIP have its own set of lwip-eth->arch->target fashion.

@sarahmarshy
Copy link
Contributor

Additionally, is there any way this DragonflyInterface can be merged with the existing driver? https://github.com/ARMmbed/mtsas-driver

@theotherjimmy
Copy link
Contributor

I think this PR has many unrelated changes. Could these be broken up into multiple PRs?

}
// TODO - proper blocking
// wait for condition variable, wait queue whatever here
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.

This can be replaced by a semaphore wait. Here's how this polling behaviour is implemented in the network classes:
https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TCPSocket.cpp#L65-L69

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe semaphores cover the complexity of the multiple input setup here - this is significantly tougher than the 1-to-1 driver->waiter setup of the socket wake-up.

My intent is to use this #3648

Using condition variables as a wait queue should be the most memory-efficient way of doing it.

I was hoping to have that functionality in place for initial merge, but we may not have time.

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 any problems with the semaphore approach besides spurious wakeups?

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 sure it could be made to work, it's just hard-to-understand and potentially error-prone. (I recall the debugging of the simpler NSAPI case) Semaphores are the wrong tool for the job, and I get grumpy every time I'm forced to figure out how to make them do what I want because the correct tool isn't available.

The correct tool is waiting over there in #3648, and I'd rather spend time getting that in than going through the Semaphore fiddling again. If there's no time for #3648, there's no time for the semaphores and I'm okay letting it spin+yield for now - that's better than any existing serial driver does, and it won't happen significantly in the PPP data pump, just in the AT connect sequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note to future self when I forget why we don't just use semaphores: Consider multiple consuming threads.

@kjbracey-arm made a good point in a different thread:

This won't work if two threads use poll at once. You need to wake every waiting thread, so call the semaphore at least N times where N is the number of threads waiting on poll. Even then, that wouldn't work reliably - you might wake one thread twice. I'm not going to dig any further on that point, as this is exactly the hole digging I was talking about not wanting to do in my PR comment.

If you keep going I think eventually you'll reconstruct a something which is equivalent to a wait queue or condition variable, so I think it'll be faster and easier to start immediately with Russ's condition variable implementation.

The problem makes sense to me now, thanks for explaining this. 👍

Makes sense to move forward with spin+yield (or wait) for now. Once the api is in we have our foot in the door to optimize later.

errno = ENOENT;
return -1;
} else if (path.isFile()) {
goto fail;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the fail block could be wrapped up in a seperate function and reused in more places?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I see that.

@@ -77,71 +77,60 @@ extern "C" {
* Note also that ARMCC errno.h defines some symbol values differently from
* the GCC_ARM/IAR/standard POSIX definitions. The definitions guard against
* this and future changes by changing the symbol definition as shown below. */
#ifdef ENOENT
#undef ENOENT
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this error now if ENOENT is not defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope. #undef is a no-op if it's not defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool

@kjbracey
Copy link
Contributor

kjbracey commented Apr 6, 2017

I think there are a few changes that have crept in that shouldn't be there, but there's a dependent tree of five changes:

The two modem drivers each need the Network stack PPP support and the ATParser and the cellular API.
The network PPP support and the ATParser both need the FileHandle work.

A proper review tool like Gerrit would let a chain like that be approved and reviewed piecemeal, and merged in in stages. Not sure what you can do in GitHub, apart from only open each PR after the last one is merged.

@kjbracey
Copy link
Contributor

kjbracey commented Apr 6, 2017

I'd want to see the nsapi_ppp.h API here aligned with the mbed_ipstack.h from #4079. Seems like they should be part of the same API. Maybe rework in the next stage - integrating now might cause more merge grief rather than less.

@hasnainvirk hasnainvirk force-pushed the cellular_feature_br branch 4 times, most recently from d9847f9 to b80fc6b Compare April 6, 2017 13:27
@@ -23,6 +23,8 @@
* Common interface that is shared between Cellular interfaces
Copy link

Choose a reason for hiding this comment

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

What is the difference between mbed-os/features/unsupported/net/cellular/CellularModem/CellularModem.h and this interface? I see couple of APIs like connect/disconnect that are common. Why is this new interface missing ATCommandsInterface or SMS APIs?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be functionally the same but based on UART rather than USB. I'd expect unsupported version to be deleted once this lands

@hasnainvirk
Copy link
Contributor Author

@bulislaw Should we actually try to deprecate nsapi_ppp_api introduced in this PR and slo tin our stuff mbed_ipstack api ? It seems more logical to tie them togather. @kjbracey-arm Can you please guide us here ? What should be the most user friendly and logical way forward ?

@bulislaw
Copy link
Member

bulislaw commented Apr 7, 2017

@hasnainvirk Yes I would say we should use one coherent solution, it can be my mbed_ipstack, we may modify it if needed. Otherwise we will keep doing the fragmentation, and keep sticking things to the side of what's there with bunch of ducttape, and collecting technical debt and will never reach convergence in networking APIs.

@kjbracey
Copy link
Contributor

kjbracey commented Apr 7, 2017

How to manage this practically? One PR is going to end up dependent on the other. Is either of these close to merging, so someone can start the conflict resolution work?

I believe our PR is possibly lower-impact, as we're only extending APIs, not changing any, whereas I believe the other one still requires a lot of work updating platforms due to API changes? Is ours more likely to go first?

@bulislaw
Copy link
Member

bulislaw commented Apr 7, 2017

I would say so. Can we say that your PR goes in when it's ready. And then @hasnainvirk or someone else from the team work on our branch to make the APIs match (and rebase)? So when we are ready with all the Ethernet changes it's all there.

@Nodraak Nodraak mentioned this pull request Jul 12, 2017
kjbracey added a commit to kjbracey/mbed-os that referenced this pull request Jan 10, 2018
This has been superceded by CellularBase. Name change occurred late
in review of ARMmbed#4119 and
original unused CellularInterface was left behind.
kjbracey added a commit to kjbracey/mbed-os that referenced this pull request Feb 1, 2018
This has been superceded by CellularBase. Name change occurred late
in review of ARMmbed#4119 and
original unused CellularInterface was left behind.
0xc0170 pushed a commit that referenced this pull request Feb 5, 2018
This has been superceded by CellularBase. Name change occurred late
in review of #4119 and
original unused CellularInterface was left behind.
kjbracey added a commit that referenced this pull request Feb 12, 2018
This has been superceded by CellularBase. Name change occurred late
in review of #4119 and
original unused CellularInterface was left behind.
kjbracey added a commit that referenced this pull request Apr 18, 2018
This has been superceded by CellularBase. Name change occurred late
in review of #4119 and
original unused CellularInterface was left behind.
kjbracey added a commit to kjbracey/mbed-os that referenced this pull request Apr 24, 2018
This has been superceded by CellularBase. Name change occurred late
in review of ARMmbed#4119 and
original unused CellularInterface was left behind.
kjbracey added a commit to kjbracey/mbed-os that referenced this pull request May 8, 2018
This has been superceded by CellularBase. Name change occurred late
in review of ARMmbed#4119 and
original unused CellularInterface was left behind.
kjbracey added a commit that referenced this pull request May 22, 2018
This has been superceded by CellularBase. Name change occurred late
in review of #4119 and
original unused CellularInterface was left behind.
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.