Bridge.call: move result retrieval to separate API #16
Conversation
src/bridge.h
Outdated
| // Lock read mutex | ||
| if (_timeout < 0) _timeout = timeout_ms; | ||
| int start = millis(); | ||
| while(true && (_timeout < 0 || (millis() - start) < _timeout)) { |
There was a problem hiding this comment.
redundant condition
| while(true && (_timeout < 0 || (millis() - start) < _timeout)) { | |
| while (_timeout < 0 || (millis() - start) < _timeout) { |
src/bridge.h
Outdated
| if (client->get_response(msg_id_wait, result)) { | ||
| k_mutex_unlock(read_mutex); | ||
| k_msleep(1); | ||
| break; | ||
| } | ||
| k_mutex_unlock(read_mutex); | ||
| k_msleep(1); |
There was a problem hiding this comment.
fold k_mutex_unlock and k_msleep, to make the lock/unlock a bit more readable
| if (client->get_response(msg_id_wait, result)) { | |
| k_mutex_unlock(read_mutex); | |
| k_msleep(1); | |
| break; | |
| } | |
| k_mutex_unlock(read_mutex); | |
| k_msleep(1); | |
| auto gotResp = client->get_response(msg_id_wait, result); | |
| k_mutex_unlock(read_mutex); | |
| k_msleep(1); | |
| if (gotResp) { | |
| break; | |
| } |
New calls will be in the form:
Bridge.call("test", 1, 2) -> returns true if the call succeeded, ignore the return
Bridge.call("test", 1, 2).result(c) -> stores the result in variable c, returns true if succeeded
Bridge.call("test", 1, 2).timeout(20) -> waits 20ms for the result, then returns fals if timeout is reached
Bridge.call("test", 1, 2).timeout(20).result(c) -> same but storing the result to variable c
Bridge.call("test", 1, 2).result(c, 20) -> same as previous call
* Usages that do not held the expected behaviour:
Bridge.call("test", 1, 2).result(c).timeout(20) -> doesn't apply the timeout, waits forever
facchinm
force-pushed
the
new_call_api
branch
from
5a96599 to
7932a81
Compare
eigen-value
left a comment
eigen-value
left a comment
There was a problem hiding this comment.
I have a general concern about usability of the proposed solution.
src/bridge.h
Outdated
| k_yield(); | ||
| } | ||
| } | ||
| return (client->lastError.code == NO_ERR) && (_timeout < 0 || (millis() - start) < _timeout); |
There was a problem hiding this comment.
I'd rather rewrite this block so that the timeout condition is evaluated at the end of the loop.
This way we get the chance of forcing:
client->lastError.code = GENERIC_ERR;
client->lastError.message = "Timed out";
Then we could either return or break and leave the last line unmodified (saving yet another call to millis)
fix: RpcResult overloaded bool using a generic return type, not setting a timeout, can potentially execute result method multiple times
| operator bool() { | ||
| char c; | ||
| return result(c); | ||
| } |
There was a problem hiding this comment.
This dummy result (c) call should pass a timeout or it will hang if the receiving end returns a type other than char
# Conflicts: # src/bridge.h
… result retrieval
…hod must be executed exactly once
…wed at Bridge level feat: Bridge notifies Router with a timeout event feat: RpcResult.error field
|
@gbr1 please test the last commit as I'm off today. |
…was consumed but notify PARSING_ERR
feat: RpcResult destructor invokes .result()
|
@gbr1 should be ready to merge. plz test it though |
gbr1
left a comment
gbr1
left a comment
There was a problem hiding this comment.
LGTM (finally - good luck to this PR)
New calls will be in the form:
Bridge.call("test", 1, 2)-> returns true if the call succeeded, ignore the returnBridge.call("test", 1, 2).result(c)-> stores the result in variable c, returns true if succeededBridge.call("test", 1, 2).timeout(20)-> waits 20ms for the result, then returns fals if timeout is reachedBridge.call("test", 1, 2).timeout(20).result(c)-> same but storing the result to variable cBridge.call("test", 1, 2).result(c, 20)-> same as previous callBridge.call("test", 1, 2).result(c).timeout(20)-> doesn't apply the timeout, waits forever