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: ATHandler send delay #6572

Merged
merged 2 commits into from Apr 10, 2018

Conversation

Projects
None yet
6 participants
@TeemuKultala
Contributor

TeemuKultala commented Apr 9, 2018

Description

For some modems there is a minimum delay between the end of the response of the previous command and the beginning of a new command, and this change takes into account that kind of requirement. The delay set to 20 ms for Telit HE910 modems. The change is tested with a Telit HE910 modem

Pull request type

[ X ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

TeemuKultala added some commits Apr 9, 2018

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Apr 9, 2018

@AriParkkila please review

@AriParkkila

PR description should state this as a fix, here implemented for TELIT.

@@ -893,6 +896,8 @@ void ATHandler::resp_stop()
set_tag(&_resp_stop, OK);
// Reset info resp prefix
memset(_info_resp_prefix, 0, sizeof(_info_resp_prefix));
_last_response_stop = rtos::Kernel::get_ms_count();

This comment has been minimized.

@AriParkkila

AriParkkila Apr 9, 2018

Contributor

Should be 0 to avoid redundant initial waiting.

This comment has been minimized.

@TeemuKultala

TeemuKultala Apr 9, 2018

Contributor

Set 0 in the constructor

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 9, 2018

PR description should state this as a fix, here implemented for TELIT.

I am confused. It's new API there so feature it is?

@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented Apr 9, 2018

There are no changes in API folder, and thus, no application using cellular need to be changed so I'd say it's not a feature.

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Apr 9, 2018

Set as Fix according to Ari's comments

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 10, 2018

I could go either way with this PR. the API for ATHandler is definitely updated, and in the past, constructor defaults that changed in a backwards compatible way were still marked as a feature, but this is also appears to be an internal-only change.

I'm fine with having it marked as a fix.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 10, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 10, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@@ -58,7 +59,7 @@ static const uint8_t map_3gpp_errors[][2] = {
{ 146, 46 }, { 178, 65 }, { 179, 66 }, { 180, 48 }, { 181, 83 }, { 171, 49 },
};
ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, int timeout, const char *output_delimiter) :
ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, int timeout, const char *output_delimiter, uint16_t send_delay) :

This comment has been minimized.

@0xc0170

0xc0170 Apr 10, 2018

Member

uint16_t send_delay - why uint16_t was chosen? vs int timeout (3rd parameter)

This comment has been minimized.

@TeemuKultala

TeemuKultala Apr 10, 2018

Contributor

the delay is always positive, and 16 bits is enough for it for practical cases

This comment has been minimized.

@0xc0170

0xc0170 Apr 10, 2018

Member

OK. I was just asking as basic types should be sufficient , if there was a specific reason to use uint16_t (why then timeout is plain int). If I add a new method here, I would be confused which one to use

@cmonr

cmonr approved these changes Apr 10, 2018

@cmonr cmonr merged commit 1c6d485 into ARMmbed:master Apr 10, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10023 cycles (+749 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B
Details
travis-ci/tools Local tools testing has passed
Details
@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 11, 2018

@TeemuKultala is ATHandler only visible internally or is it exposed to the public API ?

@TeemuKultala TeemuKultala deleted the TeemuKultala:at_send_wait branch Apr 12, 2018

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Apr 12, 2018

@adbridge ATHandler is not available in the API folder, so it is not a part of the public API

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