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/03 09:31:33 UTC
[GitHub] [incubator-nuttx] jlaitine opened a new pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
jlaitine opened a new pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779
…rocess
This fixes busylooping in work_usrthread, due to incorrect time spec given to sem_timedwait
_SEM_TIMEDWAIT works on absolute time stamps, using CLOCK_REALTIME
Signed-off-by: Jukka Laitinen <ju...@ssrc.tii.ae>
## Summary
The implementation of sem_timedwait has changed in 33ec242caf3
It calls sem_clockwait(sem, CLOCK_REALTIME, abstime), which expects absolute time stamp for wakeup, using CLOCK_REALTIME.
This patch provides the proper timespec in work_usrthread.
## Impact
This fixes busylooping in work_usrthread; it never sleeps in the semaphore.
## Testing
This has been tested on STM32F7 (Pixhawk 4 flight controller), using NuttX PROTECTED_BUILD.
--
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] jlaitine commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742773912
##########
File path: include/nuttx/clock.h
##########
@@ -394,6 +394,49 @@ void clock_resynchronize(FAR struct timespec *rtc_diff);
clock_t clock_systime_ticks(void);
#endif
+/****************************************************************************
+ * Name: clock_time2ticks
+ *
+ * Description:
+ * Return the given struct timespec as systime ticks
+ *
+ * NOTE: This is an internal OS interface and should not be called from
+ * application code.
+ *
+ * Input Parameters:
+ * reltime - Time presented as struct timespec
+ *
+ * Returned Value:
+ * Always returns OK (0)
+ *
+ ****************************************************************************/
+
+#if !defined(CONFIG_SCHED_TICKLESS)
+int clock_time2ticks(FAR const struct timespec *reltime,
+ FAR sclock_t *ticks);
+#endif
+
+/****************************************************************************
+ * Name: clock_ticks2time
+ *
+ * Description:
+ * Return the given systime ticks as a struct timespec
+ *
+ * NOTE: This is an internal OS interface and should not be called from
+ * application code.
+ *
+ * Input Parameters:
+ * ticks - Time presented as systime ticks
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742216471
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,24 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
Review comment:
not all platform support uint64_t
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961629697
> The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
>
> We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard functions.
>
> I think the fact that they are prototyped in nuttx/c;lock.h does at least make their non-standard nature clearer (but doesn't help with portability).
Thanks for your advice!
I am of course willing to re-work this if you can advice how to nicely implement the change in libs/libc/wqueue/work_usrthread.c
One obvious way is of course to duplicate the tick->timespec calculations locally in there (where I actually started at...). I was trying to avoid doing that, since it turns out to be a bit more complex than I first thought, considering all the different platforms, supported data-types etc.
--
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] jlaitine commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741969232
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
Thanks, agree on all points. Will fix this :)
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741945947
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\) (https://en.cppreference.com/w/c/chrono/timespec)
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742765082
##########
File path: include/nuttx/clock.h
##########
@@ -394,6 +394,49 @@ void clock_resynchronize(FAR struct timespec *rtc_diff);
clock_t clock_systime_ticks(void);
#endif
+/****************************************************************************
+ * Name: clock_time2ticks
+ *
+ * Description:
+ * Return the given struct timespec as systime ticks
+ *
+ * NOTE: This is an internal OS interface and should not be called from
+ * application code.
+ *
+ * Input Parameters:
+ * reltime - Time presented as struct timespec
+ *
+ * Returned Value:
+ * Always returns OK (0)
+ *
+ ****************************************************************************/
+
+#if !defined(CONFIG_SCHED_TICKLESS)
+int clock_time2ticks(FAR const struct timespec *reltime,
+ FAR sclock_t *ticks);
+#endif
+
+/****************************************************************************
+ * Name: clock_ticks2time
+ *
+ * Description:
+ * Return the given systime ticks as a struct timespec
Review comment:
```suggestion
* Return the given systime ticks as a struct timespec.
```
nit: Missing period.
--
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] jlaitine commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741969232
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
Thanks, agree on all points. Will fix this :)
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961464799
The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard functions.
I think the fact that they are prototyped in nuttx/c;lock.h does at least make their non-standard nature clearer (but doesn't help with portability).
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961464799
The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard 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] xiaoxiang781216 edited a comment on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961059326
It's better to merge the first and the third patch into one, other change looks good to me.
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-958898418
If there is a more reasonable way to just sleep N ticks in user space, please point that out; it would make much more sense to use something line nxsem_tickwait in here, but that is not available in user space...
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741945947
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\) (https://en.cppreference.com/w/c/chrono/timespec)
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\) (https://en.cppreference.com/w/c/chrono/timespec)
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742765745
##########
File path: include/nuttx/clock.h
##########
@@ -394,6 +394,49 @@ void clock_resynchronize(FAR struct timespec *rtc_diff);
clock_t clock_systime_ticks(void);
#endif
+/****************************************************************************
+ * Name: clock_time2ticks
+ *
+ * Description:
+ * Return the given struct timespec as systime ticks
+ *
+ * NOTE: This is an internal OS interface and should not be called from
+ * application code.
+ *
+ * Input Parameters:
+ * reltime - Time presented as struct timespec
+ *
+ * Returned Value:
+ * Always returns OK (0)
+ *
+ ****************************************************************************/
+
+#if !defined(CONFIG_SCHED_TICKLESS)
+int clock_time2ticks(FAR const struct timespec *reltime,
+ FAR sclock_t *ticks);
+#endif
+
+/****************************************************************************
+ * Name: clock_ticks2time
+ *
+ * Description:
+ * Return the given systime ticks as a struct timespec
+ *
+ * NOTE: This is an internal OS interface and should not be called from
+ * application code.
+ *
+ * Input Parameters:
+ * ticks - Time presented as systime ticks
Review comment:
```suggestion
* Input Parameters:
* ticks - Time presented as systime ticks
*
* Output Parameters:
* reltime - Pointer to receive the time value presented as struct timespec
```
It might be interesting to also add the documentation for the output parameters.
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742763165
##########
File path: include/nuttx/clock.h
##########
@@ -394,6 +394,49 @@ void clock_resynchronize(FAR struct timespec *rtc_diff);
clock_t clock_systime_ticks(void);
#endif
+/****************************************************************************
+ * Name: clock_time2ticks
+ *
+ * Description:
+ * Return the given struct timespec as systime ticks
Review comment:
```suggestion
* Return the given struct timespec as systime ticks.
```
nit: Missing period.
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961025238
argh. this doesn't of course build for KERNEL builds, since sched is not linked against libc there.
Would it make sense to move those timespec functions under libc/sched, as they are pure math?
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961464799
The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard 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] xiaoxiang781216 commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742216471
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,24 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
Review comment:
not all platform support uint64_t
--
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 merged pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961059326
It's better to merge the first and the third patch into one.
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742764962
##########
File path: include/nuttx/clock.h
##########
@@ -394,6 +394,49 @@ void clock_resynchronize(FAR struct timespec *rtc_diff);
clock_t clock_systime_ticks(void);
#endif
+/****************************************************************************
+ * Name: clock_time2ticks
+ *
+ * Description:
+ * Return the given struct timespec as systime ticks
+ *
+ * NOTE: This is an internal OS interface and should not be called from
+ * application code.
+ *
+ * Input Parameters:
+ * reltime - Time presented as struct timespec
Review comment:
```suggestion
* Input Parameters:
* reltime - Time presented as struct timespec
*
* Output Parameters:
* ticks - Pointer to receive the time value presented as systime ticks
```
It might be interesting to also add the documentation for the output parameters.
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961034405
@jlaitine the path at the head need update too:
```
home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_ticks2time.c:2:1: error: Relative file path does not match actual file
Error: /home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_time2ticks.c:2:1: error: Relative file path does not match actual file
Error: /home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_timespec_add.c:2:1: error: Relative file path does not match actual file
Error: /home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_timespec_subtract.c:2:1: error: Relative file path does not match actual file
```
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961629697
> The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
>
> We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard functions.
>
> I think the fact that they are prototyped in nuttx/c;lock.h does at least make their non-standard nature clearer (but doesn't help with portability).
Thanks for your advice!
I am of course willing to re-work this if you can advice how to nicely implement the change in libs/libc/wqueue/work_usrthread.c
One obvious way is of course to duplicate the tick->timespec calculations locally in there (where I actually started at...). I was trying to avoid doing that, since it turns out to be a bit more complex than I first thought, considering all the different platforms, supported data-types etc.
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961464799
The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard functions.
I think the fact that they are prototyped in nuttx/c;lock.h does at least make their non-standard nature clearer (but doesn't help with portability).
--
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 merged pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961464799
The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard functions.
I think the fact that they are prototyped in nuttx/c;lock.h does at least make their non-standard nature clearer (but doesn't help with portability).
--
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 a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742783125
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,24 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
Review comment:
I see the code has already changed, but just for reference: uint64_t is not available on all platforms supported by NuttX. Its use should be guarded by CONFIG_HAVE_LONG_LONG which gets defined in nuttx/compiler.h
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-958898418
If there is a more reasonable way to just sleep N ticks in user space, please point that out; it would make much more sense to use something line nxsem_tickwait in here, but that is not available in user space...
--
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] jlaitine commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742574943
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,24 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
Review comment:
yes, thanks. Let's just write this in cleaner way, by using clock timespec functions already provided by sched. No point in duplicating the logic in many places.
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-960676830
@xiaoxiang781216 @gustavonihei I wonder if this is an acceptable way to implement this?
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961464799
The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard 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] patacongo commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742783125
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,24 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
Review comment:
I see the code has already changed, but just for reference: uint64_t is not available on all platforms supported by NuttX. Its use should be guarded by CONFIG_HAVE_LONG_LONG which gets defined in nuttx/compiler.h
Also, uint64_t is unsafe for time calculations in some cases because accesses to 64-values are not atomic on all platforms and might need to be protected with sched_lock().
--
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 merged pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961034405
@jlaitine the path at the header need update too:
```
home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_ticks2time.c:2:1: error: Relative file path does not match actual file
Error: /home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_time2ticks.c:2:1: error: Relative file path does not match actual file
Error: /home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_timespec_add.c:2:1: error: Relative file path does not match actual file
Error: /home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/libs/libc/sched/clock_timespec_subtract.c:2:1: error: Relative file path does not match actual file
```
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961629697
> The only issue with moving these functions into libc is that it modifies the OS interface definition and violates the portable POSIX interface.
>
> We normally keep non-standard helper functions like under apps/ so avoid contaminating the OS interface.with non-standard functions.
>
> I think the fact that they are prototyped in nuttx/c;lock.h does at least make their non-standard nature clearer (but doesn't help with portability).
Thanks for your advice!
I am of course willing to re-work this if you can advice how to nicely implement the change in libs/libc/wqueue/work_usrthread.c
One obvious way is of course to duplicate the tick->timespec calculations locally in there (where I actually started at...). I was trying to avoid doing that, since it turns out to be a bit more complex than I first thought, considering all the different platforms, supported data-types etc.
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741945947
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\) (https://en.cppreference.com/w/c/chrono/timespec)
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741945947
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
--
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] jlaitine commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741969232
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
Thanks, agree on all points. Will fix this :)
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-958898418
If there is a more reasonable way to just sleep N ticks in user space, please point that out; it would make much more sense to use something line nxsem_tickwait in here, but that is not available in user space...
--
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 #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r742216471
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,24 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
Review comment:
not all platform support uint64_t
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,24 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
Review comment:
not all platform support uint64_t
--
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] gustavonihei commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741945947
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
```suggestion
clock_gettime(CLOCK_REALTIME, &rqtp);
nsec = TICK2NSEC(next);
sec = nsec / NSEC_PER_SEC;
rqtp.tv_sec += sec;
rqtp.tv_nsec += nsec - sec * NSEC_PER_SEC;
if (rqtp.tv_nsec >= NSEC_PER_SEC)
{
rqtp.tv_sec++;
rqtp.tv_nsec -= NSEC_PER_SEC;
}
```
Some considerations here:
1) Suggest using the nanoseconds macros for achieving cleaner code.
2) The cast to `uint32_t` might end up narrowing the value on 64 bits architectures, since `tv_nsec` type is `long`. I believe it can be removed.
3) After incrementing `tv_nsec`, we still need to check if the final value is within the valid range of \[0, 1000000000\] (https://en.cppreference.com/w/c/chrono/timespec)
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-958898418
If there is a more reasonable way to just sleep N ticks in user space, please point that out; it would make much more sense to use something line nxsem_tickwait in here, but that is not available in user space...
--
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] jlaitine commented on a change in pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#discussion_r741969232
##########
File path: libs/libc/wqueue/work_usrthread.c
##########
@@ -193,15 +193,18 @@ static void work_process(FAR struct usr_wqueue_s *wqueue)
{
struct timespec rqtp;
time_t sec;
+ uint64_t nsec;
/* Wait awhile to check the work list. We will wait here until
* either the time elapses or until we are awakened by a semaphore.
* Interrupts will be re-enabled while we wait.
*/
- sec = next / 1000000;
- rqtp.tv_sec = sec;
- rqtp.tv_nsec = (next - (sec * 1000000)) * 1000;
+ clock_gettime(CLOCK_REALTIME, &rqtp);
+ nsec = 1000 * TICK2USEC(next);
+ sec = nsec / (1000 * 1000 * 1000);
+ rqtp.tv_sec += sec;
+ rqtp.tv_nsec += (uint32_t)(nsec - sec * 1000 * 1000 * 1000);
Review comment:
Thanks, agree on all points. Will fix this :)
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961031794
I'll give it a try. A seemingly simple thing started to explode... but need to get protected and kernel build usr workqueues working properly ...
--
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] jlaitine commented on pull request #4779: libs/libc/wqueue/work_usrthread.c: Correct time calculation in work_p…
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #4779:
URL: https://github.com/apache/incubator-nuttx/pull/4779#issuecomment-961104744
> It's better to merge the first and the third patch into one, other change looks good to me.
Sure, will do that. Thanks!
--
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