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/03/29 12:48:45 UTC
[GitHub] [incubator-nuttx] fjpanag opened a new pull request #3226: syslog: Added multi device support in syslog_device.
fjpanag opened a new pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226
## Summary
Added support for multiple devices in syslog_device.
Continuing my work from PR #3050, we can now have multiple instances of a syslog_device, so the different channels can co-exist correctly.
This opens up the way for future development like adding/removing channels dynamically, implementing different thread locking of the syslog etc etc.
## Impact
It shouldn't have any impact on any existing configuration.
## Testing
For the time being I have only done some minimal testing (mostly that it builds OK).
I plan to do more testing (and will confirm bellow in a comment).
The PR is open however, to allow for reviewing while I 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603942796
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
@xiaoxiang781216 If I understand this correctly, `struct syslog_dev_s` should be moved to `include/nuttx/syslog/syslog.h`.
And then `struct syslog_channel_s` will be something like:
```c
struct syslog_channel_s
{
/* Channel device */
FAR struct syslog_dev_s *sc_dev;
/* Channel operations */
FAR struct syslog_channel_ops_s *ops;
/* Implementation specific logic may follow */
};
```
Correct?
Or should `sc_dev` remain a `void *`, and keep `struct syslog_dev_s *sc_dev` private?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r604172827
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,44 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)
+ (FAR const struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+typedef CODE int (*syslog_putc_t)
+ (FAR const struct syslog_channel_s *channel,
+ int ch);
+typedef CODE int (*syslog_flush_t)
+ (FAR const struct syslog_channel_s *channel);
+
+/* SYSLOG device operations */
+struct syslog_channel_ops_s
+{
syslog_putc_t sc_putc; /* Normal buffered output */
syslog_putc_t sc_force; /* Low-level output for interrupt handlers */
syslog_flush_t sc_flush; /* Flush buffered output (on crash) */
#ifdef CONFIG_SYSLOG_WRITE
syslog_write_t sc_write; /* Write multiple bytes */
#endif
+};
+
+/* This structure provides the interface to a SYSLOG channel */
+
+struct syslog_channel_s
+{
+ /* Channel device */
+
+ FAR void *sc_dev;
Review comment:
remove this field, the convention to add the additional specific driver state is:
```
struct my_syslog_channel_s
{
FAR const struct syslog_channel_ops_s *sc_ops;
// add more fields here
};
```
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,44 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)
+ (FAR const struct syslog_channel_s *channel,
Review comment:
remove const from all callback
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603283967
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -84,12 +84,17 @@
/* This structure provides the interface to a SYSLOG device */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+typedef CODE ssize_t (*syslog_write_t)(FAR void *dev, FAR const char *buf,
+ size_t buflen);
+typedef CODE int (*syslog_putc_t)(FAR void *dev, int ch);
+typedef CODE int (*syslog_flush_t)(FAR void *dev);
struct syslog_channel_s
{
+ /* Channel device */
+
+ void *sc_dev;
Review comment:
```suggestion
FAR void *sc_dev;
```
Missing FAR qualifier on common code.
##########
File path: arch/arm/src/armv7-m/arm_itm_syslog.c
##########
@@ -106,8 +107,10 @@ static const struct syslog_channel_s g_itm_channel =
*
****************************************************************************/
-static int itm_putc(int ch)
+static int itm_putc(void *dev, int ch)
{
+ (void)dev;
Review comment:
```suggestion
UNUSED(dev);
```
You may use the UNUSED macro from `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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603914407
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -84,12 +84,17 @@
/* This structure provides the interface to a SYSLOG device */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+typedef CODE ssize_t (*syslog_write_t)(FAR void *dev, FAR const char *buf,
Review comment:
should we pass ```FAR struct syslog_channel_s *``` instead
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603417101
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
Something that I am also thinking is to completely drop `sc_putc()` and require on every lower-half driver to implement an `sc_write()` only, with the requirement that the driver will perform the write atomically. If the driver cannot actually perform a write, it can iterate internally in a protected section of the code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r604389040
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,44 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)
+ (FAR const struct syslog_channel_s *channel,
Review comment:
Because the callback normally will modify syslog_channel_s, but I am fine if you want to make it const
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r604216138
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,44 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)
+ (FAR const struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+typedef CODE int (*syslog_putc_t)
+ (FAR const struct syslog_channel_s *channel,
+ int ch);
+typedef CODE int (*syslog_flush_t)
+ (FAR const struct syslog_channel_s *channel);
+
+/* SYSLOG device operations */
+struct syslog_channel_ops_s
+{
syslog_putc_t sc_putc; /* Normal buffered output */
syslog_putc_t sc_force; /* Low-level output for interrupt handlers */
syslog_flush_t sc_flush; /* Flush buffered output (on crash) */
#ifdef CONFIG_SYSLOG_WRITE
syslog_write_t sc_write; /* Write multiple bytes */
#endif
+};
+
+/* This structure provides the interface to a SYSLOG channel */
+
+struct syslog_channel_s
+{
+ /* Channel device */
+
+ FAR void *sc_dev;
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r604770073
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,44 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)
+ (FAR const struct syslog_channel_s *channel,
Review comment:
I am not sure what is the "best" here.
I don't see any way that `syslog_channel_s` is modified (or it should).
The members of `sc_dev` are not const, so the callback may use it freely.
Judging from other drivers (e.g. SPI), is shouldn't be const, to keep things uniform and simple.
But this means that also `FAR const struct syslog_channel_s *g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS]` should become non const.
Is this what we want?
Should modifications to `g_syslog_channel` be allowed?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603341029
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
Got it, much clearer now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-814065447
@xiaoxiang781216 I finished with any tests that I had in mind, and I didn't had any problems with this.
Unfortunately, I am not able to test all possible configurations, but at least the serial and the file loggers (and the underlying syslog_device) work correctly.
The rest of the changes seem to be pretty trivial.
I found a couple of issues, but they are not directly related to these changes, so I will proceed with two new PRs immediately after merging this.
Feel free to merge it, if you also think it is OK.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-814114365
Thanks for this @fjpanag
Looking forward to see how to put this to use. I also wonder how this new functionality would deal with logging to an SD card when it is removed, as I would like log to ramlog and SD.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607130534
##########
File path: drivers/syslog/syslog.h
##########
@@ -72,17 +100,19 @@ EXTERN FAR const struct syslog_channel_s *g_syslog_channel
* SYSLOG device. That would be a good extension.
*
* Input Parameters:
- * devpath - The full path to the character device to be used.
- * oflags - File open flags
- * mode - File open mode (only if oflags include O_CREAT)
+ * channel - Handle to syslog channel to be used.
+ * devpath - The full path to the character device to be used.
+ * oflags - File open flags.
+ * mode - File open mode (only if oflags include O_CREAT).
*
* Returned Value:
* Zero (OK) is returned on success; a negated errno value is returned on
* any failure.
*
****************************************************************************/
-int syslog_dev_initialize(FAR const char *devpath, int oflags, int mode);
+int syslog_dev_initialize(FAR struct syslog_channel_s *channel,
Review comment:
Yes I followed this idea. It is indeed better now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603360424
##########
File path: arch/arm/src/armv7-m/arm_itm_syslog.c
##########
@@ -106,8 +107,10 @@ static const struct syslog_channel_s g_itm_channel =
*
****************************************************************************/
-static int itm_putc(int ch)
+static int itm_putc(void *dev, int ch)
{
+ (void)dev;
Review comment:
AFAIK `(void) unused_variable;` is perfectly valid in C, with no portability issues.
Or if I am wrong, please enlighten me.
So this is why I consider this a "styling" issue.
Nevertheless, I will update all occurrences of this to use the `UNUSED()` macro. :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r606688388
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,41 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+/* SYSLOG device operations */
+struct syslog_channel_ops_s
+{
syslog_putc_t sc_putc; /* Normal buffered output */
syslog_putc_t sc_force; /* Low-level output for interrupt handlers */
syslog_flush_t sc_flush; /* Flush buffered output (on crash) */
#ifdef CONFIG_SYSLOG_WRITE
syslog_write_t sc_write; /* Write multiple bytes */
#endif
+};
+
+/* This structure provides the interface to a SYSLOG channel */
+
+struct syslog_channel_s
+{
+ /* Channel operations */
+
+ FAR const struct syslog_channel_ops_s *sc_ops;
+
+ /* Channel device */
+
+ FAR void *sc_dev;
Review comment:
remove sc_dev, let's driver embed syslog_channel_s into his own struct directly like this:
```
struct my_channel_s
{
struct syslog_channel_s;
/* add more sate here */
};
static int my_syslog_putc)(FAR struct syslog_channel_s *channel, int ch)
{
FAR struct my_channel_s *my = (FAR struct my_channel_s *)channel;
/* access the state by my->xxx */
}
```
This is the convention used by many driver.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607128788
##########
File path: drivers/syslog/syslog_filechannel.c
##########
@@ -65,6 +66,15 @@ static const struct syslog_channel_s g_syslog_file_channel =
#endif
};
+/* This structure describes the SYSLOG channel */
+
+static struct syslog_dev_s g_syslog_file_channel =
Review comment:
done
##########
File path: drivers/syslog/syslog_devchannel.c
##########
@@ -86,6 +89,15 @@ static const struct syslog_channel_s g_syslog_dev_channel =
#endif
};
+/* This structure describes the SYSLOG channel */
+
+static struct syslog_dev_s g_syslog_dev_channel =
Review comment:
done
##########
File path: drivers/syslog/syslog_consolechannel.c
##########
@@ -56,30 +57,34 @@
/* SYSLOG channel methods */
-#ifndef HAVE_LOWPUTC
-static int syslog_console_force(int ch);
-#endif
+static int syslog_console_force(FAR struct syslog_channel_s *channel,
+ int ch);
/****************************************************************************
* Private Data
****************************************************************************/
-/* This structure describes the SYSLOG channel */
+/* This structure describes the channel's operations. */
-static const struct syslog_channel_s g_syslog_console_channel =
+static const struct syslog_channel_ops_s g_syslog_ops =
{
syslog_dev_putc,
-#ifdef HAVE_LOWPUTC
- up_putc,
-#else
syslog_console_force,
-#endif
syslog_dev_flush,
#ifdef CONFIG_SYSLOG_WRITE
syslog_dev_write,
#endif
};
+/* This structure describes the SYSLOG channel */
+
+static struct syslog_dev_s g_syslog_console_channel =
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603971492
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -84,12 +84,17 @@
/* This structure provides the interface to a SYSLOG device */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+typedef CODE ssize_t (*syslog_write_t)(FAR void *dev, FAR const char *buf,
Review comment:
My suggestion come from other drivers(e.g. spi_dev_s/spi_ops_s) practice. It's better to do the right interface design in the first step, instead changing the interfae prootype again and again.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607129902
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -188,16 +175,17 @@ static inline void syslog_dev_givesem(void)
* that is why that case is handled in syslog_semtake().
*
* Input Parameters:
- * None.
+ * channel - Handle to syslog channel to be used.
*
* Returned Value:
* Zero (OK) is returned on success; a negated errno value is returned on
* any failure.
*
****************************************************************************/
-static int syslog_dev_outputready(void)
+static int syslog_dev_outputready(FAR struct syslog_channel_s *channel)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603417101
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
Something that I am also thinking is to completely drop `sc_putc()` and require on every lower-half driver to implement an `sc_write()` only, with the requirement that the driver will perform the write atomically. If the driver cannot actually perform a write, it can iterate internally in a protection section of the code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r604126971
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -84,12 +84,17 @@
/* This structure provides the interface to a SYSLOG device */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+typedef CODE ssize_t (*syslog_write_t)(FAR void *dev, FAR const char *buf,
Review comment:
@xiaoxiang781216 You may have a look, it seems better now.
I think this is what you had in mind.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603374409
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
@fjpanag it looks that syslog channel become a normal driver. Should we follow other drivers convention? e.g. spi_dev_s/spi_ops_s or rtc_lowerhalf_s/rtc_ops_s, so other people will be more familiar with your change.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603284142
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -443,34 +431,38 @@ void syslog_dev_uninitialize(void)
* for the character driver interface.
*
* Input Parameters:
- * buffer - The buffer containing the data to be output
- * buflen - The number of bytes in the buffer
+ * syslog_dev - Handle to syslog device to be used.
+ * buffer - The buffer containing the data to be output
+ * buflen - The number of bytes in the buffer
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607049302
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,41 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+/* SYSLOG device operations */
+struct syslog_channel_ops_s
+{
syslog_putc_t sc_putc; /* Normal buffered output */
syslog_putc_t sc_force; /* Low-level output for interrupt handlers */
syslog_flush_t sc_flush; /* Flush buffered output (on crash) */
#ifdef CONFIG_SYSLOG_WRITE
syslog_write_t sc_write; /* Write multiple bytes */
#endif
+};
+
+/* This structure provides the interface to a SYSLOG channel */
+
+struct syslog_channel_s
+{
+ /* Channel operations */
+
+ FAR const struct syslog_channel_ops_s *sc_ops;
+
+ /* Channel device */
+
+ FAR void *sc_dev;
Review comment:
@xiaoxiang781216 Please have a look whether it is OK now, so I can proceed with testing it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r606688388
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,41 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)(FAR struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+typedef CODE int (*syslog_putc_t)(FAR struct syslog_channel_s *channel,
+ int ch);
+typedef CODE int (*syslog_flush_t)(FAR struct syslog_channel_s *channel);
+
+/* SYSLOG device operations */
+struct syslog_channel_ops_s
+{
syslog_putc_t sc_putc; /* Normal buffered output */
syslog_putc_t sc_force; /* Low-level output for interrupt handlers */
syslog_flush_t sc_flush; /* Flush buffered output (on crash) */
#ifdef CONFIG_SYSLOG_WRITE
syslog_write_t sc_write; /* Write multiple bytes */
#endif
+};
+
+/* This structure provides the interface to a SYSLOG channel */
+
+struct syslog_channel_s
+{
+ /* Channel operations */
+
+ FAR const struct syslog_channel_ops_s *sc_ops;
+
+ /* Channel device */
+
+ FAR void *sc_dev;
Review comment:
remove sc_dev, let's driver embed syslog_channel_s into his own struct directly like this:
```
struct my_channel_s
{
struct syslog_channel_s;
/* add more sate here */
};
static int my_syslog_putc)(FAR struct syslog_channel_s *channel, int ch)
{
FAR struct my_channel_s *my = (FAR struct my_channel_s *)channel;
/* access the state by my->xxx */
}
```
This is the convention used by many drivers.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-814130198
> Thanks for this @fjpanag
> Looking forward to see how to put this to use. I also wonder how this new functionality would deal with logging to an SD card when it is removed, as I would like log to ramlog and SD.
This is similar to my use case. I will use an SD card and serial output.
For the moment I am thinking having the SD automounter to add/remove the file channel while the SD is mounted or unmounted.
This is done in my "board" logic. I don't know how the kernel could handle this. (If only we had inotify....)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603920952
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -84,12 +84,17 @@
/* This structure provides the interface to a SYSLOG device */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+typedef CODE ssize_t (*syslog_write_t)(FAR void *dev, FAR const char *buf,
Review comment:
I was thinking that it is better to keep it as a `void *` temporarily, as the underlying channels are not uniform yet (like ARM ITM for example).
But I think I will just do it properly, and go for your your recommendation above.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603400585
##########
File path: arch/arm/src/armv7-m/arm_itm_syslog.c
##########
@@ -106,8 +107,10 @@ static const struct syslog_channel_s g_itm_channel =
*
****************************************************************************/
-static int itm_putc(int ch)
+static int itm_putc(void *dev, int ch)
{
+ (void)dev;
Review comment:
I think it is fixed everywhere now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-813442458
> LGTM.
Please don't merge till I confirm finishing of any testing still pending.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603279206
##########
File path: drivers/syslog/syslog.h
##########
@@ -294,16 +309,18 @@ int syslog_force(int ch);
* for the character driver interface.
*
* Input Parameters:
- * buffer - The buffer containing the data to be output
- * buflen - The number of bytes in the buffer
+ * syslog_dev - Handle to syslog device to be used.
+ * buffer - The buffer containing the data to be output
+ * buflen - The number of bytes in the buffer
Review comment:
```suggestion
* buffer - The buffer containing the data to be output.
* buflen - The number of bytes in the buffer.
```
nit: Just for consistency.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603414103
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
Yes, this is what I had in mind, but you expressed it in a better way. Having upper/lower drivers for syslog too.
However this means that new lower drivers are to be created (as mentioned above), files may need to be moved etc.
I would prefer to finalize this migration in a later PR, after the lower-hald driver interface is finalized (`sc_close()` is still to be introduced, thread-safety protections are to be added and more).
What do you think? Shall we keep this part for the moment?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607045507
##########
File path: drivers/syslog/syslog_devchannel.c
##########
@@ -86,6 +89,15 @@ static const struct syslog_channel_s g_syslog_dev_channel =
#endif
};
+/* This structure describes the SYSLOG channel */
+
+static struct syslog_dev_s g_syslog_dev_channel =
Review comment:
remove, let syslog_dev_initialize return ```FAR syslog_channel_s *``` instead
##########
File path: drivers/syslog/syslog_filechannel.c
##########
@@ -65,6 +66,15 @@ static const struct syslog_channel_s g_syslog_file_channel =
#endif
};
+/* This structure describes the SYSLOG channel */
+
+static struct syslog_dev_s g_syslog_file_channel =
Review comment:
remove, let syslog_dev_initialize return ```FAR syslog_channel_s *``` instead
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -73,26 +73,10 @@ enum syslog_dev_state
SYSLOG_OPENED, /* SYSLOG device is open and ready to use */
};
-/* This structure contains all SYSLOGing state information */
-
-struct syslog_dev_s
Review comment:
Don't remove, let's change to:
```
* This structure contains all SYSLOGing state information */
struct syslog_dev_s
{
struct syslog_channel_s channel;
uint8_t sl_state; /* See enum syslog_dev_state */
uint8_t sl_oflags; /* Saved open mode (for re-open) */
uint16_t sl_mode; /* Saved open flags (for re-open) */
sem_t sl_sem; /* Enforces mutually exclusive access */
pid_t sl_holder; /* PID of the thread that holds the semaphore */
struct file sl_file; /* The syslog file structure */
FAR char *sl_devpath; /* Full path to the character device */
};
```
##########
File path: drivers/syslog/syslog_chardev.c
##########
@@ -30,6 +30,7 @@
#include <poll.h>
#include <errno.h>
+#include <nuttx/syslog/syslog.h>
Review comment:
remove, don't need
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -188,16 +175,17 @@ static inline void syslog_dev_givesem(void)
* that is why that case is handled in syslog_semtake().
*
* Input Parameters:
- * None.
+ * channel - Handle to syslog channel to be used.
*
* Returned Value:
* Zero (OK) is returned on success; a negated errno value is returned on
* any failure.
*
****************************************************************************/
-static int syslog_dev_outputready(void)
+static int syslog_dev_outputready(FAR struct syslog_channel_s *channel)
Review comment:
```suggestion
static int syslog_dev_outputready(FAR struct syslog_dev_s *syslog_dev)
```
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -106,8 +90,9 @@ static const uint8_t g_syscrlf[2] =
* Name: syslog_dev_takesem
****************************************************************************/
-static inline int syslog_dev_takesem(void)
+static inline int syslog_dev_takesem(FAR struct syslog_channel_s *channel)
Review comment:
```suggestion
static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_devl)
```
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -138,25 +123,27 @@ static inline int syslog_dev_takesem(void)
* of the semaphore.
*/
- g_syslog_dev.sl_holder = me;
+ syslog_dev->dev.sl_holder = me;
return OK;
}
/****************************************************************************
* Name: syslog_dev_givesem
****************************************************************************/
-static inline void syslog_dev_givesem(void)
+static inline void syslog_dev_givesem(FAR struct syslog_channel_s *channel)
Review comment:
```suggestion
static inline void syslog_dev_givesem(FAR struct syslog_dev_s *syslog_dev)
```
##########
File path: drivers/syslog/syslog.h
##########
@@ -72,17 +100,19 @@ EXTERN FAR const struct syslog_channel_s *g_syslog_channel
* SYSLOG device. That would be a good extension.
*
* Input Parameters:
- * devpath - The full path to the character device to be used.
- * oflags - File open flags
- * mode - File open mode (only if oflags include O_CREAT)
+ * channel - Handle to syslog channel to be used.
+ * devpath - The full path to the character device to be used.
+ * oflags - File open flags.
+ * mode - File open mode (only if oflags include O_CREAT).
*
* Returned Value:
* Zero (OK) is returned on success; a negated errno value is returned on
* any failure.
*
****************************************************************************/
-int syslog_dev_initialize(FAR const char *devpath, int oflags, int mode);
+int syslog_dev_initialize(FAR struct syslog_channel_s *channel,
Review comment:
let syslog_dev_initialize allocate syslog_channel_s(syslog_dev_s) internally instead
##########
File path: drivers/syslog/syslog.h
##########
@@ -48,9 +50,35 @@ extern "C"
*/
struct syslog_channel_s; /* Forward reference */
-EXTERN FAR const struct syslog_channel_s *g_syslog_channel
+EXTERN FAR struct syslog_channel_s *g_syslog_channel
[CONFIG_SYSLOG_MAX_CHANNELS];
+/* This structure contains all SYSLOGing device state information */
+
+struct syslog_dev_state_s
Review comment:
move to syslog_device.c
##########
File path: drivers/syslog/syslog.h
##########
@@ -48,9 +50,35 @@ extern "C"
*/
struct syslog_channel_s; /* Forward reference */
-EXTERN FAR const struct syslog_channel_s *g_syslog_channel
+EXTERN FAR struct syslog_channel_s *g_syslog_channel
[CONFIG_SYSLOG_MAX_CHANNELS];
+/* This structure contains all SYSLOGing device state information */
+
+struct syslog_dev_state_s
+{
+ uint8_t sl_state; /* See enum syslog_dev_state */
+ uint8_t sl_oflags; /* Saved open mode (for re-open) */
+ uint16_t sl_mode; /* Saved open flags (for re-open) */
+ sem_t sl_sem; /* Enforces mutually exclusive access */
+ pid_t sl_holder; /* PID of the thread that holds the semaphore */
+ struct file sl_file; /* The syslog file structure */
+ FAR char *sl_devpath; /* Full path to the character device */
+};
+
+/* This structure contains SYSLOG device channel and state data */
+
+struct syslog_dev_s
Review comment:
move to syslog_device.c
##########
File path: drivers/syslog/syslog_consolechannel.c
##########
@@ -56,30 +57,34 @@
/* SYSLOG channel methods */
-#ifndef HAVE_LOWPUTC
-static int syslog_console_force(int ch);
-#endif
+static int syslog_console_force(FAR struct syslog_channel_s *channel,
+ int ch);
/****************************************************************************
* Private Data
****************************************************************************/
-/* This structure describes the SYSLOG channel */
+/* This structure describes the channel's operations. */
-static const struct syslog_channel_s g_syslog_console_channel =
+static const struct syslog_channel_ops_s g_syslog_ops =
{
syslog_dev_putc,
-#ifdef HAVE_LOWPUTC
- up_putc,
-#else
syslog_console_force,
-#endif
syslog_dev_flush,
#ifdef CONFIG_SYSLOG_WRITE
syslog_dev_write,
#endif
};
+/* This structure describes the SYSLOG channel */
+
+static struct syslog_dev_s g_syslog_console_channel =
Review comment:
remove, let syslog_dev_initialize return ```FAR syslog_channel_s *``` instead
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603344875
##########
File path: arch/arm/src/armv7-m/arm_itm_syslog.c
##########
@@ -106,8 +107,10 @@ static const struct syslog_channel_s g_itm_channel =
*
****************************************************************************/
-static int itm_putc(int ch)
+static int itm_putc(void *dev, int ch)
{
+ (void)dev;
Review comment:
I think this is not covered in coding standard.
What is the preferred style in NuttX?
Also, maybe someone to add this in coding standard?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603364860
##########
File path: arch/arm/src/armv7-m/arm_itm_syslog.c
##########
@@ -106,8 +107,10 @@ static const struct syslog_channel_s g_itm_channel =
*
****************************************************************************/
-static int itm_putc(int ch)
+static int itm_putc(void *dev, int ch)
{
+ (void)dev;
Review comment:
It is a matter of consistency with the rest of the codebase. Also, I think the macro is more verbose in its purpose, that's why it is used.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603319629
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
The function `sc_putc()` has changed prototype, so a pointer to the device is passed:
```c
typedef CODE int (*syslog_putc_t)(FAR void *dev, int ch);
```
Thus `up_putc()` is incompatible. So I use `syslog_default_putc()` to translate the arguments from the new `sc_putc()` to the existing `up_putc()`.
Notice the new argument in `drivers/syslog/syslog_channel.c`, line 112.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607131300
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -73,26 +73,10 @@ enum syslog_dev_state
SYSLOG_OPENED, /* SYSLOG device is open and ready to use */
};
-/* This structure contains all SYSLOGing state information */
-
-struct syslog_dev_s
Review comment:
I did it like this. `struct syslog_dev_state_s` is removed and `struct syslog_dev_s` is now private.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603282942
##########
File path: drivers/syslog/syslog.h
##########
@@ -72,17 +87,19 @@ EXTERN FAR const struct syslog_channel_s *g_syslog_channel
* SYSLOG device. That would be a good extension.
*
* Input Parameters:
- * devpath - The full path to the character device to be used.
- * oflags - File open flags
- * mode - File open mode (only if oflags include O_CREAT)
+ * syslog_dev - Handle to syslog device to be used.
+ * devpath - The full path to the character device to be used.
+ * oflags - File open flags
+ * mode - File open mode (only if oflags include O_CREAT)
Review comment:
Done.
##########
File path: drivers/syslog/syslog.h
##########
@@ -294,16 +309,18 @@ int syslog_force(int ch);
* for the character driver interface.
*
* Input Parameters:
- * buffer - The buffer containing the data to be output
- * buflen - The number of bytes in the buffer
+ * syslog_dev - Handle to syslog device to be used.
+ * buffer - The buffer containing the data to be output
+ * buflen - The number of bytes in the buffer
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603281342
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -443,34 +431,38 @@ void syslog_dev_uninitialize(void)
* for the character driver interface.
*
* Input Parameters:
- * buffer - The buffer containing the data to be output
- * buflen - The number of bytes in the buffer
+ * syslog_dev - Handle to syslog device to be used.
+ * buffer - The buffer containing the data to be output
+ * buflen - The number of bytes in the buffer
Review comment:
```suggestion
* buffer - The buffer containing the data to be output.
* buflen - The number of bytes in the buffer.
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607130037
##########
File path: drivers/syslog/syslog.h
##########
@@ -48,9 +50,35 @@ extern "C"
*/
struct syslog_channel_s; /* Forward reference */
-EXTERN FAR const struct syslog_channel_s *g_syslog_channel
+EXTERN FAR struct syslog_channel_s *g_syslog_channel
[CONFIG_SYSLOG_MAX_CHANNELS];
+/* This structure contains all SYSLOGing device state information */
+
+struct syslog_dev_state_s
Review comment:
done
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -106,8 +90,9 @@ static const uint8_t g_syscrlf[2] =
* Name: syslog_dev_takesem
****************************************************************************/
-static inline int syslog_dev_takesem(void)
+static inline int syslog_dev_takesem(FAR struct syslog_channel_s *channel)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r604172827
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,44 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)
+ (FAR const struct syslog_channel_s *channel,
+ FAR const char *buf, size_t buflen);
+typedef CODE int (*syslog_putc_t)
+ (FAR const struct syslog_channel_s *channel,
+ int ch);
+typedef CODE int (*syslog_flush_t)
+ (FAR const struct syslog_channel_s *channel);
+
+/* SYSLOG device operations */
+struct syslog_channel_ops_s
+{
syslog_putc_t sc_putc; /* Normal buffered output */
syslog_putc_t sc_force; /* Low-level output for interrupt handlers */
syslog_flush_t sc_flush; /* Flush buffered output (on crash) */
#ifdef CONFIG_SYSLOG_WRITE
syslog_write_t sc_write; /* Write multiple bytes */
#endif
+};
+
+/* This structure provides the interface to a SYSLOG channel */
+
+struct syslog_channel_s
+{
+ /* Channel device */
+
+ FAR void *sc_dev;
Review comment:
remove this field, the convention to add the additional specific driver state is:
```
struct my_syslog_channel_s
{
FAR const struct syslog_channel_ops_s *sc_ops;
// add more fields here
};
```
so, we don't consume sc_dev field for the simple driver, but the complex driver can cast syslog_channel_s to my_syslog_channel_s directly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603414103
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
Yes, this is what I had in mind, but you expressed it in a better way. Having upper/lower drivers for syslog too.
However this means that new lower drivers are to be created (as mentioned above), files may need to be moved etc.
I would prefer to finalize this migration in a later PR, after the lower-half driver interface is finalized (`sc_close()` is still to be introduced, thread-safety protections are to be added and more).
What do you think? Shall we keep this part for the moment?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603290464
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
I'm confused about this change in this PR.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603356999
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
Just to note, my intention is to add a new channel in the future, that it will encapsulate ARM ITM, up_putc, rpmsg etc, so even these interfaces follow the common structure, and use a proper `struct syslog_dev_s` instead of this ungly `void *` in `struct syslog_channel_s`.
I believe that it will simplify the code more.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607128664
##########
File path: drivers/syslog/syslog_chardev.c
##########
@@ -30,6 +30,7 @@
#include <poll.h>
#include <errno.h>
+#include <nuttx/syslog/syslog.h>
Review comment:
Yep, removed. Was added by mistake.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603374409
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
@fjpanag it looks that syslog channel become a normal driver. Should we follow other drivers convention? e.g. spi_dev_s/spi_ops_s or rtc_lowerhalf_s/rtc_ops_s, so other people will be more familiar with your change.
It's a good time to align syslog_channel_s with other driver without compatibility in mind since this change will break the interface anyway.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603278858
##########
File path: drivers/syslog/syslog.h
##########
@@ -72,17 +87,19 @@ EXTERN FAR const struct syslog_channel_s *g_syslog_channel
* SYSLOG device. That would be a good extension.
*
* Input Parameters:
- * devpath - The full path to the character device to be used.
- * oflags - File open flags
- * mode - File open mode (only if oflags include O_CREAT)
+ * syslog_dev - Handle to syslog device to be used.
+ * devpath - The full path to the character device to be used.
+ * oflags - File open flags
+ * mode - File open mode (only if oflags include O_CREAT)
Review comment:
```suggestion
* oflags - File open flags.
* mode - File open mode (only if oflags include O_CREAT).
```
nit: Just for consistency.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r604218878
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -82,22 +82,44 @@
* Public Types
****************************************************************************/
-/* This structure provides the interface to a SYSLOG device */
+/* Forward declaration */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+struct syslog_channel_s;
-struct syslog_channel_s
-{
- /* I/O redirection methods */
+/* SYSLOG I/O redirection methods */
+
+typedef CODE ssize_t (*syslog_write_t)
+ (FAR const struct syslog_channel_s *channel,
Review comment:
Why is that a problem? Isn't it constant?
If const is to be removed, then all channels will have to also be declared as non-const.
Else they will have to be always be cast to non-const, in which case there is no meaning to the const declaration in the firt place.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603327397
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -84,12 +84,17 @@
/* This structure provides the interface to a SYSLOG device */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+typedef CODE ssize_t (*syslog_write_t)(FAR void *dev, FAR const char *buf,
+ size_t buflen);
+typedef CODE int (*syslog_putc_t)(FAR void *dev, int ch);
+typedef CODE int (*syslog_flush_t)(FAR void *dev);
struct syslog_channel_s
{
+ /* Channel device */
+
+ void *sc_dev;
Review comment:
I'm sorry, I always forget `FAR`s... Added now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-814082473
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-814149242
What I'm thinking is that maybe syslog file output should gracefully handle writing to non-accessible file, by continuing to work OK once/if the file reappears. I think this could be realated to how to perform log rotation. In our case we do not have a syslog daemon so something like that would happen externally.
Anyway, I might test this soon when I get into SD card logging.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603353811
##########
File path: arch/arm/src/armv7-m/arm_itm_syslog.c
##########
@@ -106,8 +107,10 @@ static const struct syslog_channel_s g_itm_channel =
*
****************************************************************************/
-static int itm_putc(int ch)
+static int itm_putc(void *dev, int ch)
{
+ (void)dev;
Review comment:
I believe this is something that should not be covered in the NuttX Style Guide.
It is more of a code quality question, to make the code cleaner.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603329542
##########
File path: include/nuttx/syslog/syslog.h
##########
@@ -84,12 +84,17 @@
/* This structure provides the interface to a SYSLOG device */
-typedef CODE ssize_t (*syslog_write_t)(FAR const char *buf, size_t buflen);
-typedef CODE int (*syslog_putc_t)(int ch);
-typedef CODE int (*syslog_flush_t)(void);
+typedef CODE ssize_t (*syslog_write_t)(FAR void *dev, FAR const char *buf,
+ size_t buflen);
+typedef CODE int (*syslog_putc_t)(FAR void *dev, int ch);
+typedef CODE int (*syslog_flush_t)(FAR void *dev);
struct syslog_channel_s
{
+ /* Channel device */
+
+ void *sc_dev;
Review comment:
No problem!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607130037
##########
File path: drivers/syslog/syslog.h
##########
@@ -48,9 +50,35 @@ extern "C"
*/
struct syslog_channel_s; /* Forward reference */
-EXTERN FAR const struct syslog_channel_s *g_syslog_channel
+EXTERN FAR struct syslog_channel_s *g_syslog_channel
[CONFIG_SYSLOG_MAX_CHANNELS];
+/* This structure contains all SYSLOGing device state information */
+
+struct syslog_dev_state_s
Review comment:
Removed, as noted bellow.
##########
File path: drivers/syslog/syslog.h
##########
@@ -48,9 +50,35 @@ extern "C"
*/
struct syslog_channel_s; /* Forward reference */
-EXTERN FAR const struct syslog_channel_s *g_syslog_channel
+EXTERN FAR struct syslog_channel_s *g_syslog_channel
[CONFIG_SYSLOG_MAX_CHANNELS];
+/* This structure contains all SYSLOGing device state information */
+
+struct syslog_dev_state_s
+{
+ uint8_t sl_state; /* See enum syslog_dev_state */
+ uint8_t sl_oflags; /* Saved open mode (for re-open) */
+ uint16_t sl_mode; /* Saved open flags (for re-open) */
+ sem_t sl_sem; /* Enforces mutually exclusive access */
+ pid_t sl_holder; /* PID of the thread that holds the semaphore */
+ struct file sl_file; /* The syslog file structure */
+ FAR char *sl_devpath; /* Full path to the character device */
+};
+
+/* This structure contains SYSLOG device channel and state data */
+
+struct syslog_dev_s
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603481879
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
> Yes, this is what I had in mind, but you expressed it in a better way. Having upper/lower drivers for syslog too.
>
> However this means that new lower drivers are to be created (as mentioned above), files may need to be moved etc.
>
Not to much to become upper/lower:
1. Rename syslog_channel_s to syslog_channel_ops_s
2. Add new struct syslog_channel_s which contain the pointer of syslog_channel_ops_s
3. The callback in syslog_channel_ops_s accept the pointer of syslog_channel_s as the new first argument
> I would prefer to finalize this migration in a later PR, after the lower-half driver interface is finalized (`sc_close()` is still to be introduced, thread-safety protections are to be added and more).
>
The more callback like sc_close could be added in later if required
> What do you think? Shall we keep this part for the moment?
I am fine which one come first, my major concern is to reduce your implmentation cost.
> Something that I am also thinking is to completely drop `sc_putc()` and require on every lower-half driver to implement an `sc_write()` only, with the requirement that the driver will perform the write atomically. If the driver cannot actually perform a write, it can iterate internally in a protected section of the code.
Yes, I also think sc_putc make the interface more complicated than necessary. Maybe, it's better to remove sc_putc first before we redesign the syslog 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-814156634
> What I'm thinking is that maybe syslog file output should gracefully handle writing to non-accessible file, by continuing to work OK once/if the file reappears.
This is already done.
`syslog_dev_outputready()` will automatically open the file when it becomes available.
> I think this could be realated to how to perform log rotation.
Log rotation is coming. Although it won't be done in a daemon, rather only during adding of the file, to keep things simple and efficient.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r607129095
##########
File path: drivers/syslog/syslog_device.c
##########
@@ -138,25 +123,27 @@ static inline int syslog_dev_takesem(void)
* of the semaphore.
*/
- g_syslog_dev.sl_holder = me;
+ syslog_dev->dev.sl_holder = me;
return OK;
}
/****************************************************************************
* Name: syslog_dev_givesem
****************************************************************************/
-static inline void syslog_dev_givesem(void)
+static inline void syslog_dev_givesem(FAR struct syslog_channel_s *channel)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#issuecomment-810002637
Regarding the style issue:
```
/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/drivers/syslog/syslog_write.c:100:83: error: Long line found
```
has anyone any recommendation how this is to be fixed?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603358527
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
I agree, that is a good goal
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a change in pull request #3226: syslog: Added multi device support in syslog_device.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on a change in pull request #3226:
URL: https://github.com/apache/incubator-nuttx/pull/3226#discussion_r603356999
##########
File path: drivers/syslog/syslog_channel.c
##########
@@ -44,9 +44,7 @@
* Private Function Prototypes
****************************************************************************/
-#if defined(CONFIG_ARCH_LOWPUTC)
-# define HAVE_LOWPUTC
-#elif !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
+#if !defined(CONFIG_RAMLOG_SYSLOG) && !defined(CONFIG_SYSLOG_RPMSG)
# define NEED_LOWPUTC
#endif
Review comment:
Just to note, my intention is to add a new channel in the future, that it will encapsulate ARM ITM, up_putc, rpmsg etc, so even these interfaces follow the common structure, and use a proper `struct syslog_dev_s` instead of this ugly `void *` in `struct syslog_channel_s`.
I believe that it will simplify the code more.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org