Cellular: Telit ME910 driver #10484

trowbridgec · 2019-04-25T18:53:15Z

Description

Add support for the Telit ME910 CAT-M1 cellular module.

See #8666 for more history on this issue.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change
[x] Enhancement

Reviewers

@hasnainvirk @AriParkkila @maclobdell @ARMmbed/mbed-os-wan

Release Notes

trowbridgec · 2019-04-25T18:56:45Z

@loverdeg-ep

ciarmcom · 2019-04-25T19:00:35Z

@trowbridgec, thank you for your changes.
@AriParkkila @maclobdell @hasnainvirk @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910.h

AriParkkila · 2019-04-26T07:27:37Z

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910.h

+protected: // AT_CellularDevice
+    virtual uint16_t get_send_delay() const;
+    virtual nsapi_error_t init();
+    virtual nsapi_error_t hard_power_on();


Need to be implemented if defined, or if don't have the default power implementation the declaration of power-functions should be removed.

@AriParkkila I added the default power implementation for the ME910. I was toying around with this since we have a separate, board-specific power enable pin that I had to accommodate, but that functionality doesn't make sense here (should be in our application/BSP code), so now I just have the necessary power on sequences as suggested in the ME910 manual.

@AriParkkila I see that a couple of the CI jobs are failing because my power implementation references the MDMPWRON pin:

[Error] TELIT_ME910.cpp@95,29: 'MDMPWRON' was not declared in this scope [Error] TELIT_ME910.cpp@109,29: 'MDMPWRON' was not declared in this scope

Is there a recommended way to handle this? Use a #if check for the MDMPWRON pin? Use a #if check for MODEM_ON_BOARD?

Is it better to just use the default power implementation here in the driver and leave it up to the end user to implement this in an onboard_modem_api.c file?

@trowbridgec can you follow the implementation from EC2X.

@trowbridgec You should declare the pins as class variables, e.g. TELIT_ME910.h:

bool _active_high; DigitalOut _pwr; DigitalOut _rst;

And then use the variables instead of compile-time defines, e.g. TELIT_ME910.cpp:

nsapi_error_t TELIT_ME910::hard_power_on() { if (_pwr.is_connected()) { _pwr = _active_high;

In addition, the pins need to be provided in the class constructor.

0Grit · 2019-05-02T15:15:23Z

@AriParkkila Any further recommendations to get this merge-able?

adbridge · 2019-05-17T10:00:55Z

@AriParkkila could you re-review this please?

AriParkkila · 2019-05-17T12:15:32Z

@trowbridgec the pins should be defined similar to that in EC2X, see how the class constructor takes pins and power functions use those as member variables.

…tom get_context() function

trowbridgec · 2019-06-06T18:31:19Z

@AriParkkila Thanks for your help on this!

I implemented the changes you asked for; please review!

AriParkkila · 2019-06-07T05:02:34Z

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910_CellularContext.cpp

+
+    _at.resp_stop();
+    if (_cid == -1) { // no suitable context was found so create a new one
+        if (!set_new_context(1)) {


This function could probably be removed? This line seems to be the only diff to parent implementation, and this is likely incorrect, this should use _cid after the last in use, see parent implementation.

Being able to write a custom version of the get_context() function for this driver was the motivation for PR #10442. This function has 2 slight modifications from what's in AT_CellularContext.cpp:

The modified if statement in the comment above.

If we can't find a good APN candidate, the logic that seems to work best for this module is to just use context 1 instead of creating a 7th (which is not allowed).

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910_CellularContext.cpp

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910.cpp

AriParkkila · 2019-06-07T05:13:12Z

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910.cpp

+    // +CGREG:<stat>[,[<lac>],[<ci>],[<AcT>],[<rac>][,,[,[<ActiveTime>],[<PeriodicRAU>],[<GPRSREADYtimer>]]]]
+    // Current setting: enable the network registration unsolicited result code, and selects the
+    // short format
+    _at->cmd_start("AT+CGREG=1");


Could this be removed, as it will be overridden by cellular_properties.

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910.cpp

AriParkkila · 2019-06-07T06:17:29Z

@ARMmbed/mbed-os-maintainers Any guidance you can provide with the copyright texts.

trowbridgec · 2019-06-11T16:02:33Z

features/cellular/framework/targets/TELIT/ME910/TELIT_ME910_CellularContext.cpp

+        if (pdp_type_len > 0) {
+            apn_len = _at.read_string(apn, sizeof(apn) - 1);
+            if (apn_len >= 0) {
+                if (_apn && apn_len > 0 && (strcmp(apn, _apn) != 0)) {


This modified version of the get_context() function adds a check here to only skip over (i.e. continue) a candidate APN if it's string length is greater than 0 (i.e. it's not an empty string).

Original:

if (_apn && (strcmp(apn, _apn) != 0)) { continue; }

Modified:

if (_apn && apn_len > 0 && (strcmp(apn, _apn) != 0)) { continue; }

So even if _apn has been defined you wan't to accept an pdp context with empty apn if context matches by pdp type?

@jarvte The initial motivation for this change was a scenario that I saw with this module where 6 contexts were already defined (based on the return value from the AT+CGDCONT? command), but all 6 had an empty-string APN. The logic of the current function would reject all 6 of the contexts (because it doesn't exactly match the target APN), and would therefore try to create a new, 7th context which would be denied by the module (it only supports 6 contexts).

I proposed changing the logic of the get_context() function to NOT skip over a context with an empty-string APN (i.e. ONLY skip over a context if it has a non-empty-string APN that DOESN'T match the target APN).

Sorry if I'm explaining this poorly; see PR #10442 for more info.

trowbridgec · 2019-06-18T15:12:33Z

Bump

@maclobdell @adbridge

adbridge · 2019-06-18T15:52:26Z

CI started

mbed-ci · 2019-06-19T00:55:46Z

Test run: SUCCESS

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

trowbridgec changed the title ~~Telit me910 driver~~ Telit ME910 driver Apr 25, 2019

trowbridgec mentioned this pull request Apr 25, 2019

Telit LE910 and ME910 #8666

Closed

ciarmcom requested review from AriParkkila, hasnainvirk, maclobdell and a team April 25, 2019 19:00

ciarmcom added the needs: review label Apr 25, 2019

AriParkkila suggested changes Apr 26, 2019

View reviewed changes

trowbridgec changed the title ~~Telit ME910 driver~~ Cellular: Telit ME910 driver May 2, 2019

AGlass0fMilk mentioned this pull request May 31, 2019

Cellular PSM - Wake Module Provisions? #10737

Closed

trowbridgec force-pushed the telit-me910-driver branch from d968dab to 0a717e4 Compare June 6, 2019 14:26

Chris Trowbridge added 7 commits June 6, 2019 10:26

Add Telit ME910 driver

841607d

Remove default power on/off functions

1a20ea1

Remove unnecessary #includes

f0afd1f

Remove ME910 flow control debug message

21a8ed8

Remove ME910 trace group #define

0d9facd

Add default soft/hard power on/off sequences for ME910

0a717e4

Modify ME910 driver to follow power on/off best practices and add cus…

9a1309f

…tom get_context() function

AriParkkila suggested changes Jun 7, 2019

View reviewed changes

trowbridgec commented Jun 11, 2019

View reviewed changes

Chris Trowbridge added 2 commits June 12, 2019 09:36

Remove unnecessary init AT commands

ea5d8cf

Moved DEFAULT_DELAY_BETWEEN_AT_COMMANDS constant to cpp file

28f7a4e

jarvte approved these changes Jun 14, 2019

View reviewed changes

adbridge approved these changes Jun 18, 2019

View reviewed changes

adbridge added needs: CI release-version: 5.13.1 and removed needs: review labels Jun 18, 2019

adbridge added ready for merge and removed needs: CI labels Jun 19, 2019

adbridge merged commit 2fd7f80 into ARMmbed:master Jun 19, 2019

adbridge removed the ready for merge label Jun 19, 2019

trowbridgec deleted the telit-me910-driver branch June 19, 2019 12:06

Cellular: Telit ME910 driver #10484

Cellular: Telit ME910 driver #10484

Uh oh!

Conversation

trowbridgec commented Apr 25, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

trowbridgec commented Apr 25, 2019

Uh oh!

ciarmcom commented Apr 25, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0Grit commented May 2, 2019

Uh oh!

adbridge commented May 17, 2019

Uh oh!

AriParkkila commented May 17, 2019

Uh oh!

trowbridgec commented Jun 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AriParkkila commented Jun 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarvte Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trowbridgec commented Jun 18, 2019

Uh oh!

adbridge commented Jun 18, 2019

Uh oh!

mbed-ci commented Jun 19, 2019

Uh oh!

Uh oh!

jarvte Jun 12, 2019 •

edited

Loading