Various Improvements and Fixes for etimer & timer

[bkozak-scanimetrics]

This patch makes the following changes to the etimer and timer modules

1. Fixes several style guide violations in etimer
2. Make etimer use a sorted list internally in order to improve efficiency of finding expired timers and calculating etimer_next_expiration_time. This will greatly cut down on the number of times that the linked list is fully traversed.
3. Adds a function to timer to compare the expiry times of timers
4. Adds a function to etimer to return the timer which is next to expire rather than just the expiry time*
5. Various other small changes

*This is better than just returning the expiry time as does etimer_next_expiration_time because the timer structures carry more information than just the expiry time and because the existing timer module functions can be used to analyse the structure. For example, we might call timer_expired to find out if the time lies in the past.

[adamdunkels]

The reason why the next_expiration variable exists, instead of dynamically computing it by traversing the list, is to allow it to be read from an interrupt context. This has been intended to be used by implementations of the clock interface that schedules clock interrupts based on when etimers are expected to fire ("tickless" clock code). The list of etimers must not be traversed from an interrupt because the interrupt may occur at a time when the pointers are not consistent (and may cause e.g. an infinite loop).

I haven't fully checked the code in this pull request, but I'd just like to check that the above reasoning still holds with the changes in this PR?

[bkozak-scanimetrics]

... is to allow it to be read from an interrupt context.

Now that you mention it, there is one case where there would be a problem when calling etimer_next_expiration_time from an ISR. If the next-to-expire etimer is set, reset, adjusted etc. before it is expired then there is a possible race condition.

I think I can fix this without adding next_to_expire back in.

[bkozak-scanimetrics]

I just added the variable back in. Seems to be possible to get rid of next_expiration and still be ISR safe but it's too much work to really do it right IMO.

[cdealti]

@bkozak-scanimetrics I was about to propose a quick and dirty fix on how next_expiration is computed in current code (the expiration time of timers may be actually in the past when timers are compared). Your refactor seems to fix this potential issue in a very elegant way.

@adamdunkels how to detect that the value returned by etimer_next_expiration_time is in the future? I assume you are referring to a "timer compare" clock implementation where the hardware timer is set to issue an interrupt when the counter matches the value returned by the above function.
However this cannot be done in the ISR of the clock implementation because the value of next_expiration will be updated asynchronously by the etimer_process so it's not yet available in the ISR to set the new value of the compare register. This could be done later but, due to wraps of the clock, how can we detect if the next expiration time is still in the future?
The existing usages of etimer_next_expiration_time in the codebase show some weird usages of this function. Some of them are probably incorrect ignoring the possibility of a clock wrap, some I cannot quite understand.

[bkozak-scanimetrics]

Regarding the Travis failure, I just noticed it now. Not sure what's going on because the Travis build on my own account passed without any errors. Re-based to see if it fixes it.

[bkozak-scanimetrics]

the expiration time of timers may be actually in the past when timers are compared

Yes actually I did notice this; I think I forgot to mention it in any commit message unfortunately. Thanks for pointing it out.

how to detect that the value returned by etimer_next_expiration_time is in the future?
... However this cannot be done in the ISR of the clock implementation because the value of next_expiration will be updated asynchronously ...
... Some of them are probably incorrect ignoring the possibility of a clock wrap, some I cannot quite understand.

Just want to say that I completely agree with you here. Part of the motivation of this PR was to provide the etimer_next_to_expire function so that it is easier to tell if the next to expire etimer has already expired (just use timer_expired).

[bkozak-scanimetrics]

Ok my own Travis build passed again. Hopefully we won't get the same problem on this build.

[bkozak-scanimetrics]

I've performed a re-base. Although there were no merge conflicts, Travis is failing since one of the 6502 builds runs out of memory now.

On the CC2650, this PR increases overall code size by about 90 bytes. As has been discussed on this page, this patch adds significant performance improvements, bug fixes and functionality so I think that a 90 byte flash cost is more than justified.

I don't think that it makes sense for me to try and adjust this PR just to work around the limitations of the 6502 and its compiler. The 6502 build complains that it needs at least another 286 bytes of memory and I don't think that there is any reasonable way to make this change while still keeping all of the improvements from this PR unless separate conditionally compiled code is made just for the 6502. Furthermore, from what I have seen, attempts to optimize code for better memory consumption on the 6502 often result in worse code on other platforms; for example, one might manually inline a small (or once-called) function or make a local variable into a static variable in order to get a few extra bytes on the 6502. Finally, if anyone is going to optimize this code for the 6502 it won't be me since I have neither a machine to test on nor any interest in the platform.

