Skip to content

conn: extend API with timeout support#3921

Closed
DipSwitch wants to merge 1 commit intoRIOT-OS:masterfrom
DipSwitch:conn_timeout_extension
Closed

conn: extend API with timeout support#3921
DipSwitch wants to merge 1 commit intoRIOT-OS:masterfrom
DipSwitch:conn_timeout_extension

Conversation

@DipSwitch
Copy link
Copy Markdown
Member

Extending the conn API to support timeouts.

This PR depends on #3997 to implement -EWOULDBLOCK

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 22, 2015

Let's decide on this after the release. We are included conn primarily to provide POSIX sockets for the release, but it is certainly not set in stone.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So recvfrom is now non-blocking? Otherwise, please document, what does 0 mean?

@DipSwitch
Copy link
Copy Markdown
Member Author

Sockets also include select and support features like MSG_DONTWAIT and O_NONBLOCK. I think it is varly common tot wait for a response with a timeout :)

@miri64 miri64 self-assigned this Sep 22, 2015
@miri64 miri64 added this to the Release NEXT MAJOR milestone Sep 22, 2015
@miri64 miri64 added Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Sep 22, 2015
@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 22, 2015

Sure, I'm just hesitant to make recvfrom non-blocking per default.

@DipSwitch
Copy link
Copy Markdown
Member Author

Recv stays blocking with 0 I meant infinent

@DipSwitch DipSwitch force-pushed the conn_timeout_extension branch 2 times, most recently from 4afa161 to d64658c Compare September 22, 2015 09:24
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior for timeout == 0 should be documented here.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 29, 2015

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

@DipSwitch
Copy link
Copy Markdown
Member Author

I will in a moment, but the core change is needed for -EWOULDBLOCK implementation, so we would need to wait anyway :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DipSwitch Could you explain a bit how this function works?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaspar030 Addressed in the lines below and updated the doxygen documentation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DipSwitch DipSwitch force-pushed the conn_timeout_extension branch from d64658c to b3e1052 Compare September 30, 2015 08:44
@DipSwitch
Copy link
Copy Markdown
Member Author

Split the PR's and updated the documentation.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 27, 2015

Please rebase.

@OlegHahm
Copy link
Copy Markdown
Member

This should make it easy to implement select(), I guess.

@DipSwitch
Copy link
Copy Markdown
Member Author

rebased

@DipSwitch DipSwitch force-pushed the conn_timeout_extension branch from 811ffca to 663569c Compare December 3, 2015 08:54
@DipSwitch DipSwitch force-pushed the conn_timeout_extension branch from 663569c to b1ce2c3 Compare December 3, 2015 08:58
@DipSwitch
Copy link
Copy Markdown
Member Author

Rebased and squashed since #3997 is merged now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the comment. There can be other msg_receive()s in the thread. Also: line length ;-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the msg_receive "knows" to ignore the packet it just send back to itself?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's basically the same you proposed in your issue, right?

@OlegHahm OlegHahm modified the milestones: Release 2015.12, Release 2016.03 Dec 8, 2015
@OlegHahm OlegHahm modified the milestones: Release 2016.03, Release 2015.12 Dec 8, 2015
@miri64
Copy link
Copy Markdown
Member

miri64 commented Mar 23, 2016

Since we plan to rediscuss conn for the next release I would prefer to postpone this one for the next release.

@miri64 miri64 modified the milestones: Release 2016.07, Release 2016.04 Mar 23, 2016
@DipSwitch
Copy link
Copy Markdown
Member Author

Fine with me :)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 6, 2016

From what I can gather one can easily implement the required behavior with the receive callbacks I proposed for conn in #5091. Am I right?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 6, 2016

I would rather use that to e.g. implement timeout behavior externally, than adding timers as a required dependency for conn.

@DipSwitch
Copy link
Copy Markdown
Member Author

Yes I guess that would work as well. Then the callback can send a message to the waiting thread :)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 8, 2016

Okay, then I close this PR

@miri64 miri64 closed this Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants