You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/02/05 16:54:37 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5421: sched/wdog: fix adding one extra tick to any delay
pkarashchenko opened a new pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421
## Summary
This changes tremendously improve Real-Time performance of the OS and apps since till now an extra systick was added by wd_start() and that lead to wrong timing in the system for all functionality that relies on wdog
## Impact
All over the system and apps
## Testing
Tested on SAME70-QMTECH board that toggles GPIO periodically. The pulse width and period are right as expected
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030696886
Following up in https://github.com/apache/incubator-nuttx/pull/5424
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030709045
Yes. I got the idea. I'm familiar with time drift compensation techniques. But the case that I'm trying to fix is next. I'm having a system with 10ms systick. I want to organize a periodic event (for example send a message) with 30ms. To use this I'm arming periodic POSIX timer with 30ms period and have a task that is waiting for a signal published by POSIX timer. When signal is received, the message is sent. I assume that I can achieve better or less accuracy by setting task priority depending on the load of my system. But what I'm getting is a message with 40ms period not depending from task priority. I do not see any reasons for that, but that is how currently it works. So this is not a sleep() use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030864343
>
You are welcome to participate in discussion https://github.com/apache/incubator-nuttx/pull/5424
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030686006
I did find the last time this came up. Here https://github.com/apache/incubator-nuttx/pull/4805, #4805
Please read the comments there too. There many other discussions in the old Google and Yahoo! groups as well. Like https://groups.google.com/g/nuttx/c/iMvPKunE2VA/m/YAOmm_FsBgAJ , https://nuttx.yahoogroups.narkive.com/v1gmmM8U/inaccurate-watchdog-delay , to mention a couple more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030690182
Adding one or not has nothing to do with accuracy. The accuracy will be the same in either case. The only thing that changes in the mean error. If you don't add one, the error will lie in -1 to 0 ticks (which can be fatal). If you do add one, the error will lie within 0 and +1 ticks. Same accuracy.
timerfd is newer and perhaps has an error. I am unfamiliar with the design. wdog is 100% correct, if there is a problem with POSIX timers or timerfd, they should be fixed. wdogs should not be changed due to errors in other timers.
Here is a two sentence summary that could be used as a comment: "The contract with the use is that the wdog will delay FOR AT LEAST as long as requested, but may delay long due to variety of factors. The wdog has no knowledge of the the phase of the system timer when it is started: The next timer interrupt may occur immediately or may be delayed for almost a full cycle. In order to meet the contract requirement, the requested time is also always incremented by one so that the delay is always at least as long as requested."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030687570
@gregory-nutt please send me the link to explanation and I will insert proper comment into the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030687570
@gregory-nutt please send me the link to explanation and I will insert it into the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko closed pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko closed pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030681140
This is an incorrect change and has has been rejected numerous times. It is a consequence of faulty testing. There is an good explanation of why this change is incorrect here: https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays
This has to be at least the 10th time this change has been suggested and rejected. The most recent was just a couple of months ago. I will try to find it for your.
If this change were merged, it might improve timing in your faulty test case, but will introduce fatal timing errors in other cases.
THIS MUST NOT BE MERGED!!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030682289
I can't find the most recent discussions. I can't remember if they were email discussions or PR discussions. There are many other discussions with people are confused themselves into trying to remove that +1 which is just totally wrong.
Perhaps we should put some comments there to prevent this from happening yet again in the future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030691563
Yes. The operation of wdog and delays is fully understood and I 100% agree with that. The timerfd was introduced recently, but they are implemented 90% the same a POSIX timers and POSIX timers rely on wdog so currently have accumulative if periodic timers are used. I think that is bad and should be fixed. The first expiration of a POSIX timers can have +1 tick, but all subsequent should not. I will try to see how that can be fixed. Maybe we can add extra path from systick interrupt and introduce separate timer recalculation path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030692795
Probably the fast fix is to subtract 1 tick from delay is timer reoccurrence handler :)
Seems to be very simple and elegant and should give desired behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030707769
> That is crappy code (any maybe not even correct), ...
It is certainly not correct. What we want is an error based on the time that we wanted and the time that we got, then to apply the error to the delay. Maybe this is closer:
```
clock_t last_time = current_time;
for (; ; )
{
clock_t next_time = last_time + x;
clock_t current_time = clock_systime_ticks();
int error= next_time - current_time;
last_time = next_time;
delay(x + error);
}
```
So after x ticks elapse, and current time is one more the expected time, then the next delay would be x - 1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030698632
When I said that the tests that most people run are "faulty", the usual problem is that they are starting the watching timer in phase with the system time, essentially at phase 0. That is not a random start time, but is a fixed worst case start time and results in the largest error each time. That is a consequence of the faulty test. If the wdog is started at random times, you would see that the behavior is more or less perfect.
The way to avoid accumulating errors from wdog times (which have inherent time errors anyway) is to use the current time to determine that time of the next tick.
For example, don't do the moral equivalent of:
```
for (; ;)
{
delay(x);
}
```
That might causes a loop with a period of x+1 on a system that not busy. It would then run synchronous with the system time in that case ("faulty"). On a busy system, the period could be nearly random, which is maybe worse.
But if you do something logically equivalent to:
```
clock_t last_time = current_time;
for (; ; )
{
clock_t next_time = last_time + x;
clock_t current_time = clock_systime_ticks();
int ticks = next_time - current_time;
last_time = next_time;
delay(ticks);
}
```
That is crappy code (any maybe not even correct), but you get this idea. You track the time that you want using the current time error for corrections. This may result in with a potentially variable number of ticks. And the time will never drift.
If the delay is one tick to large, it will be one tick smaller next time. This should stabilize quickly to a good periodic timer and is better resistant to system load.
You can even do some dithering to get effectively higher resolution. For example, I could generate a period event that is 2.5 x the system timer (although each period could differ by one tick).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030709045
Yes. I got the idea. I'm familiar with time drift compensation techniques. But the case that I'm trying to fix is next. I'm having a system with 10ms systick. I want to organize a periodic event (for example send a message) with 30ms. To use this I'm arming periodic POSIX timer with 30ms period and have a task that is waiting for a signal published by POSIX timer. When signal is received, the message is sent. I assume that I can achieve better or less accuracy by setting task priority depending on the load of my system. But what I'm getting is a message with 40ms period not depending from task priority. I do not see any reasons for that, but that is how currently it works. So this is not a sleep() use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko removed a comment on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko removed a comment on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030687570
@gregory-nutt please send me the link to explanation and I will insert proper comment into the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030864343
@hartmannathan You are welcome to participate in discussion https://github.com/apache/incubator-nuttx/pull/5424
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030682525
BTW, you can't get the more accuracy than one tick.. Sleep one tick is between 0-1 tick with your patch, Sleep one tick is between 1-2 tick without your patch. So there isn't much difference from the accuracy perspective. But, most people expect the real sleep should be longer than request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030686625
> I can't find the most recent discussions. I can't remember if they were email discussions or PR discussions. There are many other discussions with people are confused themselves into trying to remove that +1 which is just totally wrong.
>
> Perhaps we should put some comments there to prevent this from happening yet again in the future?
Yes. Comment would be fine. But without this change the POSIX timers and timerfd are running with errors and that also seems not to be fine. I agree that things like sleep or delay may have +1 (actually +N) accuracy, but I do not expect timers to have so bad accuracy especially for periodic timers. Let's not merge this change and I will think how timers can be fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030682525
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #5421: sched/wdog: fix adding one extra tick to any delay
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5421:
URL: https://github.com/apache/incubator-nuttx/pull/5421#issuecomment-1030863289
I'm late to the party but I just wanted to add that I was also confused by the + 1 tick in the past and I remember writing to ask about it. I think it might be in the old Google Group. I don't have the link handy at this time. What Greg is describing above sounds a little like a Bresenham-style algorithm. It works for drawing slanted lines on a graphic display but also works for things like fitting a time intervals that don't divide easily. In my case, it turned out that I needed a much higher resolution timer and for that I ended up using one of the chip's timer peripherals to produce an interrupt (zero-latency in my application). I recognize that that is not always an option but may be something to consider. Anyway this PR is closed so this is a moot point now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org