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

Platform support for new CellularInterface in UBLOX_C027 and UBLOX_C030_U201. #4337

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
@RobMeades
Contributor

RobMeades commented May 17, 2017

This change implements the platform support which allows both the mbed-os OnboardModem and the u-blox PPP and AT data drivers to be used with both the u-blox C027 and u-blox C030 platforms.

@0xc0170 0xc0170 requested a review from hasnainvirk May 18, 2017

@amq

This comment has been minimized.

Contributor

amq commented May 18, 2017

Do you plan to add support for CellLocate?

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 18, 2017

Yup, though that doesn't need to go into mbed-os, there will be a separate Mercurial library providing support for HTTP as well as CellLocate.

@RobMeades RobMeades referenced this pull request May 18, 2017

Merged

Cellular feature br #4119

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 18, 2017

Could you change the title to remove the [ ]? We use the titles of PRs in the release notes and they get rendered with markdown, so the brackets will interfere.

@RobMeades RobMeades changed the title from [UBLOX_C027, UBLOX_C030_U201] Implementation of the new CellularInterface. to UBLOX_C027, UBLOX_C030_U201: implementation of the new CellularInterface. May 18, 2017

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 18, 2017

@theotherjimmy : done.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 18, 2017

Thanks! Looks great.

@amq

This comment has been minimized.

Contributor

amq commented May 18, 2017

Do I understand it correctly?

  • CellLocate will require the AT_DATA flavour, unless disconnects are acceptable
  • the AT_DATA flavour fully supports mbed-client, at least with UDP
  • mbed-client with the AT_DATA flavour will (or at least can) rely on mbed-tls for DTLS
@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 18, 2017

CellLocate will require the AT_DATA flavour

CellLocate is entirely AT based, so it doesn't actually require anything at all, other than a base class which instantiates the AT parser. EDIT: my mistake, checking again it does require an "AT DATA" -style connection to have been established previously so yes, it does require the AT DATA flavour.

the AT_DATA flavour fully supports mbed-client, at least with UDP

It presents a sockets interface, both UDP and TCP, exactly as LWIP does.

mbed-client with the AT_DATA flavour will (or at least can) rely on mbed-tls for DTLS

Not tested yet but, provided DTLS makes no calls around the sides of the absolutely standard sockets interface, it should work in exactly the same way as it does with LWIP. You'll be somewhat challenged for RAM on C027 of course.

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 18, 2017

I've update this pull request to align with the latest changes in #4119. The submits are now all a bit of a mess, please advise how you would like me to proceed.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 19, 2017

@RobMeades Can you please rebase again ? There was a PR merged ahead of #4119 that introduced changes in lwipopts.h. #4119 is now updated.

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 19, 2017

Done. Test results, in case anyone needs them:

+-------------------------+-----------------+---------------------------------------------------------------------------------------+-------------------------------------------------+--------+--------+--------+--------------------+
| target                  | platform_name   | test suite                                                                            | test case                                       | passed | failed | result | elapsed_time (sec) |
+-------------------------+-----------------+---------------------------------------------------------------------------------------+-------------------------------------------------+--------+--------+--------+--------------------+
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic-tests-unit_tests-default         | Connect using local instance, must be last test | 1      | 0      | OK     | 59.97              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic-tests-unit_tests-default         | Connect with credentials                        | 1      | 0      | OK     | 20.76              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic-tests-unit_tests-default         | Connect with preset credentials                 | 1      | 0      | OK     | 35.68              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic-tests-unit_tests-default         | Set randomise                                   | 1      | 0      | OK     | 33.3               |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic-tests-unit_tests-default         | TCP async echo test                             | 1      | 0      | OK     | 43.41              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic-tests-unit_tests-default         | UDP echo test                                   | 1      | 0      | OK     | 39.77              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic_at_data-tests-unit_tests-default | Connect using local instance, must be last test | 1      | 0      | OK     | 55.11              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic_at_data-tests-unit_tests-default | Connect with credentials                        | 1      | 0      | OK     | 42.05              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic_at_data-tests-unit_tests-default | Connect with preset credentials                 | 1      | 0      | OK     | 33.75              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic_at_data-tests-unit_tests-default | Set randomise                                   | 1      | 0      | OK     | 32.45              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic_at_data-tests-unit_tests-default | TCP async echo test                             | 1      | 0      | OK     | 86.91              |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic_at_data-tests-unit_tests-default | UDP async echo test                             | 1      | 0      | OK     | 75.1               |
| UBLOX_C030_U201-GCC_ARM | UBLOX_C030_U201 | mbed-os-features-cellular-target_ublox_modem_generic_at_data-tests-unit_tests-default | UDP echo test                                   | 1      | 0      | OK     | 53.37              |
+-------------------------+-----------------+---------------------------------------------------------------------------------------+-------------------------------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 13 OK
mbedgt: completed in 637.70 sec
@amq

This comment has been minimized.

Contributor

amq commented May 22, 2017

Current mbed-client doesn't seem to work with the AT_DATA flavour

For some reason, this

palStatus_t pal_plat_getAddressInfo(const char *url, palSocketAddress_t *address, palSocketLength_t* length)
{
    palStatus_t result = PAL_SUCCESS;

    SocketAddress translatedAddress; // by default use the fist supported net interface - TODO: do we need to select a different interface?
    result = s_pal_networkInterfacesSupported[0]->gethostbyname(url, &translatedAddress);
    if (result == 0)
    {
        result = socketAddressToPalSockAddr(translatedAddress, address, length);
    }
    else // error happened
    {
        result = translateErrorToPALError(result);
    }
    return result; 
}

in pal_plat_network.cpp

ends up here

nsapi_error_t UbloxCellularDriverGenAtData::socket_listen(nsapi_socket_t handle,
                                                          int backlog)
{
    return NSAPI_ERROR_UNSUPPORTED;
}

while the PPP flavour ends up here (as expected)

nsapi_error_t NetworkInterface::gethostbyname(const char *name, SocketAddress *address, nsapi_version_t version)
{
    return get_stack()->gethostbyname(name, address, version);
}
@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 22, 2017

Interesting: I thought socket_listen() was purely for TCP Server implementations. Is there any reason why mbed client needs to be a TCP Server?

@amq

This comment has been minimized.

Contributor

amq commented May 22, 2017

What's also interesting, with PPP, socket_listen() doesn't actually get called

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 22, 2017

@RobMeades Yes that is right, socket_listen() is purely used for TCP server type sockets. I can't think of any reason that gethostname() would fallback to socket_listen() somehow. That would be absurd. I have not seen your code yet, but I'm sure your resolving DNS using AT command and hence providing nsapi_error_t NetworkInterface::gethostbyname(blah, blah). If not, then there is problem here.

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 22, 2017

Yes, gethostbyname() translates to AT+UDNSRN=0,"blah.com" and it's definitely working here. Strange. @amq, can you point me at a build I can try?

@hasnainvirk

This comment has been minimized.

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 22, 2017

I can implement xxxsockopt() if that is a requirement.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 22, 2017

@RobMeades I don't really know if this is a requirement. I don't have PAL code base to check the usage. @JanneKiiskila Do you have any idea where client cloud would use PAL layer abstraction of setsockopt() getsockopt() ?

@amq

This comment has been minimized.

Contributor

amq commented May 22, 2017

@RobMeades here is the test code: https://github.com/amq/mbed-os-example-client

Compiles for UBLOX_C027 and UBLOX_C030_U201 (with a separate mbed_app.json)

mbed update
mbed compile -t GCC_ARM -m UBLOX_C027

For a full functionality, you'd need to get the security.h contents from https://connector.mbed.com/

@amq

This comment has been minimized.

Contributor

amq commented May 22, 2017

Looks like it's not setsockopt() or getsockopt(), at least breakpoints don't trigger there.

Here is a call trace:
socket_listen_trace

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented May 22, 2017

@avolinski can give a hand with PAL issues.

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 22, 2017

I've got @amq's code running now but I have a few questions that it's probably best not to mess up this thread with. @amq: how can I contact you?

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented May 23, 2017

Update: looks like this is simply down to ordering of the inherited classes in UbloxCellularDriverGenAtData.h. PAL casts the cellular driver to NetworkInterface and so CellularInterface, which is a single inheritor of NetworkInterface, has to be first in the list. I will refresh the pull request once the dust has settled on all the discussions over in #4119.

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented May 25, 2017

@RobMeades Thanks for the PR.

Also, we recommend our contributors follow Chris Beam’s seven rules of great commit messages to keep the commit history clear. We find the commit.template feature particularly helpful.

When you refresh this PR, could you please match this format by shortening the subject line and changing it to the imperative mood? For example, you could change it to "Implement new CellularInterface in UBLOX_C027, UBLOX_C030_U201"

Thanks for your contributions.

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 30, 2017

@RobMeades What is the status of this ?
Also looks like it is failing Looks Oulu CI .
@SeppoTakalo ? @marhil01 ? With a weird job not found error ?

HTTP ERROR 404

Problem accessing /job/ARMmbed/job/mbed-os-cliapp/job/master/4042/. Reason:

    Not Found

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 15, 2017

Hmm, actually maybe not, trying to include mbed.h down in the targets directory doesn't seem to be so straightforward, it complains.

