Fix issues in setting radio access technology #12020

linlingao · 2019-12-03T20:47:00Z

Summary of changes

Fix a missing comma when radio access technology is specified. This missing comma causes modem to return syntax error.
Add implementation for setting radio access technology for Telit ME910. Without this, RAT is set back to UNKNOWN.
Add configuration in JSON to allow PLMN fallback to auto if manual selection fails.

Impact of changes

Migration actions required

Documentation

None

Pull request type

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

Test results

[] 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

linlingao · 2019-12-03T20:48:19Z

Thanks to @marcuschangarm for spotting and identifying most of the issues addressed in this PR.

ciarmcom · 2019-12-03T22:00:29Z

@linlingao, thank you for your changes.
@ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

AriParkkila · 2019-12-04T06:15:24Z

features/cellular/framework/AT/ATHandler.h

@@ -48,7 +48,7 @@ class FileHandle;
 extern const char *OK;
 extern const char *CRLF;

-#define BUFF_SIZE 32
+#define BUFF_SIZE 128


Any specific reason for this?

Some AT commands such as AT+CGDCONT have longer than 32-character responses. When AT debug is turned on, you'll see AT ERR instead of AT RX. See the trace below:

Increasing the buffer to 128 eliminates these errors and makes debug easier. I feel like it's a good tradeoff to show most commonly used AT commands with minimal RAM usage increase.

Could you check if you have made any changes in ATHandler? The ERR print should be from fill_buffer, but it prints first "AT overflow" which is missing in the log above.

BUFF_SIZE is actually needed to read just the prefix, that's +CGDCONT: in the log above, so it should be large enough to hold the longest prefix and 32 should be enough here.

