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

Make SMS configurable in Cellular stack #11873

Merged
merged 2 commits into from
Nov 27, 2019

Conversation

AnttiKauppila
Copy link

@AnttiKauppila AnttiKauppila commented Nov 15, 2019

Description

Put SMS behind configuration flag to save some memory (4,5kB) for Cellular users not needing SMS features.
UBlox N2XX target is also updated

Summary of change (What the change is for and why)

Unittests have been updated also

Documentation (Details of any document updates required)

Pull request type (required)

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers (optional)

@AriParkkila @mirelachirica @JanneKiiskila @anttiylitokola

Release Notes (required for feature/major PRs)

Summary of changes

Cellular stack has now configurable SMS support. This is added to Cellular mbed_lib.json. It is enabled by default for backward compatibility.

Impact of changes

If one does not need SMS, disable it in mbed_app.json to decrease the application footprint.

Migration actions required

None

@ciarmcom
Copy link
Member

@AnttiKauppila, thank you for your changes.
@AriParkkila @mirelachirica @JanneKiiskila @anttiylitokola @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

@@ -314,6 +314,7 @@ class CellularDevice {
*/
virtual CellularNetwork *open_network(FileHandle *fh = NULL) = 0;

#if MBED_CONF_CELLULAR_USE_SMS || defined(DOXYGEN_ONLY)
/** Create new CellularSMS interface.
*
* @param fh file handle used in communication to modem. This can be, for example, UART handle. If null, then the default
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the trick how to enable this via the mbed_app.json target override be added here or?

Copy link
Contributor

@JanneKiiskila JanneKiiskila left a comment

Choose a reason for hiding this comment

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

Activation tip somewhere, looks otherwise just fine!

@AnttiKauppila AnttiKauppila added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Nov 18, 2019
@AnttiKauppila
Copy link
Author

Activation tip somewhere, looks otherwise just fine!

Like I have put in mbed_lib.json?

@JanneKiiskila
Copy link
Contributor

Activation tip somewhere, looks otherwise just fine!

Like I have put in mbed_lib.json?

Well, that's not quite enough - we don't want people to modify the mbed_lib.json files, they should understand how to override that in their own mbed_app.json instead, using the target overrides. But, I think that should probably be in the cellular API docs instead - the example "add these lines to your mbed_app.json to get SMS API enabled..." type of thing.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 19, 2019

Disable SMS by default from Cellular stack

Please amend the commit msg with details shared above (why is this disabled).

@AnttiKauppila
Copy link
Author

Activation tip somewhere, looks otherwise just fine!

Like I have put in mbed_lib.json?

Well, that's not quite enough - we don't want people to modify the mbed_lib.json files, they should understand how to override that in their own mbed_app.json instead, using the target overrides. But, I think that should probably be in the cellular API docs instead - the example "add these lines to your mbed_app.json to get SMS API enabled..." type of thing.

But Janne, this is the DEFAULT Mbed OS configuration mechanism, nobody should modify mbed_lib.json anywhere but override the config as per our documentation: https://os.mbed.com/docs/mbed-os/v5.14/reference/configuration.html
And you can list cellular specific configurations like this:
mbed compile --config --prefix cellular

What else do you need?

Put SMS behind configuration flag to save some memory (4,5kB) for Cellular users not needing SMS features.
UBlox N2XX target is also updated
@AnttiKauppila
Copy link
Author

@0xc0170 Commit msg updated

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 25, 2019

will start CI as soon as some 5.15 jobs complete

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 25, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 25, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@AnttiKauppila AnttiKauppila changed the title Disable SMS by default from Cellular stack Make SMS configurable in Cellular stack Nov 26, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 26, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 removed the needs: CI label Nov 27, 2019
@0xc0170 0xc0170 merged commit 58d6f5f into ARMmbed:master Nov 27, 2019
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.

6 participants