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

Renaming Ublox library for mbed cellular framework #6402

Merged
merged 3 commits into from Apr 6, 2018

Conversation

@mudassar-ublox
Contributor

mudassar-ublox commented Mar 20, 2018

Description

Renaming UBLOX target library form "UBLOX_LISA_U" to "UBLOX_PPP" for mbed cellular framework.

LISA is not a cellular technology, it is the physical shape of module. Technology used for cellular test is PPP, So changing name from LISA_U to PPP as suggested by @kjbracey-arm (Kevin Bracey).

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 20, 2018

Fine with the new name, but I think you should probably retain a typedef or simple inheritance with deprecation tag for the LISA_U name, as that's what's gone out for 5.8. This would be a breaking change for an app that was manually requesting that driver.

@cmonr cmonr added needs: work and removed needs: review labels Mar 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 21, 2018

Agree with @kjbracey-arm on inheritance with current device name.

@mudassar-ublox If you like, you could add a deprecation warning to the old name targeting 5.9.

@bqam-ublox bqam-ublox force-pushed the u-blox:cellular_test_target branch from 1e673c8 to eb806b6 Mar 26, 2018

@bqam-ublox

This comment has been minimized.

Contributor

bqam-ublox commented Mar 26, 2018

@cmonr deprecated warnings have been added

@0xc0170 0xc0170 added needs: review and removed needs: work labels Mar 26, 2018

@@ -35,6 +35,11 @@ class UBLOX_LISA_U_CellularPower : public AT_CellularPower
virtual nsapi_error_t off();
};
class UBLOX_LISA_U_CellularPower : public UBLOX_PPP_CellularPower
{
MBED_DEPRECATED_SINCE("mbed-os-5.9", "This API will be deprecated, Use UBLOX_PPP_CellularPower instead of UBLOX_LISA_U_CellularPower.");

This comment has been minimized.

@geky

geky Mar 26, 2018

Member

Hello, this isn't the correct way to mark a class as deprecated.

The deprecated attribute needs to be before the class:

MBED_DEPRECATED_SINCE("mbed-os-5.9", "This API will be deprecated, Use UBLOX_PPP_CellularPower instead of UBLOX_LISA_U_CellularPower.")
class UBLOX_LISA_U_CellularPower  : public UBLOX_PPP_CellularPower
{
   blablabla

Otherwise this won't compile on GCC or ARM.
https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_toolchain.h#L297

This comment has been minimized.

@bqam-ublox

bqam-ublox Mar 27, 2018

Contributor

Actually, I put a semi colon at the end of the deprecated warning and it compiled successfully.

Thanks for the correction. I have modified the code now.

This comment has been minimized.

@geky

geky Mar 27, 2018

Member

Oh that's interesting. Although the deprecated message still won't show up.

Thanks for fixing this 👍

@0xc0170 0xc0170 added needs: work and removed needs: review labels Mar 26, 2018

@cmonr

cmonr approved these changes Mar 27, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 27, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 27, 2018

ARM license network issue.

Will rebuild once CI gap is available.

@cmonr cmonr added needs: CI and removed needs: work labels Mar 27, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 28, 2018

/morph export-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 28, 2018

/morph mbed2-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 28, 2018

@0xc0170 Might need you to restart these. Looks like GitHub is ignoring me again..

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 28, 2018

/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 28, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 29, 2018

Still holding for reviews.

@cmonr cmonr added needs: review and removed needs: CI labels Mar 29, 2018

@cmonr cmonr requested a review from AriParkkila Mar 29, 2018

@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented Mar 29, 2018

This is a valid change, but still UBLOX_PPP is not a very good name either...

  1. u-blox cellular modules support both PPP mode and internal modem IP stack
  2. PPP mode is typically configured with "lwip.ppp-enabled" in mbed_app.json

I want to raise another relating concern here...

Ublox-C027 (and ublox-C030) may have different cellular modules like LISA-C200, LISA-U200 or SARA-G350. From AT command point of view u-blox cellular module variation comes from type codes C (CDMA), U (UMTS) and G (GPRS) and associated platform version such as U2 or G3, so I think good naming could be something like "UBLOX_U2".

Mbed OS detects at compile-time only board target ublox-C027 (mbedls target_id) and it does not know which cellular module type is actually mounted on the board.

The problem is that we can't detect automatically at compile-time which cellular device type is mounted on the board. Is there any?

@bqam-ublox

This comment has been minimized.

Contributor

bqam-ublox commented Mar 29, 2018

@RobMeades please comment

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Mar 29, 2018

@AriParkkila: the plan is that u-blox will maintain a PPP-mode driver, which will work with all our of modems that support PPP mode, and one or more AT-mode driver, which will support these modems and others which have no PPP interface. The first driver will be UBLOX_PPP. the others will be UBLOX_ATxxx, where xxx represents any necessary variations since, in those cases, we know that there are ranges of AT commands that do not fall within 3GPP 27.007. Should there be any need for variations in AT command behaviour for different modems within the single PPP set (unlikely given the very small number of AT commands we are concerned with here), we will detect and deal with that at run-time, by querying the module name, as we do today. For instance, U2 and G3 are identical in all of the respects were are concerned with here and so shouldn't need separate handling.

Is there a reason why the above would not work?

@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented Mar 29, 2018

If cellular module variation can be handled at runtime then this is good.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 4, 2018

@AriParkkila @AnttiKauppila Can you approve/request changes please?

@0xc0170

0xc0170 approved these changes Apr 5, 2018

@0xc0170 0xc0170 merged commit f331ac3 into ARMmbed:master Apr 6, 2018

11 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10525 cycles
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Apr 6, 2018

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