Compile [100.0%]: onboard_modem_api.c
[Fatal Error] Callback.h@21,15: new: No such file or directory
[ERROR] In file included from ./mbed-os/rtos/Thread.h:30:0,
                 from ./mbed-os/rtos/rtos.h:31,
                 from ./mbed-os/mbed.h:20,
                 from .\mbed-os\targets\TARGET_NXP\TARGET_LPC176X\TARGET_UBLOX_C027\onboard_modem_api.c:19:
./mbed-os/platform/Callback.h:21:15: fatal error: new: No such file or directory
 #include <new>
@sg-

This comment has been minimized.

Member

sg- commented Jun 15, 2017

The point is the Ublox cellular library should have these, not a .c hal driver mbed.h in the hal is very bad idea. Where is the cellular-library, I could make comments there

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 15, 2017

I was following the pattern established for C027 in #4119 (onboard_modem_api.c came in with that PR). That way the C030 board can be used with the OnboardCellularInterface without any modifications or external code whatsoever. Surely that's the right thing to do, not require the user to go and find some external code simply to switch the modem on!?

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 15, 2017

The other thing to bare in mind is that both C027 and C030 are cellular boards. Without cellular they have no meaning. Having their "on" switch inside their target code in mbed-os is somewhat fundamental to the concept of a cellular board.

FYI, all the example code and libraries are in the u-blox team depot here.

@sg-

This comment has been minimized.

Member

sg- commented Jun 15, 2017

onboard_modem_api.c came in with that PR

Right but this isn't necessarily right. Boards compose with libraries. The APIs for IO support static initialization for all needed modes. This is where gpio_ex sutff came from in 2013. The cellular library should be able to handle the IO for the modem and the PPP communication. I dont see why its needed in 2 places.

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 15, 2017

It is in one place: with the target CELLULAR board where it makes sense, the on-switch API for the board. That way it can be used by OnboardCellularInterface, UbloxPPPCellularInterface and UbloxATCellularInterface. Everybody wins.

As you can probably tell, I'm kinda losing my patience with this now. Please tell me how you would like the cellular on/off-switch implemented, provided that:

  1. It works with all three libraries.
  2. It stays with the board. It is fundamental to the board, it is not external/peripheral/optional in any way.
@sg-

This comment has been minimized.

Member

sg- commented Jun 15, 2017

How does a K64F use the Ublox cellular library? Can you share a link to where this work is?

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 15, 2017

I believe you're talking about the cellular shield from Embedded Artists. That re-uses the C027_Support library. When compiled for C027 and with an on-board modem, mdm.cpp calls what used to be C027_api.c in mbed-os/targets/TARGET_NXP/TARGET_LPC176X/TARGET_UBLOX_C027, which recently became ublox_low_level_api.c and was then wrapped by onboard_modem_api.c since powering on is not as simple as setting a GPIO statically. So for this case the power on/off code was with the mbed target C027 board, Embedded Artists just took a ride on it.

@sg-

This comment has been minimized.

Member

sg- commented Jun 15, 2017

Right, but what if I have a K64F not a C027?

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 15, 2017

That's entirely up to them, the u-blox code does not provide a way to power up and down their shield, it only calls the hooks into the on/off functions for the C027 board in mbed; see around line 2285 here.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jun 16, 2017

@sg- I will try to clear the air here once again. If you have a K64F board and an Embedded Artist shield, you will inherit either from UARTSerialInterface or PPPCellularInterface depending upon your needs and you will override modem_init(), modem_power_up(), modem_power_down(), modem_deinit() APIs. That's the model which should be followed. It is clearly documented in the API docs.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Jun 16, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 17, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 17, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 580

All builds and test passed!

@sg-

This comment has been minimized.

Member

sg- commented Jun 18, 2017

That's entirely up to them, the u-blox code does not provide a way to power up and down their shield, it only calls the hooks into the on/off functions for the C027 board in mbed

It seems the case 4 is the model that's best fitted (need some wordsmiths here). https://github.com/ARMmbed/mbed_OS_API_Docs/blob/5.5/docs/APIs/communication/cellular.md#providing-module-modem-api

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jun 19, 2017

@sg- Yes, absolutely spot on. If they have a modem on a shield, and target it let's say K64F, Case 4 applies plus Case 1 or 2 depending upon their use of AT only or PPP mode.

@0xc0170 0xc0170 removed the needs: CI label Jun 19, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 19, 2017

@sg- Are you happy with this change set?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2017

The PR looks good to me.

@adbridge adbridge merged commit 810e16f into ARMmbed:master Jun 20, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sg- sg- removed the ready for merge label Jun 20, 2017

@RobMeades RobMeades deleted the u-blox:cellular_feature_br_ublox_pr1 branch Jul 3, 2017

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