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/03/26 15:19:53 UTC
[GitHub] [incubator-nuttx] Donny9 opened a new pull request #5853: driver/syslog: Add microseconds after date time
Donny9 opened a new pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853
## Summary
driver/syslog: Add microseconds after date time
Signed-off-by: Jiuzhu Dong <do...@xiaomi.com>
## Impact
Displays more detailed time information
## Testing
local test
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835897829
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
I think there is no easy solution for this. I will write few possible cases that comes in my mind, then we can discuss this based on opinions from other reviewers:
1. Add `clock_printf()` API that will work similar to `strftime()`, but will get `struct timespec` as an input and will implement additional print format (like `%f` in python variant of `strftime()`). The standard `strftime` can be reused for all parts except `%f` so probably API will be not too "heavy".
2. Remove `CONFIG_SYSLOG_TIMESTAMP_FORMAT` config option and hard-code format to `"%e/%m/%y %H:%M:%S"` and keep your variant with `[%s.%06ld]`.
3. Parse `CONFIG_SYSLOG_TIMESTAMP_FORMAT` here and identify where `%S` is placed and if it is present then generate new format string that will be `"%e/%m/%y %H:%M:%S.value"` like with already pre-printed value of microseconds.
4. other variant.
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835811925
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
`SYSLOG_TIMESTAMP_FORMATTED` means that it `Uses the standard "strftime" format specifiers.` and that is stated in Kconfig description so introduction of this change will violate the description.
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835897829
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
I think there is no easy solution for this. I will write few possible cases that comes in my mind, then we can discuss this based on opinions from other reviewers:
1. Add `clock_printf()` API that will work similar to `strftime()`, but will get `struct timespec` as an input and will implement additional print format (like `%f` in python variant of `strftime()`). The standard `strftime` can be reused for all parts except `%f` so probably API will be not too "heavy".
2. Remove `CONFIG_SYSLOG_TIMESTAMP_FORMAT` config option and hard-code format to `"%e/%m/%y %H:%M:%S"` and keep your variant with `[%s.%06ld]`.
3. Parse `CONFIG_SYSLOG_TIMESTAMP_FORMAT` here and identify where `%S` is placed and if it is present then generate new format string that will be `"%e/%m/%y %H:%M:%S.value"` with already pre-printed value of microseconds.
4. other variant.
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835936942
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
Yes. Option 1 seems to be the most straightforward. The `clock_printf()` will just reprint the initial format string and handle %f only while `strftime` will handle the rest.
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r836024367
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
Do you mean to add a general clock_printf API? my suggestion limit to the scope of syslog which mean:
1. Add SYSLOG_TIMESTAMP_FORMAT_MICROSECOND which depend on SYSLOG_TIMESTAMP_FORMAT
2. Guard the microsecond log by SYSLOG_TIMESTAMP_FORMAT_MICROSECOND
And SYSLOG_TIMESTAMP_FORMAT_MICROSECOND default to y since SYSLOG_TIMESTAMP_FORMAT is "%e/%m/%y %H:%M:%S" which is good to append microsecond.
--
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] jerpelea commented on a change in pull request #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
jerpelea commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r838175004
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
@Donny9 please add a new print option as suggested
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835811577
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
That is understood. I will make more clear example. By default `CONFIG_SYSLOG_TIMESTAMP_FORMAT` is set to `"%e/%m/%y %H:%M:%S"`, but what is user set's it to `"%e/%m/%y %H:%M"` and do not want to see a seconds (that is more theoretical than a practical case and I agree, but still), then it will be very strange to see dot and milliseconds value after month.
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835808410
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
The change ensure the output align with line 137 which output the fraction part unconditionally.
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835811798
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
Or even `CONFIG_SYSLOG_TIMESTAMP_FORMAT` can be changed to `"%H:%M:%S %e/%m/%y"` by displaying time first
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835788849
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
Should this be optional? I mean that user has full control of time stamp format that is printed via `CONFIG_SYSLOG_TIMESTAMP_FORMAT`, but this is not under control of the user.
--
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] Donny9 commented on a change in pull request #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835831188
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
@pkarashchenko
Does this mean adding microseconds members to the structure tm? It doesn't look good because TM is in UTC
Or check whether there is a string %S in FMT to append microseconds?
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r835932426
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
Let's add a new option?
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r836244158
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,8 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
- ret = lib_sprintf(&stream.public, "[%s] ", date_buf);
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
Yes. That also can be the case.
> Let's add a new option?
I read it as "let's add a new print option %f" :)
--
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 #5853: driver/syslog: Add microseconds after date time
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5853:
URL: https://github.com/apache/incubator-nuttx/pull/5853#discussion_r839361539
##########
File path: drivers/syslog/vsyslog.c
##########
@@ -130,7 +130,12 @@ int nx_vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap)
if (ret > 0)
{
+#if defined(CONFIG_SYSLOG_TIMESTAMP_FORMAT_MICROSECOND)
+ ret = lib_sprintf(&stream.public, "[%s.%06ld] ",
+ date_buf, ts.tv_nsec / 1000);
Review comment:
```suggestion
date_buf, ts.tv_nsec / NSEC_PER_USEC);
```
--
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