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

Cellular: Fix for AT handler consume to tag #8350

Merged
merged 1 commit into from Oct 23, 2018

Conversation

Projects
None yet
8 participants
@mirelachirica
Contributor

mirelachirica commented Oct 9, 2018

Description

This PR addresses: #8308

"I'm trying to make a driver for a sim5320 using cellular framework, but ATHandler doesn't parse some responses correctly. After some investigation, I found that problem is caused by ATHandler::consume_to_tag method.

If sequence from buffer contains tag and a symbol before tag coincides with zero symbol of the tag, then the tag won't be detected. For example, if it consumes to a tag "\r\n" in a sequence "\r\r\nOK", the tag won't be found."

Pull request type

[ x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Oct 9, 2018

@jarvte

jarvte approved these changes Oct 10, 2018

@@ -926,16 +926,14 @@ bool ATHandler::consume_to_tag(const char *tag, bool consume_tag)
int c = get_char();
if (c == -1) {
break;
} else if (c == tag[match_pos]) {
} else if (c == tag[match_pos] || (match_pos = 1 && c == tag[--match_pos])) {

This comment has been minimized.

@mirelachirica

mirelachirica Oct 10, 2018

Contributor

@AnttiKauppila there is compile warning about the "match_pos = 1": operation on 'match_pos' may be undefined. Is that something we have to avoid?
The alternative way which I tried to avoid is an else branch that in case c == tag[0] would have to do same operations as in this branch.

This comment has been minimized.

@mirelachirica

mirelachirica Oct 10, 2018

Contributor

Adding parenthesis around "match_pos = 1" cancels the warning. I updated and added explanatory comments also.

This comment has been minimized.

@marcemmers

marcemmers Nov 6, 2018

Contributor

This is still causing a warning when compiling with IAR. Could be that we are using version 8 of IAR. Anyway, you could also do the assignment directly which does solve the warning for IAR:
} else if (c == tag[match_pos] || c == tag[match_pos = 0]) {

@mirelachirica mirelachirica force-pushed the mirelachirica:at_consume_to_tag_fix branch from 8757902 to 59f3193 Oct 10, 2018

@jarvte

jarvte approved these changes Oct 10, 2018

@vznncv

vznncv approved these changes Oct 15, 2018

@0xc0170

Fix looks good but the commit msg could have details from Description

If sequence from buffer contains tag and a symbol before tag coincides with zero symbol of the tag, then the tag won't be detected. For example, if it consumes to a tag "\r\n" in a sequence "\r\r\nOK", the tag won't be found."

To answer question - how this fixes the tag

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 16, 2018

Cellular: Fix for AT handler consume to tag
If sequence from buffer contains tag but symbol before tag is same as
first symbol of the tag, then the tag wasn't detected.

For example, "\r\n" tag was not found from "\r\r\nOK" sequence.

@mirelachirica mirelachirica force-pushed the mirelachirica:at_consume_to_tag_fix branch from 59f3193 to 31f153a Oct 19, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 22, 2018

LGTM (cant approve as github is having issues), will start CI

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 22, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit ba23fef into ARMmbed:master Oct 23, 2018

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 672 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10212 cycles (-61 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Oct 23, 2018

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