Considering all this, I request that the maintainers either find a way to reduce memory consumption for the 6502 in a different part of the code or consider dropping 6502 support altogether rather than drop this PR and all future PRs which affect the 6502 in a similar way. Otherwise, if anyone wants to try and modify this patch so that it will work with the 6502 once again, I'd appreciate it.

[greg-king5]

The list is always sorted; and, it is searched linearly. Therefore, the first timer test is redundant; it stays true until after the second test has found the insertion point. Also, appending to the list is the same as inserting in front of the list's NULL.

The above 10 lines can be replaced by this code:

    if(etimer_lte(timer, t_this, now)) {
      break;
    }
    t_last = t_this;
  }
  t_last->next = timer;
  timer->next = t_this;

[bkozak-scanimetrics]

@greg-king5 thanks for the review. I believe you're right and have added your suggested changes.

I've already tested the code on my own Travis account and although everything else passes the 6502 build will still fail.

[bkozak-scanimetrics]

@greg-king5 has been kind enough to offer a patch to reduce the 6502's code usage. When testing on my own Travis account, this was enough for the 6502 test case to pass.

I don't claim to understand exactly why this works so see the commit message of 30bf4d1 for details.

[bkozak-scanimetrics]

I have a desire to allow for the complete elimination of sys tick interrupts. My primary motivation is to allow for a higher resolution of clock_time_t without suffering an undue performance loss.

This can be done quite easily (I think) by adding support for an API call to set an RTC interrupt from the etimer module similar to how rtimer works.

Since this PR contains some fairly major structural changes for etimer, however, I'd like to see this merged (or otherwise closed if there's a good reason to drop it) before I release any new work on etimer.

Additionally I've been using this PR for months now with no issues so I think that it should me merged unless someone has a competing patch that does the job better.

Could some maintainer who is responsible for Core (I assume that @adamdunkels is probably busy lately) please have a look at this?

[joakimeriksson]

I agree fully with your intent of getting rid of interrupt driven ticks. I will try to take some time within a few weeks and test and do a code review of this (if no one have done it before).

[bkozak-scanimetrics]

Thanks @joakimeriksson I very much appreciate it.

[mguc]

I tried the above patch since I was experiencing an issue with the CSMA where the queue was filled up and never emptied due to messed up timers. This happened randomly after hardware interrupts and I observed that the etimer_next_expiration_time is not updated after that. I added some debugging output to clock_arch_time_to_etimer in the clock module of the platform I'm using (JN516x).

clock_time_t
clock_arch_time_to_etimer(void)
{
  clock_time_t time_to_etimer, next_expiration_time, current_clock_time;
  if(etimer_pending()) {
    next_expiration_time = etimer_next_expiration_time();
    current_clock_time = clock_time();
    if(next_expiration_time < current_clock_time){
        printf("next_expiration_time: %u, current_clock_time %u\n", next_expiration_time, current_clock_time);
    }
    time_to_etimer = next_expiration_time - current_clock_time;
    if(time_to_etimer < 0) {
      time_to_etimer = 0;
    }
  } else {
    /* no active etimers */
    time_to_etimer = (clock_time_t)-1;
  }
  return time_to_etimer;
}

After a while I see the following:

next_expiration_time: 323868, current_clock_time 701750
next_expiration_time: 323868, current_clock_time 701751
next_expiration_time: 323868, current_clock_time 701752
next_expiration_time: 323868, current_clock_time 701752
next_expiration_time: 323868, current_clock_time 701753

I'm not sure if this issue is just related to the JN516x platform since there was a big change in the clock module. However before and after this patch the issue still persists.

[atiselsts]

Hi mguc, the code is clearly wrong, as time_to_etimer is unsigned: if(time_to_etimer < 0) { Can you check changing the line to this helps? if((int32_t)time_to_etimer < 0) { Looks like this error was introduced when integer types were converted to clock_time_t.

[mguc]

You are absolutely right. Your suggestion fixed the issue for me. I will make a PR for the JN516x platform concerning this problem.

[bkozak-scanimetrics]

I've made some changes to reduce code size on 6502 but Travis failed on the large-rpl build. Large RPL passed on my own account so it must be that same issue where the build randomly fails.

Would someone be so kind as to restart it for me?

[alignan]

⌛