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 2021/11/08 17:00:02 UTC
[GitHub] [incubator-nuttx] CV-Bowen opened a new pull request #4805: wdog/wd_start: remove the "delay+1" operation, which will cause the a…
CV-Bowen opened a new pull request #4805:
URL: https://github.com/apache/incubator-nuttx/pull/4805
wdog/wd_start.c: remove the "delay+1" operation, which will cause the actual delay time to be one tick longer than expected delay time.
## Summary
The sleep() time is not correct because of the incorrect delay calculation in wdog/wd_start : 192. The delay time will be one tick (system clock tick) longer than expected delay time.
## Impact
## Testing
Measure the sleep time after call sleep() using clock() function in linux simulation environment.
### Test code:
```c
int main(int argc, FAR char *argv[])
{
int count = 0;
clock_t clock1 = 0;
clock_t clock2 = 0;
printf("\n");
while(count < 20)
{
clock1 = clock();
sleep(1);
clock2 = clock();
printf("count:%d, clock1: %ld, clock2: %ld.\n", count, clock1, clock2);
count++;
}
printf("Test End!\n");
return 0;
}
```
### Test result before revision:
```shell
nsh> hello
count:0, clock1: 4294967254, clock2: 4294967355.
count:1, clock1: 4294967355, clock2: 4294967456.
count:2, clock1: 4294967456, clock2: 4294967557.
count:3, clock1: 4294967557, clock2: 4294967658.
count:4, clock1: 4294967658, clock2: 4294967759.
count:5, clock1: 4294967759, clock2: 4294967860.
count:6, clock1: 4294967860, clock2: 4294967961.
count:7, clock1: 4294967961, clock2: 4294968062.
count:8, clock1: 4294968062, clock2: 4294968163.
count:9, clock1: 4294968163, clock2: 4294968264.
count:10, clock1: 4294968264, clock2: 4294968365.
count:11, clock1: 4294968365, clock2: 4294968466.
count:12, clock1: 4294968466, clock2: 4294968567.
count:13, clock1: 4294968567, clock2: 4294968668.
count:14, clock1: 4294968668, clock2: 4294968769.
count:15, clock1: 4294968769, clock2: 4294968870.
count:16, clock1: 4294968870, clock2: 4294968971.
count:17, clock1: 4294968971, clock2: 4294969072.
count:18, clock1: 4294969072, clock2: 4294969173.
count:19, clock1: 4294969173, clock2: 4294969274.
Test End!
nsh>
```
We can see that the sleep time is always one tick (system clock tick) longer than expected sleep time.
### Test result after revision:
```shell
nsh> hello
count:0, clock1: 4294967334, clock2: 4294967434.
count:1, clock1: 4294967434, clock2: 4294967534.
count:2, clock1: 4294967534, clock2: 4294967634.
count:3, clock1: 4294967634, clock2: 4294967734.
count:4, clock1: 4294967734, clock2: 4294967834.
count:5, clock1: 4294967834, clock2: 4294967934.
count:6, clock1: 4294967934, clock2: 4294968034.
count:7, clock1: 4294968034, clock2: 4294968134.
count:8, clock1: 4294968134, clock2: 4294968234.
count:9, clock1: 4294968234, clock2: 4294968334.
count:10, clock1: 4294968334, clock2: 4294968434.
count:11, clock1: 4294968434, clock2: 4294968534.
count:12, clock1: 4294968534, clock2: 4294968634.
count:13, clock1: 4294968634, clock2: 4294968734.
count:14, clock1: 4294968734, clock2: 4294968834.
count:15, clock1: 4294968834, clock2: 4294968934.
count:16, clock1: 4294968934, clock2: 4294969034.
count:17, clock1: 4294969034, clock2: 4294969134.
count:18, clock1: 4294969134, clock2: 4294969234.
count:19, clock1: 4294969234, clock2: 4294969334.
Test End!
nsh>
```
The sleep time is correct.
--
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 #4805: wdog/wd_start: remove the "delay+1" operation, which will cause the a…
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4805:
URL: https://github.com/apache/incubator-nuttx/pull/4805#issuecomment-963466532
This must not be merged. This existing code is correct; this change is incorrect and will result in delays shorter than requested which must never be permitted to happen. Refer to https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays for an explanation.
This change is submitted every few weeks and is always wrong and is always rejected. I will close it 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
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #4805: wdog/wd_start: remove the "delay+1" operation, which will cause the a…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4805:
URL: https://github.com/apache/incubator-nuttx/pull/4805#issuecomment-963466532
This must not be merged. This existing code is correct; this change is incorrect and will result in delays shorter than requested which must never be permitted to happen. Refer to https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays for an explanation.
Basically, the user error occurs because they set up artificial tests where the timer is started synchronously with the system timer. In this artificial situation, yes, the timer will be one tick too long. However, in the real world case where the phase of the system timer is unknown, this change is known to break the timer.
This change is submitted every few weeks and is always wrong and is always rejected. I don't know what else we can do to stop this changes. This is probably the 5th or 6th time this incorrect change has been proposed. I will 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
[GitHub] [incubator-nuttx] patacongo closed pull request #4805: wdog/wd_start: remove the "delay+1" operation, which will cause the a…
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #4805:
URL: https://github.com/apache/incubator-nuttx/pull/4805
--
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 edited a comment on pull request #4805: wdog/wd_start: remove the "delay+1" operation, which will cause the a…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4805:
URL: https://github.com/apache/incubator-nuttx/pull/4805#issuecomment-963466532
This must not be merged. This existing code is correct; this change is incorrect and will result in delays shorter than requested which must never be permitted to happen. Refer to https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays for an explanation.
Basically, the user error occurs because they set up artificial tests where the timer is started synchronously with the system timer. In this artificial situation, yes, the timer will be one tick too long. However, in the real world case where the phase of the system timer is unknown, this change is known to break the timer.
This test is *invalid*:
while(count < 20)
{
clock1 = clock();
sleep(1);
clock2 = clock();
printf("count:%d, clock1: %ld, clock2: %ld.\n", count, clock1, clock2);
count++;
}
because it is fully synchronous with the system timer.
This change is submitted every few weeks and is always wrong and is always rejected. I don't know what else we can do to stop this changes. This is probably the 5th or 6th time this incorrect change has been proposed. I will 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
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #4805: wdog/wd_start: remove the "delay+1" operation, which will cause the a…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4805:
URL: https://github.com/apache/incubator-nuttx/pull/4805#issuecomment-963466532
This must not be merged. This existing code is correct; this change is incorrect and will result in delays shorter than requested which must never be permitted to happen. Refer to https://cwiki.apache.org/confluence/display/NUTTX/Short+Time+Delays for an explanation.
Basically, the user error occurs because they set up artificial tests where the timer is started synchronously with the system timer. In this artificial situation, yes, the timer will be one tick too long. However, in the real world case where the phase of the system timer is unknown, this change is known to break the timer.
This test is *invalid*:
clock1 = clock();
sleep(1);
clock2 = clock();
because it is fully synchronous with the system timer.
This change is submitted every few weeks and is always wrong and is always rejected. I don't know what else we can do to stop this changes. This is probably the 5th or 6th time this incorrect change has been proposed. I will 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