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/07 07:49:31 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5432: sched/wdog: fix delay calculation in wd_start()
pkarashchenko opened a new pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432
## Summary
Do not add extra tick to a delay if wd_start() is called from system tick interrupt handler.
This is an alternative to a solution that is proposed in https://github.com/apache/incubator-nuttx/pull/5424
## Impact
`wdog` users that perform `wd_start()` call from `wdog` expiration callback
## Testing
Tested on SAME70-QMTECH board with logic analyzer. The application is based on with POSIX timers that toggles pin periodically.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031237159
> If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
Linux timer API even with the latest code still suffer the same issue as wdog since both use the tick as unit. You should add a new set API like hrtimer which use nanosecond unit: https://www.kernel.org/doc/html/latest/timers/hrtimers.html. And implement all other timer related api on top of hrtimer.
Since the unit of arch(up_alarm_xxx or up_timer_exx) API in tickless mode use nanosecond unit, it's very lucky that we don't need introduce the new arch API to support the high resolution timer.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800440874
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
This approach has two issues:
1. If some wdog callback run a bit long, the phase error will be large
2. Can't fix all possible place(e.g. other interrupt happen near the timer interrupt)
Since the accuracy of wdog is one tick, the change can't improve the accuracy smaller than one tick in general from the statistics perspective. But it improve the most common case, I am fine with the change.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800701698
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
> >
> > I can agree with 1, not with 2. Even if separate HW timer is used the other interrupt can happen near the timer interrupt. 2 is usually mitigated by assigning of a proper interrupt priority and enabling nested interrupts, so can't be a valid argument in this discussion.
>
> Why do you assume other interrupt happen near the timer interrupt? Many interrupts are trigger by the external activity, nobody can forecast when the activity will happen.
I'm assuming that it may happen, but external interrupts may happen near other MCU timer interrupt. The usage if a dedicated HW timer was suggested as a solution to have more accurate timers. What I'm trying to say that in terms of 2 there is no difference with sys tick interrupt
--
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] acassis commented on a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800604123
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
Since this modification fixes the common case, it should be enabled by default.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031690476
> > The implementation of of high resolution timers will probably need to rely on MCU HW timer and will be an optional feature.
>
> In that case, there will _have_ to be additional Kconfigs to select which HW timer provides this service.
No, the interface already exist, we don't need new one:
https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/arch.h#L1527-L1743
The above arch functions are enough to implement the high resolution timer.
Of course, you can add Kconfig in the chip level to select which hardware timer should be use to implement these functions.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1032478053
I will move the summary from this discussion into dev mailing thread, so there will be saved history.
Due to execution time of wdog handlers is not determined the actual timer delay may vary depending on a system load.
Despite solution proposed by me seems to be attractive from a common use case point of view it may lead to overall system degradation (undetermined behavior) in the systems with high load and high number of wdog based objects.
The only good approach is to implement a high resolution SW timers that are decoupled from wdog and then base POSIX timers and timerfd on high resolution timers in case if that is supported by the arch.
@patacongo @xiaoxiang781216 @acassis @hartmannathan thank you all for participating in the number of reviews and discussions. I really appreciate efforts spent to get the optimal solution shown up.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800701698
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
> >
> > I can agree with 1, not with 2. Even if separate HW timer is used the other interrupt can happen near the timer interrupt. 2 is usually mitigated by assigning of a proper interrupt priority and enabling nested interrupts, so can't be a valid argument in this discussion.
>
> Why do you assume other interrupt happen near the timer interrupt? Many interrupts are trigger by the external activity, nobody can forecast when the activity will happen.
I'm assuming that it may happen, but external interrupts may happen near other MCU timer interrupt. The usage of a dedicated HW timer was suggested as a solution to have more accurate timers. What I'm trying to say that in terms of 2 there is no difference between sys tick interrupt and dedicated HW timer interrupt.
And in case of 2 accuracy drop can be fixed by setting proper interrupt priority and enabling nested interrupts.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800762549
##########
File path: sched/wdog/wd_start.c
##########
@@ -74,6 +74,12 @@
# define CALL_FUNC(func, arg) func(arg)
#endif
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static bool g_wdexpiration_context;
Review comment:
Shouldn't it be volatile, also?
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
Review comment:
sclock_t is a signed value. Do we really want to use `==` here?
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
Review comment:
Maybe we need tri-state:
```
if (delay < 0)
{
...
}
else if (delay == 0)
{
...
}
else
{
...
}
```
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031579961
> The implementation of of high resolution timers will probably need to rely on MCU HW timer and will be an optional feature.
In that case, there will _have_ to be additional Kconfigs to select which HW timer provides this service. We can't just pick one because this is very arch- and chip- dependent, and some timers have special capabilities which the application may need. ARM has the SysTick timer which (if I understand correctly) doesn't have to be configured for 1 KHz operation, though that seems to be what everyone is doing. But other archs don't have such a timer and rely on peripheral timers on the SoC. Please don't misunderstand: I think it would be great to have a nicely integrated solution. I just think it will be an awful lot of work to get there. (Unless we leave it up to the board logic, but that is its own can of worms.)
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031266567
> > If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
>
> Linux timer API even with the latest code still suffer the same issue as wdog since both use the tick as unit. You should add a new set API like hrtimer which use nanosecond unit: https://www.kernel.org/doc/html/latest/timers/hrtimers.html. And implement all other timer related api on top of hrtimer.
> Since the unit of arch(up_alarm_xxx or up_timer_exx) API in tickless mode use nanosecond unit, it's very lucky that we don't need introduce the new arch API to support the high resolution timer.
>
The implementation of of high resolution timers will probably need to rely on MCU HW timer and will be an optional feature.
Please understand me right. I'm fine if proposed change is rejected by community and I'm just trying to find a way to achieve the best possible operation for timers functionality because believe that it is the right way of doing things.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800701698
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
> >
> > I can agree with 1, not with 2. Even if separate HW timer is used the other interrupt can happen near the timer interrupt. 2 is usually mitigated by assigning of a proper interrupt priority and enabling nested interrupts, so can't be a valid argument in this discussion.
>
> Why do you assume other interrupt happen near the timer interrupt? Many interrupts are trigger by the external activity, nobody can forecast when the activity will happen.
I'm assuming that it may happen, but external interrupts may happen near other MCU timer interrupt. The usage if a dedicated HW timer was suggested as a solution to have more accurate timers. What I'm trying to say that in terms of 2 there is no difference between sys tick interrupt and dedicated HW timer interrupt.
And in case of 2 accuracy drop can be fixed by setting proper interrupt priority and enabling nested interrupts.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko closed pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800682426
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
>
> I can agree with 1, not with 2. Even if separate HW timer is used the other interrupt can happen near the timer interrupt. 2 is usually mitigated by assigning of a proper interrupt priority and enabling nested interrupts, so can't be a valid argument in this discussion.
Why do you assume other interrupt happen near the timer interrupt? Many interrupts are trigger by the external activity, nobody can forecast when the activity will happen.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800672535
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
> Since this modification fixes the common case, it should be enabled by default.
I'm almost convinced to close both of my PRs and start implementation of high resolution timers ;)
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800671694
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
> This approach has two issues:
>
> 1. If some wdog callback run a bit long, the phase error will be large
> 2. Can't fix all possible place(e.g. other interrupt happen near the timer interrupt)
>
> Since the accuracy of wdog is one tick, the change can't improve the accuracy smaller than one tick in general from the statistics perspective. But it improve the most common case, I am fine with the change.
I can agree with 1, not with 2. Even if separate HW timer is used the other interrupt can happen near the timer interrupt. 2 is usually mitigated by assigning of a proper interrupt priority and enabling nested interrupts, so can't be a valid argument in this discussion.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031276071
> > > If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
> >
> >
> > Linux timer API even with the latest code still suffer the same issue as wdog since both use the tick as unit. You should add a new set API like hrtimer which use nanosecond unit: https://www.kernel.org/doc/html/latest/timers/hrtimers.html. And implement all other timer related api on top of hrtimer.
> > Since the unit of arch(up_alarm_xxx or up_timer_exx) API in tickless mode use nanosecond unit, it's very lucky that we don't need introduce the new arch API to support the high resolution timer.
>
> The implementation of of high resolution timers will probably need to rely on MCU HW timer and will be an optional feature.
>
What I mean high resolution is that the software timer(posix timer, timerfd) has the same accuracy as the hardware timer. So the developer can select the right hardware timer to implement arch_timer_xxx or arch_alarm_xxx API. The problem you hit is that wdog accuracy(one tick) is lower than the hardware capability.
> Please understand me right. I'm fine if proposed change is rejected by community and I'm just trying to find a way to achieve the best possible operation for timers functionality because believe that it is the right way of doing things.
The best way is what Linux has been done.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031237159
> If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
Linux timer API even with the latest code still suffer the same issue as wdog since both use the tick as unit. You should add a new set API like hrtimer which use nanosecond unit: https://www.kernel.org/doc/html/latest/timers/hrtimers.html. And implement all other timer related api on top of hrtimer. Since the unit of arch(up_alarm_xxx or up_timer_exx) API in tickless mode use nanosecond unit, it's very lucky that we don't need introduce the new arch API to support the high resolution timer.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031237159
> If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
Linux timer API even with the latest code still suffer the same issue as wdog since both use the tick as unit. You should add a new set API like hrtimer which use nanosecond unit: https://www.kernel.org/doc/html/latest/timers/hrtimers.html
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031166433
If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800450173
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
Maybe it is good to move this under a feature flag? So the user will enable this behavior explicitly. By default it will be not enabled.
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r801023271
##########
File path: sched/wdog/wd_start.c
##########
@@ -74,6 +74,12 @@
# define CALL_FUNC(func, arg) func(arg)
#endif
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static bool g_wdexpiration_context;
Review comment:
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 commented on a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800438531
##########
File path: sched/wdog/wd_start.c
##########
@@ -74,6 +74,12 @@
# define CALL_FUNC(func, arg) func(arg)
#endif
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static bool g_wdexpiration_context;
Review comment:
Change to g_in_wd_expiration?
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
This approach has two issues:
1. If some wdog callback run a bit long, the phase error will be large
2. Can't fix all possible place(e.g. other interrupt happen near the timer interrupt)
Since the accuracy of wdog is one tick, the change can't improve the accuracy smaller than one tick in general from the statistics perspective. But it improve in the most common case, I am fine with the change.
--
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] acassis commented on a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800599616
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
@pkarashchenko enabling it be default will bring benefits or drawbacks? I think we need to reduce the number of complex configurations inside NuttX, not create more configurations. The menu configuration "entropy" is becoming higher and higher...
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800701698
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
{
delay = 1;
}
- else if (++delay <= 0)
+ else
{
- delay--;
+ /* Calculate delay+1, forcing the delay into a range that we can
+ * handle. In general wd_start() can be called at any time and we do
+ * not know is it at the beginning in the middle or at the end of a
+ * system clock phase, so we need to add 1 tick to ensure that the
+ * operation will be delayed for at least delay number of ticks. But
+ * in a special case when wd_start() is called from a wd expiration
+ * callback that is called from a system tick interrupt handler we know
+ * that we are exactly at the beginning of a new phase so we do not
+ * need to add an extra tick in this case.
+ */
+
+ delay += g_wdexpiration_context ? 0 : 1;
Review comment:
> >
> > I can agree with 1, not with 2. Even if separate HW timer is used the other interrupt can happen near the timer interrupt. 2 is usually mitigated by assigning of a proper interrupt priority and enabling nested interrupts, so can't be a valid argument in this discussion.
>
> Why do you assume other interrupt happen near the timer interrupt? Many interrupts are trigger by the external activity, nobody can forecast when the activity will happen.
I'm assuming that it may happen, but external interrupts may happen near other MCU timer interrupt. The usage if a dedicated HW timer was suggested as a solution to have more accurate timers. What I'm trying to say that in terms of 2 there is no difference between sys tick interrupt and dedicated HW timer interrupt
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800978992
##########
File path: sched/wdog/wd_start.c
##########
@@ -204,15 +214,31 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
up_getpicbase(&wdog->picbase);
wdog->arg = arg;
- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Delay for at least one tick should be performed */
- if (delay <= 0)
+ if (delay == 0)
Review comment:
The negative case is check few lines above and the error code is returned, so no need to handle negative values here
--
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 a change in pull request #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#discussion_r800983282
##########
File path: sched/wdog/wd_start.c
##########
@@ -74,6 +74,12 @@
# define CALL_FUNC(func, arg) func(arg)
#endif
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static bool g_wdexpiration_context;
Review comment:
I will change to volatile, but since the wd_start is included indirectly via wd expiration callback the flag value will be checked anyway
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031276071
> > > If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
> >
> >
> > Linux timer API even with the latest code still suffer the same issue as wdog since both use the tick as unit. You should add a new set API like hrtimer which use nanosecond unit: https://www.kernel.org/doc/html/latest/timers/hrtimers.html. And implement all other timer related api on top of hrtimer.
> > Since the unit of arch(up_alarm_xxx or up_timer_exx) API in tickless mode use nanosecond unit, it's very lucky that we don't need introduce the new arch API to support the high resolution timer.
>
> The implementation of of high resolution timers will probably need to rely on MCU HW timer and will be an optional feature.
>
What I mean high resolution is that the software timer(posix timer, timerfd) has the same accuracy as the hardware timer. So the developer can select the right hardware timer to implement arch_timer_xxx or arch_alarm_xxx API. The problem you hit is that wdog accuracy(one tick) is lower than the hardware capability.
> Please understand me right. I'm fine if proposed change is rejected by community and I'm just trying to find a way to achieve the best possible operation for timers functionality because believe that it is the right way of doing things.
The best way is what Linux has been done, I think.
--
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 #5432: sched/wdog: fix delay calculation in wd_start()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5432:
URL: https://github.com/apache/incubator-nuttx/pull/5432#issuecomment-1031276071
> > > If this change will be rejected I will probably need to implement something like http://www.makelinux.net/ldd3/chp-7-sect-4.shtml and base POSIX timers and `timerfd` on top of it.
> >
> >
> > Linux timer API even with the latest code still suffer the same issue as wdog since both use the tick as unit. You should add a new set API like hrtimer which use nanosecond unit: https://www.kernel.org/doc/html/latest/timers/hrtimers.html. And implement all other timer related api on top of hrtimer.
> > Since the unit of arch(up_alarm_xxx or up_timer_exx) API in tickless mode use nanosecond unit, it's very lucky that we don't need introduce the new arch API to support the high resolution timer.
>
> The implementation of of high resolution timers will probably need to rely on MCU HW timer and will be an optional feature.
>
What I mean high resolution is that the software timer(posix timer, timerfd) has the same accuracy as the hardware timer. The problem you hit is that wdog accuracy(one tick) is lower than the hardware capability.
> Please understand me right. I'm fine if proposed change is rejected by community and I'm just trying to find a way to achieve the best possible operation for timers functionality because believe that it is the right way of doing things.
The best way is what Linux has been done.
--
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