You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "realprocrastinator (via GitHub)" <gi...@apache.org> on 2024/03/27 01:52:34 UTC
[PR] sched/wdog: remove unreachable branch [nuttx]
realprocrastinator opened a new pull request, #12005:
URL: https://github.com/apache/nuttx/pull/12005
## Summary
It seems that the `else if` branch would only execute if `++delay` would reults in an integer overflow. For example the `delay` is 0x7fff_ffff or 0x7fffffff_ffffffff according to the type of `sclock_t`. However in that case, `--delay` will revert the delay back to value before overflow. So IMO, the `if else` branch doesn't have any impact on the final result of delay if it is already a positive integer. Pls correct me if I was wrong :-)
## Impact
## Testing
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1540902772
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
The overflow will be reverted, but non overflow cases will not increment.
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1541121299
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
Notice the comment just before this code section:
```
/* Calculate delay+1, forcing the delay into a range that we can handle.
*
* NOTE that one is added to the delay. This is correct and must not be
* changed: The contract for the use wdog_start is that the wdog will
* delay FOR AT LEAST as long as requested, but may delay longer due to
* variety of factors. The wdog logic 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.
*
* There is extensive documentation about this time issue elsewhere.
*/
``
The increment in the delay is a required part of the timer logic and must be retained: Always increment the delay by unless there is an integer overflow, the back out the increment. That is correct and must not be changed. There is a complete explanation in the NuttX Confluence Wiki with the title "Short Time Delays. I can't reach that page now, but I will add the URL when I can.
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#issuecomment-2026914602
Closed per OP
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "realprocrastinator (via GitHub)" <gi...@apache.org>.
realprocrastinator commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1543944529
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
Yeah, you are right! I missed the condition that if `delay > 0`, the value must also be increased by one! Thx for the detailed explanation. @patacongo @pkarashchenko
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1541121299
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
Notice the comment just before this code section:
```
/* Calculate delay+1, forcing the delay into a range that we can handle.
*
* NOTE that one is added to the delay. This is correct and must not be
* changed: The contract for the use wdog_start is that the wdog will
* delay FOR AT LEAST as long as requested, but may delay longer due to
* variety of factors. The wdog logic 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.
*
* There is extensive documentation about this time issue elsewhere.
*/
```
The increment in the delay is a required part of the timer logic and must be retained: Always increment the delay by unless there is an integer overflow, the back out the increment. That is correct and must not be changed. There is a complete explanation in the NuttX Confluence Wiki with the title "Short Time Delays. I can't reach that page now, but I will add the URL when I can.
Here is the URL to that full explanation that I promised: https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "realprocrastinator (via GitHub)" <gi...@apache.org>.
realprocrastinator commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1544201148
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
Sure!
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "realprocrastinator (via GitHub)" <gi...@apache.org>.
realprocrastinator commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1543944529
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
Yeah, you are right! I missed the condition that if `delay > 0 && delay < INT_MAX`, the value must also be increased by one! Thx for the detailed explanation. @patacongo @pkarashchenko
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 closed pull request #12005: sched/wdog: remove unecessary branch
URL: https://github.com/apache/nuttx/pull/12005
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1541121299
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
Notice the comment just before this code section:
```
/* Calculate delay+1, forcing the delay into a range that we can handle.
*
* NOTE that one is added to the delay. This is correct and must not be
* changed: The contract for the use wdog_start is that the wdog will
* delay FOR AT LEAST as long as requested, but may delay longer due to
* variety of factors. The wdog logic 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.
*
* There is extensive documentation about this time issue elsewhere.
*/
```
The increment in the delay is a required part of the timer logic and must be retained: Always increment the delay by unless there is an integer overflow, the back out the increment. That is correct and must not be changed. There is a complete explanation in the NuttX Confluence Wiki with the title "Short Time Delays. I can't reach that page now, but I will add the URL when I can.
Here is the URL to that full explanation that I promised: https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays . This comes up often so this information should probably appear in the main documentation. I thought that a simple comment in-line would stop the many attempts to change this code but that seems to be insufficient.
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1544185324
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
so, could we close this pr 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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1540784167
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
@patacongo could you take a look?
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1540902772
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
The overflow will be subtracted back, so removal is fine for that, but non overflow cases will not increment.
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1540902772
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
The overflow will be subtracted back, but non overflow cases will not increment.
--
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
Re: [PR] sched/wdog: remove unecessary branch [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on code in PR #12005:
URL: https://github.com/apache/nuttx/pull/12005#discussion_r1541121299
##########
sched/wdog/wd_start.c:
##########
@@ -218,10 +218,6 @@ int wd_start(FAR struct wdog_s *wdog, sclock_t delay,
{
delay = 1;
}
- else if (++delay <= 0)
Review Comment:
Notice the comment just before this code section:
```
/* Calculate delay+1, forcing the delay into a range that we can handle.
*
* NOTE that one is added to the delay. This is correct and must not be
* changed: The contract for the use wdog_start is that the wdog will
* delay FOR AT LEAST as long as requested, but may delay longer due to
* variety of factors. The wdog logic 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.
*
* There is extensive documentation about this time issue elsewhere.
*/
```
The increment in the delay is a required part of the timer logic and must be retained: Always increment the delay by unless there is an integer overflow, the back out the increment. That is correct and must not be changed. There is a complete explanation in the NuttX Confluence Wiki with the title "Short Time Delays. I can't reach that page now, but I will add the URL when I can.
Here is the URL to that full explanation that I promised: https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays
This comes up often so this information should probably appear in the main documentation. I thought that a simple comment in-line would stop the many attempts to change this code but that seems to be insufficient.
--
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