sys/xtimer: code simplification (removing long_list, using xtimer_remove, removing redundant expiry check)#13103
Conversation
| timer_list_head->start_time = (uint32_t)now; | ||
| timer_list_head->long_start_time = (uint32_t)(now >> 32); | ||
| } | ||
| } |
There was a problem hiding this comment.
_update_timers() already did timer expiry check and it does not have to be re-done here.
| timer->offset = 0; | ||
| timer->start_time = 0; | ||
| timer->long_start_time = 0; | ||
| timer->next = NULL; |
There was a problem hiding this comment.
A few lines above are actually what xtimer_remove() can do.
| volatile uint64_t _xtimer_current_time = 0; | ||
|
|
||
| static xtimer_t *timer_list_head = NULL; | ||
| static xtimer_t *long_list_head = NULL; |
There was a problem hiding this comment.
does not need to manage a separate long_list. One timer list is actually enough.
|
@Hyungsin is this only cleanup? |
|
@Hyungsin also in general all this comments you make in the PR are great but it would be even better to have them in the commit message. |
Yes. I think so. |
…ove, removing redundant expiry check)
Hyungsin
force-pushed
the
forupstream_xtimer_simplification
branch
from
232b06d to
6ccdedd
Compare
Hyungsin
changed the title
Added details on commit msg. |
smlng
added
Area: timers
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
|
Huh? Nobody reviewing this in 7 month? @MichelRottleuthner can you have a look? |
|
ping @MichelRottleuthner! |
|
The changes make sense but as with all timer related things this needs solid testing to not break something. Sadly I cannot allocate time for this currently. |
stale
bot
removed
the
State: stale
If testing is what is missing do you mind adding the other review flags :) |
MichelRottleuthner
added
Reviewed: 1-fundamentals
|
I ran my benchmark on current master (bfb8fc5) and on this PR after a rebase to the same master. The benchmark test is largely based on the There are some conflict when rebasing, so I just accept all incoming changes. The diff between the rebased PR and master (from the rebased branch One my test (sleep jitter) timed out on the rebased PR. The test have a periodic timer triggering every 100ms with a number of timer triggering randomly in the background. At 10 background timer, it is still okay but at 20 the tests takes too long (more than 45 seconds) and timed out. I didn't investigate further into as to what causes this. |
|
Please also check for the bug fixed in #13850. The test introduced there should cover that, but IMHO it doesn't hurt (except time spent) to also re-test the bug described there under "Issues/PRs references". |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
6 participants
Contributions
@MichelRottleuthner Remove redundant part of xtimer_core.c (e.g., long timer list). I put some comments on the code.
Testing procedure
Issues/PRs references