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/06/04 01:32:11 UTC
[GitHub] [incubator-nuttx] fjpanag opened a new pull request, #6363: Added color reset when a syslog channel is added.
fjpanag opened a new pull request, #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363
## Summary
When `CONFIG_SYSLOG_COLOR_OUTPUT` is enabled, the terminal color is set before the message is printed, and it is reset at the end of the print.
It can happen that the log output may be interrupted (e.g. system crash, external reset etc). In this case, the color reset escape sequence will not be sent. On the next boot, all output on the terminal is presented on the wrong color (the terminal remembers the last setting, while the firmware assumes that everything is in a clean state).
This fix resets the terminal color every time a syslog channel is added, so even the very first print will be correct.
## Impact
Handles the case where a syslog message is printed with an incorrect color.
## Testing
I logged the bytes sent from the logger, and indeed the escape sequence is sent the moment the channel is added.
--
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 diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889697609
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
Review Comment:
I'm not sure if we need this change, as seems that only the first message will be printed in a wrong color or if no syslog messages are in place (very unlikely) then the terminal will remain in wrong color.
--
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 diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889697609
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
Review Comment:
I'm not sure if we need this change, as seems that only the first message will be printed in a wrong color or if no syslog messages (very unlikely) are in place then the terminal will remain in wrong color.
--
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 #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#issuecomment-1170365975
The 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] pkarashchenko commented on a diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889697224
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
Review Comment:
The BL might not use color output at all. I mean that we can't handle all the cases. I'm not sure even that Linux and other Unix systems does. For example recently I've been using Windows + cmake in terminal and when cmake compilation failed the terminal in Windows was left at the color of cmake stage that failed. I do not see any was how we can handle all the corner cases.
--
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 #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363
--
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] fjpanag commented on a diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889684150
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
Review Comment:
This is one possible solution, but it will always leave the terminal configured on the last used color, instead of returning it to normal like it does now.
Is this desirable?
I think this will cause unwanted side-effects.
Examples:
* Using the same serial port both for the logger and for NSH.
* When restarting the system, a bootloader may need to use the serial output before NuttX starts (and resets the terminal).
Another solution is to add the color sequence both at the start and the end of the print, but it seems a bit excessive.
--
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] fjpanag commented on a diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889521983
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
Review Comment:
done
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
+
+ while (*col_rst)
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889493834
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
Review Comment:
what about the default channel?
--
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] fjpanag commented on a diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889521805
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
Review Comment:
Indeed, I didn't think about this.
This is not very straight-forward, since the default channel is statically initialized.
How do you propose to handle 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] fjpanag commented on pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#issuecomment-1169925719
@xiaoxiang781216 @pkarashchenko please have a look on this proposition.
I think that this is the "best" solution, handling most of the edge cases.
So, formatting is applied before any print, once for the date and other prefixes and if needed for the message itself.
When printing is complete, the color is reset.
This way printing is not affected by any previous colors applied, and it leaves the terminal in a normal state.
--
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 diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889477026
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
+
+ while (*col_rst)
Review Comment:
```suggestion
while (*col_rst != '\0')
```
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
Review Comment:
```suggestion
FAR char * col_rst = "\e[0m";
```
--
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 diff in pull request #6363: Added color reset when a syslog channel is added.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6363:
URL: https://github.com/apache/incubator-nuttx/pull/6363#discussion_r889544850
##########
drivers/syslog/syslog_channel.c:
##########
@@ -259,13 +259,26 @@ int syslog_channel(FAR struct syslog_channel_s *channel)
int i;
#endif
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ char * col_rst = "\e[0m";
+#endif
+
DEBUGASSERT(channel != NULL);
if (channel != NULL)
{
DEBUGASSERT(channel->sc_ops->sc_putc != NULL &&
channel->sc_ops->sc_force != NULL);
+#if defined(CONFIG_SYSLOG_COLOR_OUTPUT)
+ /* Reset the terminal style before using it. */
Review Comment:
Can we move https://github.com/apache/incubator-nuttx/blob/master/drivers/syslog/vsyslog.c#L218-L222 to the beginning?
--
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