Cellular: Changed ATHandler yield to wait #6744

AriParkkila · 2018-04-25T11:43:07Z

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 · 2018-04-25T11:43:53Z

Please review @jarvte

jarvte · 2018-04-25T11:47:39Z

remove also other yield to wait

0xc0170 · 2018-04-25T11:57:27Z

remove also other yield to wait

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

jarvte · 2018-04-25T12:03:41Z

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 😁

kjbracey · 2018-04-25T12:33:40Z

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 · 2018-04-25T12:49:49Z

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

kjbracey · 2018-04-25T13:21:49Z

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).

mirelachirica · 2018-05-02T13:26:40Z

features/cellular/framework/AT/ATHandler.cpp

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


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?

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.

mirelachirica · 2018-05-02T13:29:15Z

features/cellular/framework/AT/ATHandler.cpp

-            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


I would move this comment bellow "else if"

mirelachirica · 2018-05-02T13:56:25Z

features/cellular/framework/AT/ATHandler.cpp

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


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

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 · 2018-05-03T11:45:36Z

Requests above fixed in 13c7d05

kjbracey · 2018-05-03T12:16:56Z

features/cellular/framework/AT/ATHandler.cpp

@@ -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 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.

kjbracey · 2018-05-03T12:44:55Z

features/cellular/framework/AT/ATHandler.cpp


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


Left this variable behind.

kjbracey · 2018-05-03T12:48:19Z

features/cellular/framework/AT/ATHandler.cpp


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


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 · 2018-05-03T15:38:29Z

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

0xc0170 · 2018-05-04T11:06:40Z

@mirelachirica @jarvte Please review the update

AriParkkila · 2018-05-09T13:45:40Z

1f75d3b

0xc0170 · 2018-05-09T14:04:31Z

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

AriParkkila · 2018-05-14T09:39:09Z

rebased

cmonr · 2018-05-14T15:58:22Z

/morph build

mbed-ci · 2018-05-14T16:42:08Z

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 · 2018-05-15T04:12:55Z

Test : SUCCESS

Build number : 1816
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6744/1816

mbed-ci · 2018-05-15T05:32:11Z

Exporter Build : SUCCESS

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

0xc0170 requested a review from jarvte April 25, 2018 11:47

0xc0170 added the needs: review label Apr 25, 2018

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

AriParkkila force-pushed the yield-to-wait branch from 0344090 to 63ce7ba Compare April 25, 2018 12:06

0xc0170 requested a review from kjbracey April 25, 2018 15:33

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

mirelachirica reviewed May 2, 2018

View reviewed changes

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

kjbracey reviewed May 3, 2018

View reviewed changes

kjbracey previously approved these changes May 3, 2018

View reviewed changes

kjbracey reviewed May 3, 2018

View reviewed changes

0xc0170 added needs: review and removed needs: work labels May 3, 2018

AriParkkila dismissed kjbracey’s stale review via ad37e90 May 4, 2018 06:22

kjbracey previously approved these changes May 4, 2018

View reviewed changes

jarvte approved these changes May 4, 2018

View reviewed changes

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

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

Ari Parkkila added 5 commits May 14, 2018 10:23

Cellular: Changed ATHandler yield to wait

98ef516

Cellular: AT requests are to be processed within time period

559abd3

Cellular: ATHandler yield to wait review fixes

9b896a1

Cellular: Fixed ATHandler write poll timeout

3bdb6d1

Cellular: AT handler review fixes

287a1a8

AriParkkila dismissed kjbracey’s stale review via 287a1a8 May 14, 2018 07:37

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

cmonr added release-version: 5.8.5 release-version: 5.9.0-rc1 and removed release-version: 5.8.5 labels May 14, 2018

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

kjbracey approved these changes May 15, 2018

View reviewed changes

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

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

cmonr removed the ready for merge label May 15, 2018

AriParkkila deleted the yield-to-wait branch September 10, 2018 08:26

Cellular: Changed ATHandler yield to wait #6744

Cellular: Changed ATHandler yield to wait #6744

Uh oh!

Conversation

AriParkkila commented Apr 25, 2018

Description

Pull request type

Uh oh!

AriParkkila commented Apr 25, 2018

Uh oh!

jarvte commented Apr 25, 2018

Uh oh!

0xc0170 commented Apr 25, 2018

Uh oh!

jarvte commented Apr 25, 2018

Uh oh!

kjbracey commented Apr 25, 2018

Uh oh!

AriParkkila commented Apr 25, 2018

Uh oh!

kjbracey commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AriParkkila commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented May 3, 2018

Uh oh!

0xc0170 commented May 4, 2018

Uh oh!

AriParkkila commented May 9, 2018

Uh oh!

0xc0170 commented May 9, 2018

Uh oh!

AriParkkila commented May 14, 2018

Uh oh!

cmonr commented May 14, 2018

Uh oh!

mbed-ci commented May 14, 2018

Triggering tests

Uh oh!

mbed-ci commented May 15, 2018

Uh oh!

mbed-ci commented May 15, 2018

Uh oh!

Uh oh!