Skip to content
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

vtimer: Callback timer #860

Closed
miri64 opened this issue Mar 5, 2014 · 22 comments
Closed

vtimer: Callback timer #860

miri64 opened this issue Mar 5, 2014 · 22 comments
Assignees
Labels
Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Comments

@miri64
Copy link
Member

miri64 commented Mar 5, 2014

Currently vtimer does not support callbacks publicly. As far as I know the current approach to circumvent this flaw is to start a thread everytime a certain action is needed after a certain time interval, either by the use of vtimer_sleep(), vtimer_set_wakeup(), or vtimer_set_msg(). Maybe some efficiency would be gained if these actions all were handled by a single thread. A rough scetch would be

typedef struct {
    void (*callback)(void *);
    void *args;
} vtimer_callback_t;

void vtimer_callback_handler(void)
{
    msg_t msg;
    while (1) {
        msg_receive(&msg);
        if (msg->type == MSG_TIMER) {
            void *args = ((vtimer_callback_t *)msg.content.ptr)->args;
            ((vtimer_callback_t *)msg.content.ptr)->callback(args);
        }
    }
}

int vtimer_set_callback(vtimer_t *t, timex_t interval, vtimer_callback_t *callback)
{
    int pid = thread_getpid();
    return vtimer_set_msg(t, intervall, pid, callback);
}
@OlegHahm
Copy link
Member

OlegHahm commented Mar 5, 2014

One has to think about how to determine the best stack size for this handler thread.

@miri64
Copy link
Member Author

miri64 commented Mar 5, 2014

Because I did not know an answer for this, I did not started an actual pull request.

@kaspar030
Copy link
Contributor

I'd prefer instead of the content of the inner if in vtimer_callback_handler sth like

vtimer_callback_t *t = (vtimer_callback_t*) msg.content.ptr;
t->callback(t->args);

for consistency and readability.

@Kijewski
Copy link
Contributor

Kijewski commented Mar 8, 2014

I don't like this idea:

  • How big is the stack of the callback handler?
  • How deep into the stack is it at this point of execution?
  • How big is it's priority?
  • Shouldn't the callback only be called if the priority of the issuer is reasonably high?

vtimer_sleep(), vtimer_set_msg() and vtimer_set_wakeup() solve all this questions neatly, granted, with a little overhead.

@OlegHahm
Copy link
Member

OlegHahm commented Mar 9, 2014

I agree that there some open points to discuss, but I wouldn't discard the idea in general.

How big is the stack of the callback handler?

Definitely a point to discuss, but I guess setting it to an default value of KERNEL_CONF_STACKSIZE_DEFAULT + KERNEL_CONF_STACKSIZE_PRINTF should be as fine in most cases as it is for the rest of the system.

How deep into the stack is it at this point of execution?

I don't understand this question.

How big is it's priority?

Valid question, but if it is well-defined, I don't see a fundamental problem here.

Shouldn't the callback only be called if the priority of the issuer is reasonably high?

Also up to discuss, but assigning a very priority to the thread itself and then evaluating the calling threads priority seems like a good approach to me.

@mehlis mehlis assigned Kijewski and OlegHahm and unassigned mehlis and Kijewski Mar 24, 2014
@OlegHahm OlegHahm added this to the FIX ME FIRST milestone Jun 3, 2014
@OlegHahm
Copy link
Member

Though the issue number is low, I would like to untag this issue, since I still like the idea but wouldn't give it a high priority.

@OlegHahm OlegHahm removed this from the FIX ME FIRST milestone Jun 20, 2014
@OlegHahm
Copy link
Member

Hm, although I agree to Kaspar's concerns, I still think that we should have some solution for this issue.

@OlegHahm
Copy link
Member

And I'm still thinking this.

@DipSwitch
Copy link
Member

Me to, It would be nice to not have to create a task and stack for just one periodical timer which have to run once in a while... One solution would be to use the IDLE task to schedule timed callbacks

@cgundogan
Copy link
Member

It would be nice to not have to create a task and stack for just one periodical timer

If you are using a stack for your main anyways then you don't need an extra thread for your periodic task. .. assuming you do msg_recv() in your main loop

@DipSwitch
Copy link
Member

Valid, but than you would need to write your own API in your application and copy past it to all your projects. Because I would like to give all my event / interrupt driven systems to periodically update something...

I use the ARM system clock for this, but it could be any timer, like xtimer

/**
 * The timed_callback struct can be statically allocated for each callback
 * you might want to 'dynamically' allocate.
 */
typedef struct _timed_callback
{
    LinkedList list;                //!< Linked list support (not to be used by ourself)

    int active;                     //!< Indicates whether the timer is running or not
    int nNumberOfMS;                //!< The amount of milliseconds to wait (this number is updated while the timer is running)
    fpTimedCallback callback;       //!< The callback to call when the timer hits 0
    void *args;                     //!< The arguments which should be passed to the callback
} timed_callback;

/**
 * \brief add a system alarm callback
 *
 * The callback item will be added to the alarm chain. The chain will
 * be updated every system clock
 *
 * \return the number of milliseconds the call will be made
 */
extern int sysclk_add_timed_callback(timed_callback *tc);

/**
 * \brief remove a system alarm callback
 *
 * The callback item will be removed from the alarm chain.
 *
 * \return 0 on success otherwise -1
 */
extern int sysclk_remove_timed_callback(timed_callback *tc);

/**
 * \brief System Clock interrupt handler
 *
 * This functions processes all the requested callback timers if there are any.
 */
