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

Deprecated warnings for feature/netsocket/cellular #6264

Merged
merged 5 commits into from Mar 16, 2018

Conversation

Projects
None yet
6 participants
@jarvte
Contributor

jarvte commented Mar 5, 2018

Description

Deprecated warnings for feature/netsocket/cellular. Moved APN_db.h under features/cellular/easy_cellular/.

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change

@0xc0170 0xc0170 requested a review from kjbracey-arm Mar 5, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 5, 2018

We have MBED_DEPRECATED , this will come later (when?) ?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 5, 2018

So you're not deprecating it yet, this is just a pre-warning? Not sure how useful that is - if you have the replacement ready, deprecate it now. If you haven't, not much point with the prewarning, although maybe you could point at a feature branch or something. Is the intent that the actual deprecation happens in 5.9, or in a 5.8.y patch?

I think you need to be more specific on what you're deprecating in favour of, rather than just pointing at a directory. Eg if writing a driver derived from PPPCellularInterface or UARTCellularInterface, they should be deriving from AT_CellularDevice etc instead.

If as an app they were using them directly as "generic" they should doing what? There's no direct equivalent - something providing "CellularBase" on an arbitrary FileHandle or set of pins. Does AT_CellularDevice work directly without inheritance - would that give a generic base driver with new API? Could EasyCellularConnection have extra constructors taking specific pins or a FileHandle, so it could be a direct substitute?

For OnboardModemInterface as an app, I guess the replacement is yet to come - it will be CellularBase::get_default_instance() or NetworkInterface::get_default_Instance() in 5.9 if #6108 happens.

Why move the APN database? It's a general utility - if it's not provided by the main framework and you're expecting apps to do APN lookup themselves if not using the easy wrapper, which I understood to be the case, it should remain in a utility position so they can get at it. The easy wrapper and PPPCellularInterface are a couple of potential users, not the only ones.

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Mar 5, 2018

Hi @kjbracey-arm, we cannot decide when this can be deprecated; It is Mbed Os decision.
We want to put this text here for guiding developers not to use these files anymore as the support for those have ended. As I have understood deprecation might happen when Mbed OS 6.0 is introduced, but it might as well not happen.

I agree that commenting should be more specific for developer to easier find a replacement.

IMHO APN database should not be next to deprecated classes because developers might think it will be deprecated as well.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 5, 2018

Deprecation marker would presumably go into 5.x.y; 6.0 is when stuff marked as deprecated in 5.x.y could actually be deleted.

I would suggest putting actual real MBED_DEPRECATED markers into master ready for release in 5.9 - gives us time to adjust anything to have concrete answers for what the direct replacement is (and by which time we know any kinks in this first 5.8 iteration will have been worked out).

If that's planned, these pre-warnings might be worthwhile to do first for a patch release.

If you want the APN database next to the new code, then features/cellular/utils or features/cellular/framework/utils would be fine.

@jarvte jarvte force-pushed the jarvte:master branch from 1f1d6e2 to 29f6a32 Mar 5, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 8, 2018

Looks like a bad rebase , as some commits here are duplicates? please review

@jarvte jarvte force-pushed the jarvte:master branch 2 times, most recently from b143f78 to e073e09 Mar 8, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 12, 2018

I would suggest putting actual real MBED_DEPRECATED markers into master ready for release in 5.9 - gives us time to adjust anything to have concrete answers for what the direct replacement is (and by which time we know any kinks in this first 5.8 iteration will have been worked out).

If that's planned, these pre-warnings might be worthwhile to do first for a patch release.

@AnttiKauppila Is this correct, MBED_DEPRECATED intentionally not used here, will come later?

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Mar 12, 2018

We have agreed to use MBED_DEPRECATED. We will update this

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Mar 12, 2018

Actually used MBED_DEPRECATED_SINCE as we plan to drop support in Mbed-OS 5.9.

@jarvte jarvte force-pushed the jarvte:master branch from 6f697fc to 16e8b77 Mar 13, 2018

@jarvte

This comment has been minimized.

Contributor

jarvte commented Mar 13, 2018

ARM internal ref: IOTCELL-544

@0xc0170 0xc0170 requested a review from AnttiKauppila Mar 13, 2018

@cmonr cmonr referenced this pull request Mar 13, 2018

Merged

Cellular: update attach test #6350

1 of 5 tasks complete

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Mar 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 15, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 15, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 15, 2018

Will relaunch when able to. ARM license CI issue.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 15, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 15, 2018

Build : SUCCESS

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

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 16, 2018

CI issue: ARM network licenses could not be checked out during build.

Restarting.
/morph export-build

@mbed-ci

This comment has been minimized.

@cmonr

cmonr approved these changes Mar 16, 2018

@cmonr cmonr merged commit 7c30faf into ARMmbed:master Mar 16, 2018

19 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 Local events testing has passed
Details
travis-ci/littlefs Passed, code size is 10060B (+0.00%)
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment