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: Changed ATHandler yield to wait #6744

Merged
merged 5 commits into from May 15, 2018

Conversation

Projects
None yet
7 participants
@AriParkkila
Contributor

AriParkkila commented Apr 25, 2018

Description

Yield doesn't really achieve very much - it's still using 100% of CPU power and it won't let lower-priority threads run.

Fixes internal issue OTCELL-556.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented Apr 25, 2018

Please review @jarvte

@0xc0170 0xc0170 requested a review from jarvte Apr 25, 2018

@jarvte

This comment has been minimized.

Contributor

jarvte commented Apr 25, 2018

remove also other yield to wait

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 25, 2018

remove also other yield to wait

Please use request changes that would make this red and needs: work label applied immediately

@jarvte

This comment has been minimized.

Contributor

jarvte commented Apr 25, 2018

Please use request changes that would make this red and needs: work label applied immediately

Yes, I wasn't added as a reviewer yet. Too fast 😁

@AriParkkila AriParkkila force-pushed the AriParkkila:yield-to-wait branch from 0344090 to 63ce7ba Apr 25, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 25, 2018

This is clunky - you should be using poll() to wait for readability with a timeout, as used elsewhere for reading. Yes, its implementation currently uses wait_ms itself, but that can be improved in future - it will eventually become proper "sleep and wake only when serial activity or timeout".

@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented Apr 25, 2018

@kjbracey-arm do you mean like ATCmdParser::getc()

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 25, 2018

Yes, like that. I assumed you were still using it for your own normal non-URC reading, but it seems to have fallen out. Having and using the poll() API is fundamental to making any semi-blocking thing like this work efficiently. (Step 2 is actually making the poll implementation efficient, but conceptually it can be - spinning and rechecking can't).

@0xc0170 0xc0170 requested a review from kjbracey-arm Apr 25, 2018

@AriParkkila AriParkkila force-pushed the AriParkkila:yield-to-wait branch 2 times, most recently from 268c49d to 23e98b8 May 2, 2018

fill_buffer();
if (!fill_buffer()) {
tr_error("fill failed %s", prefix);
return;

This comment has been minimized.

@mirelachirica

mirelachirica May 2, 2018

Contributor

What if we have already needed data in buffer, fetched during process_oob() -> match_urc() -> set_scope(InfoType) -> consume_char(' ') -> get_char() -> fill_buffer()? Should return only if buffer is also empty?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 2, 2018

Contributor

fill_buffer is currently waiting for data. Which it should only be doing if it thinks it hasn't got enough. So presumably it shouldn't be being called at all here. Or it should be being called with a zero timeout, at least, if you're trying to get stuff out of the serial port.

resp a bit later does a fill_buffer when it's out of data - that's the one that should have a timeout for the AT response time.

if (mem_str(_recv_buff, _recv_len, CRLF, CRLF_LENGTH)) {
_start_time = rtos::Kernel::get_ms_count(); // time to process next (potential) URC
} else if (mem_str(_recv_buff, _recv_len, CRLF, CRLF_LENGTH)) {
// If no match found, look for CRLF and consume everything up to CRLF

This comment has been minimized.

@mirelachirica

mirelachirica May 2, 2018

Contributor

I would move this comment bellow "else if"

#endif
} while ((uint32_t)timer.read_ms() < _at_timeout);
int timeout = (_start_time + _at_timeout) - rtos::Kernel::get_ms_count();
if (timeout >= 0) {

This comment has been minimized.

@mirelachirica

mirelachirica May 2, 2018

Contributor

@kjbracey-arm suggests to still attempt one poll with 0 timeout even if the timeout expired. Same in write().

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 2, 2018

Contributor

Yes, you may have overshot your precise time, presumably due to CPU load or scheduling or debugger interference, but if you are already in your own processing context you may as well have one last immediate check before you go. No need to say "timeout" if the data is actually already there.

This is consistent with what other APIs that take "until" times do, like Semaphore::wait_until.

uint64_t now = Kernel::get_ms_count();

if (now >= millisec) {
    return wait(0);
} else if (millisec - now >= osWaitForever) {
    // API permits early return
    return wait(osWaitForever - 1);
} else {
    return wait(millisec - now);
}

You might also want to handle the "target time is too big for int32_t" case. So:

if (now >= _start_time + _at_timeout) {
   timeout = 0;
} else if ( _start_time + _at_timeout - now > INT_MAX) {
   timeout = INT_MAX;
} else {
   timeout = _start_time + _at_timeout - now;
}

Unfortunate that there's no poll_until that does that for you. I possibly should add it; I didn't because it doesn't exist in any POSIX or C/C++ standard.

@AriParkkila AriParkkila force-pushed the AriParkkila:yield-to-wait branch from 23e98b8 to 13c7d05 May 3, 2018

@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented May 3, 2018

Requests above fixed in 13c7d05

@@ -1099,7 +1097,12 @@ size_t ATHandler::write(const void *data, size_t len)
fhs.events = POLLOUT;
size_t write_len = 0;
for (; write_len < len; ) {
int count = poll(&fhs, 1, _at_timeout);
int timeout = (_start_time + _at_timeout) - rtos::Kernel::get_ms_count();

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 3, 2018

Contributor

This should probably have the same timeout computation logic as for the fill_buffer, so attempting something if time has passed. So maybe make it a "compute timeout" subroutine.

You only need the device error on the return from poll, not this extra one.

pollfh fhs;
fhs.fh = _fileHandle;
fhs.events = POLLIN;
int count = poll(&fhs, 1, timeout);
int timeout = 0;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 3, 2018

Contributor

Left this variable behind.

set_error(NSAPI_ERROR_DEVICE_ERROR);
tr_debug("AT TIMEOUT, scope: %d timeout: %lu", _current_scope, _at_timeout);
if (wait_for_timeout) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 3, 2018

Contributor

It possibly feels like this set_error should be one layer up, where you're printing the tr_warn("AT TIMEOUT"). The error view maybe depends what you were waiting for.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 3, 2018

@kjbracey-arm Not certain if this is approved and those 2 comments should block this PR ? @AriParkkila please review them

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 4, 2018

@mirelachirica @jarvte Please review the update

@jarvte

jarvte approved these changes May 4, 2018

@cmonr cmonr removed the needs: CI label May 7, 2018

@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented May 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 9, 2018

Can you please rebase instead of merge (the commit is not needed at all here)

@AriParkkila AriParkkila force-pushed the AriParkkila:yield-to-wait branch from 1f75d3b to ad37e90 May 14, 2018

@AriParkkila AriParkkila force-pushed the AriParkkila:yield-to-wait branch from ad37e90 to 287a1a8 May 14, 2018

@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented May 14, 2018

rebased

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 14, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: work needs: CI labels May 14, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented May 14, 2018

Build : SUCCESS

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

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.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 15, 2018

@cmonr cmonr merged commit 991d461 into ARMmbed:master May 15, 2018

13 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/astyle Passed, 845 warnings (+0 warnings)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10246 cycles (+1229 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label May 15, 2018

@AriParkkila AriParkkila deleted the AriParkkila:yield-to-wait branch Sep 10, 2018

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