void __interrupt SysTick_Handler(void)
{
    // check if we have alarms to process
    volatile timed_callback *list = listHead;

    // increase the SysClock counter with the set
    systicksMS += eCurrentTimeInterval;

#ifdef SYSCLK_TICK_HANDLER
    // call the OS/Application tick handler

    extern void SYSCLK_TICK_HANDLER;
    SYSCLK_TICK_HANDLER;

#endif

    if (delayMS)
        --delayMS;

    while (list)
    {
        // decrease time to wait with sysclock interval
        list->nNumberOfMS -= eCurrentTimeInterval;

        // if the timer hits or lower we invoke the callback
        if (list->nNumberOfMS <= 0)
        {
            // call the callback
            int ret = 0;

            if (list->active)
                ret = list->callback(list->args);

            // if the function returns a value larger then 0 this will be our new interval
            if (ret > 0)
            {
                list->nNumberOfMS = (ret + (eCurrentTimeInterval - (ret % eCurrentTimeInterval)));
            }
            else if (list->active) // assuming we are currently removing the item from the list
            {
                // keep copy
                timed_callback *tmp = (timed_callback*)list;

                // if the value is 0 or negative we remove this item from the list
                list->active = 0;

                // get the next item in the list before we remove the node
                list = (timed_callback*)list->list.next;

                // remove the item from the list
                _sys_clock_remove_tc(tmp);

                // we already have the next list entry, continue while loop
                continue;
            }
        }

        // get the next item in the list
        list = (timed_callback*)list->list.next;
    }
}

@cgundogan
Copy link
Member

Maybe I didn't understand it correctly, but this seems like a re-implementation of the xtimer. Why not just use the xtimer? You could then concentrate only on receiving your custom event in your main loop and executing whatever callback fits to the incoming msg_t. But you would need to re-trigger the timer to make it periodic in the current xtimer implementation. However, that's just one extra line

@DipSwitch
Copy link
Member

This is something I use on bare metal projects :) But I think your right, although it could be nice to add the functionality you described to a module so it's easier to use for everyone. Would this be a good idea?

This would also come in handy for modules I wrote like TFTP and DNS. Since this probably will be system modules, they can't use this "application" logic per default.

@cgundogan
Copy link
Member

I would go for something similar like drafted below. I don't know how to put this programming paradigm into a module (: Doesn't this example fit your needs?

static msg_t msg1 = { .type = MY_PROJECT_MSG_1 }, msg_2 = { .type = MY_PROJECT_MSG_2 };
static xtimer_t timer1, timer2;
static msg_t _msg_q[MY_PROJECT_MSGQ_SZ];
...
int main()
{
    msg_t incoming;
    msg_init_queue(_msg_q, MY_PROJECT_MSGQ_SZ);
    /* start timers */
    xtimer_set_msg(&timer1, 1 * SEC_IN_USEC, &msg1, thread_getpid());
    xtimer_set_msg(&timer2, 5 * SEC_IN_USEC, &msg2, thread_getpid());

    while(1) {
        msg_receive(&incoming);
        switch (incoming.type) {
            case MY_PROJECT_MSG_1:
                callback_1();
                /* this line makes the timer periodic */
                xtimer_set_msg(&timer1, 1 * SEC_IN_USEC, &msg1, thread_getpid());
                break;
            case MY_PROJECT_MSG_2:
                callback_2();
                /* this line makes the timer periodic */
                xtimer_set_msg(&timer2, 5 * SEC_IN_USEC, &msg2, thread_getpid());
                break;
            case ...:
                ...
                break;
        }
...............
    }
}

@DipSwitch
Copy link
Member

Not entierly, but I get the idea. One message type and one xtimer would be enough for me. If you sort them and get the first one that should be triggered. And I want them to be able to dynamically (un)register, so the LCD drawer (for example) won't depend on specific code for the LCD drawer in another location than the LCD drawer itself. Every module / application part needs to be able to work on it's own without depending on code in common sources. Which I most of the time only use to connect parts to one another.

Please note that I most of the time don't use embedded OS'es, only when networking is required.

@miri64
Copy link
Member Author

miri64 commented Oct 22, 2015

I'm happy with the solution provided by xtimer. Since vtimer is basically not existent anymore I would prefer to close this issue. If you want to discuss more complex timer callbacks I would prefer to do this in a new issue.

@DipSwitch
Copy link
Member

Ok

@miri64 miri64 closed this as completed Oct 22, 2015
@OlegHahm
Copy link
Member

Did someone open a new issue? I think the problem of how to trigger a callback function when a timer expires does still exist in xtimer or am I mistaken?

@OlegHahm
Copy link
Member

I take this as a no.

@kaspar030
Copy link
Contributor

@OlegHahm Correct, same "problem" with xtimer. At least xtimer clearly documents the dangers of using in-ISR callbacks.

@cgundogan
Copy link
Member

I guess I never understood the question correctly. It is possible with xtimer (and was with vtimer) to listen to a message, receive this message and act appropriately to this message with a function call (callback).
Is the mere question rather how to abstract this receiving+acting upon a message into a separate kind of foo to lessen the boilerplate of programming an event loop?

@kaspar030
Copy link
Contributor

@cgundogan On other systems (POSIX...), it is possible to have asynchronous timer callbacks, e.g., set timer, continue work, and the timer's callback will do stuff without any waiting-for-message loop, possibly on another CPU, in the background.

This has been possible with hwtimer, vtimer and xtimer forever, but only with the callback firing in ISR context. This is not considered "safe" by some, e.g., those callbacks might not behave as expected and it's easy to break stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

No branches or pull requests

7 participants