I didn't make other changes. With the increased BUFF_SIZE, the AT ERR goes away.
BUFF_SIZE is used in ATHandler::read_int()
int32_t ATHandler::read_int()
{
if (!ok_to_proceed() || !_stop_tag || _stop_tag->found) {
return -1;
}

char buff[BUFF_SIZE]; if (read_string(buff, sizeof(buff)) == 0) { return -1; }

which is called by ATHandler::at_cmd_int() while reading response.

resp = read_int(); resp_stop(); return unlock_return_error();

The code above indicates BUFF_SIZE is also used in reading response.
If you look at AT_CellularContext::get_context(), _at.read_int() is called in response to AT+CGDCONT?. This explains why the increase of BUFF_SIZE eliminates AT ERR.
I do see BUFF_SIZE is used for prefix as well as you said.

@linlingao Can you share the code how you are reading the CGDCONT response, that might be missing something like while (_at.info_resp())?

@linlingao The reason I want clean logs just with the codes from master to see if the modem response is missing <cr><ln>, which would explain buffer overflows. Those can be seen in your log but not in the log above.

Are you convinced this fix is good? Is there anything else you'd like me to investigate pertaining to this fix?

@linlingao I'd still like to understand the root cause, otherwise it's possible that increasing BUFF_SIZE does not fix the problem but it just hides the ERR prints.

@AriParkkila Would you like me to revert this change (other than the errors seen in the trace, there doesn't seem to be significant issues) while you continue to investigate? I'd like to get the other more important fixes in.

@linlingao That sounds good since BUFF_SIZE is not directly related to this PR. If you can repeat the BUFF_SIZEproblem and get the AT logs at DEBUG level with master codes (without any changes) we should be able to reproduce the problem to study it further.

AriParkkila · 2019-12-04T06:27:20Z

features/cellular/framework/AT/AT_CellularNetwork.cpp

        } else {
-            return _at.at_cmd_discard("+COPS", "=1,2", "%s", plmn);
+            return _at.at_cmd_discard("+COPS", "=4,2,", "%s", plmn);


This changes network selection to be first manual and then automatic (mode=4). Do you have some specific reason for this change? Currently, if PLMN is defined only that PLMN is allowed for to avoid any unexpected costs, e.g. due to roaming.

The purpose of having the auto fallback is to increase the chance of connecting to a network. I also noticed this is the setting used in AT command tester from M2Msupport. I wasn't aware that roaming cost was a factor. I can change it back to 1 if you think cost outweighs connectivity.

Do you think COPS mode could be a configuration option in mbed_lib.json?

Not all modems support COPS mode 4, so that should also be defined for each modem as CellularProperty.

I can add a config in mbed_lib.json to COPS mode. There're only two modes to select from:manual and manual then automatic. Do you have preference on the name of the config?
Yes, I can add COPS mode 4 to CellularProperty. Let me know if you have preference on the name of the enum.

I added a config in mbed_lib.json for choosing either manual or manual then automatic.
WRT adding a property in CellularProperty, I checked all the modems supported in our stack except RiotMicro RM1000 (I can't find their AT commands online), they all support mode 4. Do you still want me to add a property?

BC95 supports just COPS =0, =1 and =2. That may have changed in some of the latest release?

Most likely. According to their spec, 4 is supported.
https://www.quectel.com/UploadImage/Downlad/Quectel_BC95_AT_Commands_Manual_V1.9.pdf

However, there is still a note that Only =0, =1 and =2 are supported. on the page 33.

I missed the note. They shouldn't have listed 3 and 4 in parameters if they're not supported. Good catch.

adbridge · 2019-12-13T16:04:30Z

@linlingao please address the comments, also a rebase will be needed by the looks of it.

linlingao · 2019-12-13T21:18:33Z

I have incorporated review comments and rebased.

kjbracey · 2019-12-19T11:12:14Z

Needs another rebase, I'm afraid. @AriParkkila, can you review changes?

AriParkkila · 2019-12-19T11:31:35Z

@linlingao Can you remove BUFF_SIZE from here and open another issue for it, also SARA4_PPP.cpp and GENERIC_AT3GPP.cpp are missing PROPERTY_AT_COPS_FALLBACK_AUTO.

… reliability. Fix a syntax error when RAT is set

…manual operator selection

… entries

linlingao · 2019-12-19T20:50:33Z

Incorporated new comments and rebased again.

adbridge · 2019-12-24T12:11:27Z

CI started

mbed-ci · 2019-12-24T12:39:05Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR

0xc0170 · 2020-01-02T08:17:22Z

CI restarted

mbed-ci · 2020-01-02T10:03:47Z

Test run: SUCCESS

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

adbridge · 2020-02-17T12:44:08Z

@linlingao we missed this at the time, but is this adding an internal only implementation or is this new API exposed to the user ? In which case it should be a feature updated not a patch....

0xc0170 · 2020-02-17T15:52:08Z

@linlingao we missed this at the time, but is this adding an internal only implementation or is this new API exposed to the user ? In which case it should be a feature updated not a patch....

It added a new config value to targets config. Please @linlingao confirm.

linlingao · 2020-03-02T22:39:41Z

@adbridge @0xc0170 No new API was added in this PR. I updated the code to handle radio access technology more cleanly.

adbridge · 2020-03-03T10:59:24Z

@linlingao yes but does this change anything in the way the user needs to configure things?

linlingao · 2020-03-03T15:31:00Z

@adbridge This PR doesn't "change" anything in the way the user configures RAT. I added "plmn-fallback-auto" in json to allow the user to fine tune RAT. Prior to this PR, when manual is selected via json, network search would be limited to the specified network only. If the user wishes to limit to the specified network but this network doesn't work, now the user has a way to fall back to automatic selection. Do we consider addition to JSON an API change or new API? If so, we can mark this "feature update". This PR also fixes a bug, should I mark both "feature update" and "patch update"?

adbridge · 2020-03-04T11:31:17Z

@linlingao For future reference it isn't good practice to add bug fixes and additional changes in the same PR. Anything that changes the user experience or means they need or can do anything different to what they did before is not just a 'bug fix' (unless the change is a direct result of something that was known to be wrong already according to technical spec or the documentation). Even changes to JSON files could be considered as a functionality change (in some instances). In this case you are adding additional configuration which can be used so I would suggest this counts as functionality change - so PR type should be changed accordingly. I don't think any additional documentation is required though. Impact of changes and migration is only mandatory for major changes.

ciarmcom requested review from a team December 3, 2019 22:00

ciarmcom added the needs: review label Dec 3, 2019

AriParkkila reviewed Dec 4, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Dec 11, 2019

linlingao force-pushed the fix_rat branch from 8df6e7a to 1234936 Compare December 13, 2019 20:51

kjbracey added needs: review needs: work and removed needs: work needs: review labels Dec 19, 2019

linlingao added 5 commits December 19, 2019 11:21

Increase BUFF_SIZE to avoid error in AT response when debug is turned on

9e17653

Change AT+COPS mode to manual first then fall back to auto to improve…

a5a9683

… reliability. Fix a syntax error when RAT is set

Implement set_access_technology_impl for Telit ME910

d5d4520

add config option and property to choose automatic fallback for PLMN …

c8524c2

…manual operator selection

Revert BUFF_SIZE fix, to be fixed in another PR. Add missing property…

0bc8f4f

… entries

linlingao force-pushed the fix_rat branch from 1234936 to 0bc8f4f Compare December 19, 2019 19:48

AriParkkila approved these changes Dec 20, 2019

View reviewed changes

adbridge approved these changes Dec 24, 2019

View reviewed changes

adbridge added needs: CI and removed needs: work labels Dec 24, 2019

0xc0170 added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: CI labels Jan 2, 2020

0xc0170 merged commit 6baa5e4 into ARMmbed:master Jan 2, 2020

adbridge added the Incomplete Release Notes label Feb 17, 2020

Fix issues in setting radio access technology #12020

Fix issues in setting radio access technology #12020

Conversation

linlingao commented Dec 3, 2019 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

linlingao commented Dec 3, 2019

ciarmcom commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linlingao Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adbridge commented Dec 13, 2019

linlingao commented Dec 13, 2019

kjbracey commented Dec 19, 2019

AriParkkila commented Dec 19, 2019

linlingao commented Dec 19, 2019

adbridge commented Dec 24, 2019

mbed-ci commented Dec 24, 2019

0xc0170 commented Jan 2, 2020

mbed-ci commented Jan 2, 2020

adbridge commented Feb 17, 2020

0xc0170 commented Feb 17, 2020

linlingao commented Mar 2, 2020

adbridge commented Mar 3, 2020

linlingao commented Mar 3, 2020

adbridge commented Mar 4, 2020

linlingao commented Dec 3, 2019 •

edited

Loading

linlingao Dec 5, 2019 •

edited

Loading