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

At handler improvements #11306

Merged
merged 2 commits into from Sep 2, 2019

Conversation

@AnttiKauppila
Copy link
Contributor

commented Aug 23, 2019

Description

New ATHandler functions taken into use for rest of the targets (BG96 was updated initially) to reduce code size. This means basically that new functions using variadic list approach are taken into use and with those one can usually write AT commands in single line instead of multiple lines.
Only internal changes and API's are not modified.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-wan

Release Notes

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

How do these 2 commits differ if both have the same commit msg ? Shouldn't it then be one or commit messages changed?

@AnttiKauppila AnttiKauppila force-pushed the AnttiKauppila:ATHandler_improvements branch from 0e290d3 to eec276e Aug 23, 2019
@AnttiKauppila

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Some git magic :)

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Aug 23, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

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

Copy link
Member

left a comment

Can we at least expand the commit message?
26 files changed and do not find there what improvements - I've noticed we use new commands like cmd_start_stop instead of previously start/write/stop. Brief description would be nice to have.

Copy link
Contributor

left a comment

Couldnt spot any issue. Checked WISE_1570 TCP socket tests - passing.

_at.write_int(read_len);
_at.cmd_stop();

_at.cmd_start_stop("+QIRD", "=", "%d%d%d%d", 0, 1, socket->id, read_len);

This comment has been minimized.

Copy link
@mirelachirica

mirelachirica Aug 26, 2019

Contributor

Would it be good to maintain the explanatory comments for 0 and 1 usage?

This comment has been minimized.

Copy link
@AnttiKauppila

AnttiKauppila Aug 26, 2019

Author Contributor

Better would be to use defines instead of magical numbers

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 26, 2019
@AnttiKauppila

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Can we at least expand the commit message?
26 files changed and do not find there what improvements - I've noticed we use new commands like cmd_start_stop instead of previously start/write/stop. Brief description would be nice to have.

Updated, is it better now?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Also commit message (I dont see the update here) ?

@AnttiKauppila

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Propose a better one. What do you want to see in it?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

New ATHandler functions taken into use for rest of the targets (BG96 was updated initially) to reduce code size. This means basically that new functions using variadic list approach are taken into use and with those one can usually write AT commands in single line instead of multiple lines.
Only internal changes and API's are not modified.

This should be sufficient in the commit msg (explains what is being improved and why).

New ATHandler functions taken into use for rest of the targets (BG96 was updated initially) to reduce code size. This means basically that new functions using variadic list approach are taken into use and with those one can usually write AT commands in single line instead of multiple lines.
Only internal changes and API's are not modified.
@AnttiKauppila AnttiKauppila force-pushed the AnttiKauppila:ATHandler_improvements branch from eec276e to d08d55d Aug 27, 2019
_at->cmd_start("AT+CFUN=0");
_at->cmd_stop_read_resp();
_at->at_cmd_discard("+CFUN", "=0");

if (_at->get_last_error() == NSAPI_ERROR_OK) {

This comment has been minimized.

Copy link
@mudassar-ublox

mudassar-ublox Aug 28, 2019

Contributor

"_at->get_last_error()" looks outside lock/unlock as "_at->at_cmd_discard" is unlocking inside it.

Timer t1;
t1.start();
while (!(t1.read() >= 180)) {
_at.cmd_start("AT+UPSND=" PROFILE ",8");
_at.cmd_stop();
_at.cmd_start_stop("+UPSND", "=", "%d%d", PROFILE, 8);

This comment has been minimized.

Copy link
@mudassar-ublox

mudassar-ublox Aug 28, 2019

Contributor

Calling without at lock().

@0xc0170 0xc0170 self-requested a review Aug 29, 2019
@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 29, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

One small styling change needed (see astyle job failure).

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@AnttiKauppila Is this targeting 5.14? Not yet labeled !

@AnttiKauppila AnttiKauppila force-pushed the AnttiKauppila:ATHandler_improvements branch from 71d5753 to 5076792 Aug 29, 2019
@AnttiKauppila AnttiKauppila force-pushed the AnttiKauppila:ATHandler_improvements branch from 5076792 to 9151606 Aug 30, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Sep 2, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Known issue with CI, restarting the build

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 2, 2019

Test run: SUCCESS

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

@AnttiKauppila

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

@0xc0170 Is this now ready to go in.

@0xc0170
0xc0170 approved these changes Sep 2, 2019
@0xc0170 0xc0170 merged commit 39733cb into ARMmbed:master Sep 2, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(-1556 bytes) RAM(-136 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8330 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@AnttiKauppila AnttiKauppila deleted the AnttiKauppila:ATHandler_improvements branch Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.