conn: extend API with timeout support#3921
Conversation
|
Let's decide on this after the release. We are included |
sys/net/gnrc/conn/gnrc_conn.c
Outdated
There was a problem hiding this comment.
So recvfrom is now non-blocking? Otherwise, please document, what does 0 mean?
|
Sockets also include |
|
Sure, I'm just hesitant to make recvfrom non-blocking per default. |
|
Recv stays blocking with 0 I meant infinent |
4afa161 to
d64658c
Compare
sys/include/net/conn/ip.h
Outdated
There was a problem hiding this comment.
Behavior for timeout == 0 should be documented here.
|
Can you put the core change in a different PR, please. core changes need two ACKs and this would hold up the review of the rest of this PR ;-) |
|
I will in a moment, but the core change is needed for |
There was a problem hiding this comment.
@DipSwitch Could you explain a bit how this function works?
There was a problem hiding this comment.
@kaspar030 Addressed in the lines below and updated the doxygen documentation
There was a problem hiding this comment.
@DipSwitch I still have no clue. Could you elaborate why a simple
xtimer_set_msg_timeout(timeout);
msg_receive();
is not enough? (no need to put that into doxygen).
edit fixed formatting
There was a problem hiding this comment.
Because for most application protocols you would want to send a message and wait for a response, but if you use UDP for example, the message can get lost and you cannot retransmit your lost packet because the thread would hang till it receives an response.
There was a problem hiding this comment.
Sorry, I messed up the markdown above.
I see the need for recvfrom() with timeout, I just don't get why the function does something three times, some not-and'ing, and then peeks into the message queue, compared to xtimer_set_msg_timeout(); msg_recv();.
There was a problem hiding this comment.
The idea here was to avoid load on the xtimers linked list and all it's calculations if there is no timeout specified. This could be moved to the xtimer driver though.
And the adding and subtracting is done because there is a loop and I would like the timeout value to be hard. Although, this might never be the case if an different message is received.
d64658c to
b3e1052
Compare
|
Split the PR's and updated the documentation. |
|
Please rebase. |
|
This should make it easy to implement select(), I guess. |
|
rebased |
811ffca to
663569c
Compare
663569c to
b1ce2c3
Compare
|
Rebased and squashed since #3997 is merged now. |
There was a problem hiding this comment.
I don't get the comment. There can be other msg_receive()s in the thread. Also: line length ;-)
There was a problem hiding this comment.
@DipSwitch, I had problems understanding this concept in the beginning, too. The idea is that this IPC call will pop the message from the head of the queue and enqueue it at the end.
There was a problem hiding this comment.
And the msg_receive "knows" to ignore the packet it just send back to itself?
There was a problem hiding this comment.
No, if there are less than 3 messages in the queue, it will indeed be checked multiple times. IMO the 3 is rather arbitrary anyway and we need a better concept here in the future. For now I don't see a better solution.
There was a problem hiding this comment.
How about something like this?
Note: It doesn't make sense for main to call msg_send_self but that's just to easily find the entry code of the POC
/**
* @brief POC for usage
* @{
*/
static bool _msg_callback(msg_t *m, void *arg) {
if (m->type == MY_TYPE) {
// handle and return true
*(int*)arg = 1;
return true;
}
return false;
}
static int main(int argc, char **argv) {
volatile int state = 0;
msg_receiver_t cb = { NULL, _msg_callback, &state };
msg_register_receiver(&cb);
msg_t m;
for (;;) {
if (msg_receive(&m) && !_msg_callback(&m)) {
msg_send_self(&m, &cb);
}
switch (state) {
case 0:
// state process for processing data
break;
case 1:
// do a conn transfer, request DNS, request TFTP or other
// functions which are going over the network and can take
// a long time to complete
break;
}
}
msg_unregister_receiver(&cb);
}
/**
* @}
*/
typedef struct msg_receiver {
/**
* @brief next msg_receiver in the chain
*/
struct msg_receiver *next;
/**
* @brief callback to be called by called functions if
* they receive a msg that isn't for them
* if the msg is never accepted it will be dropped
*/
bool (*msg_callback)(msg_t *msg, void *args);
/**
* @brief argument to pass, state object for example
*/
void *arg;
} msg_receiver_t;
/**
* @brief send self doesn't requeue, but searches for next msg handler
*/
void msg_send_self(msg_t *m, msg_receiver_t *current) {
while (current->next) {
current = current->next;
if (current->msg_callback(m, current->arg)) {
return;
}
}
// drop the message because it isn't important
}
/**
* @brief before calling a function which handles messages you register your callback
*/
void msg_register_receiver(msg_receiver_t *recv) {
LL_PREPEND(current_thread->msg_receivers);
}
/**
* @brief after returning from the function you unregister your callback
*/
void msg_unregister_receiver(msg_receiver_t *recv) {
// check if no upper frame forgot to unregister it's receiver
assert(recv == current_thread->msg_receivers);
current_thread->msg_receivers = recv->next;
}There was a problem hiding this comment.
That's basically the same you proposed in your issue, right?
|
Since we plan to rediscuss conn for the next release I would prefer to postpone this one for the next release. |
|
Fine with me :) |
|
From what I can gather one can easily implement the required behavior with the receive callbacks I proposed for |
|
I would rather use that to e.g. implement timeout behavior externally, than adding timers as a required dependency for |
|
Yes I guess that would work as well. Then the callback can send a message to the waiting thread :) |
|
Okay, then I close this PR |
Extending the
connAPI to support timeouts.This PR depends on #3997 to implement
-EWOULDBLOCK