Hi Florian!
Thank you for reviewing my patch!
I hopefully corrected the issues you mentioned and also tried to make
mb_init_connection_[rtu|tcp] less redundant by moving things back into
mb_init_connection whenever possible.
On Fri, Apr 15, 2011 at 9:37 AM, Florian Forster <[email protected]> wrote:
>> +#ifdef __APPLE_CC__
>> + char device[64];
>> +#else
>> + char device[16];
>> +#endif
>
> Why this distinction? Couldn't we just use 64 bytes in both cases?
> 48 bytes don't hurt anyone nowadays.
Very true, but it's defined with that distinction in modbus.h of
libmodbus. Actually having only 16 bytes for the device name makes it
impossible to use udev-created symlinks as they tend to be longer than
16 bytes.
See https://github.com/stephane/libmodbus/issues/11
I removed the distinction and added a check to the config-reading
phase so device-names with more than 15 characters can be detected and
the user can be warned about the restriction. A possible work-around
would be to resolve symbolic links and pass the link target to
libmodbus (which is usually shorter than 15 chars)
> What happends if one passes a zero to libmodbus?
libmodbus sanitizes them and uses built-in defaults when invalid
values are passed, printing a warning to stdout. As you mentioned,
this happens in a stage where those warnings are not revealed to the
user, so I kept some basic checks in the configuration phase,
>> + /* data_bit = */ (uint8_t)host->data_bits,
>> + /* stop_bit = */ (uint8_t)host->stop_bits);
>
> Why are those members not of type uint8_t? As far as I can see they're
> not used for anything else.
They are integer because I use cf_util_get_int to read them from the
config file. Casting the parameter of cf_util_get_int to be uint8_t*
is kinda dangerous if it expects it to be int... Casting the parameter
of modbus_init_rtu to uint8_t would only truncate unreasonable values
and this would never happen as the values go through some sanity
checking after being parsed by cf_util_get_integer.
>> + else if ( host->type_com == RTU )
>
> I don't see where this member is ever set to "RTU".
So this was why it didn't work... How embarrassing... Added the
assignment accordingly.
>> + else if (strcasecmp ("Parity", child->key) == 0)
>> + {
>> + status = cf_util_get_string_buffer (child, host->parity, sizeof
>> (host->parity));
>> + if (status != 0)
>> + return (status);
>> + if (host->parity[0] == 0)
>> + return (EINVAL);
>
> You should do the sanity checking here: During the configuration period
> errors can still be printed to STDOUT and the user is more likely to see
> them.
Considering this, does it make sense to make the call to
modbus_init_[tcp|rtu] already during the configuration phase?
>
>> + else if (strcasecmp ("Interval", child->key) == 0)
>> + status = cf_util_get_int (child, &host->interval);
>
> Intervals are stored in "cdtime_t" since version 5.0. There is a
> specialized config handling function for this.
If you don't mind, I'd complete the patch against 4.3.10 for now, as
it's main purpose is to work with OpenWrt which includes version 4 of
collectd for now. As soon as this is done, I'll have a look at version
5 and the changes you mentioned.
--- a/src/modbus.c 2011-03-26 18:08:53.000000000 +0200
+++ b/src/modbus.c 2011-04-25 12:35:00.000000000 +0300
@@ -91,6 +91,12 @@
struct mb_host_s /* {{{ */
{
char host[DATA_MAX_NAME_LEN];
+ type_com_t type_com;
+ char device[64];
+ int baud;
+ int data_bits;
+ int stop_bits;
+ char parity[5];
char node[NI_MAXHOST];
/* char service[NI_MAXSERV]; */
int port;
@@ -265,25 +271,10 @@
return (conv.f);
} /* }}} float mb_register_to_float */
-static int mb_init_connection (mb_host_t *host) /* {{{ */
+static int mb_init_connection_tcp (mb_host_t *host) /* {{{ */
{
int status;
- if (host == NULL)
- return (EINVAL);
-
- if (host->is_connected)
- return (0);
-
- /* Only reconnect once per interval. */
- if (host->have_reconnected)
- return (-1);
-
- modbus_set_debug (&host->connection, 1);
-
- /* We'll do the error handling ourselves. */
- modbus_set_error_handling (&host->connection, NOP_ON_ERROR);
-
if ((host->port < 1) || (host->port > 65535))
host->port = MODBUS_TCP_DEFAULT_PORT;
@@ -305,6 +296,56 @@
host->is_connected = 1;
host->have_reconnected = 1;
return (0);
+} /* }}} int mb_init_connection_tcp */
+
+static int mb_init_connection_rtu (mb_host_t *host) /* {{{ */
+{
+ int status;
+ DEBUG ("Modbus plugin: Trying to open \"%s\".",
+ host->device);
+
+ modbus_init_rtu (&host->connection,
+ /* device = */ host->device,
+ /* baud = */ host->baud,
+ /* parity = */ host->parity,
+ /* data_bit = */ (uint8_t)host->data_bits,
+ /* stop_bit = */ (uint8_t)host->stop_bits);
+
+ status = modbus_connect (&host->connection);
+ if (status != 0)
+ {
+ ERROR ("Modbus plugin: modbus_connect (%s, %i baud, %i %s %i) failed with status %i.",
+ host->device, host->baud, host->data_bits, host->parity, host->stop_bits, status);
+ return (status);
+ }
+
+ host->is_connected = 1;
+ host->have_reconnected = 1;
+ return (0);
+} /* }}} int mb_init_connection_rtu */
+
+static int mb_init_connection (mb_host_t *host) /* {{{ */
+{
+ if (host == NULL)
+ return (EINVAL);
+
+ if (host->is_connected)
+ return (0);
+
+ /* Only reconnect once per interval. */
+ if (host->have_reconnected)
+ return (-1);
+
+ modbus_set_debug (&host->connection, 1);
+
+ /* We'll do the error handling ourselves. */
+ modbus_set_error_handling (&host->connection, NOP_ON_ERROR);
+
+ if ( host->type_com == TCP )
+ return mb_init_connection_tcp(host);
+ else if ( host->type_com == RTU )
+ return mb_init_connection_rtu(host);
+ else return -1;
} /* }}} int mb_init_connection */
#define CAST_TO_VALUE_T(ds,vt,raw) do { \
@@ -748,6 +789,7 @@
if (host->host[0] == 0)
return (EINVAL);
+ host->type_com = TCP;
for (i = 0; i < ci->children_num; i++)
{
oconfig_item_t *child = ci->children + i;
@@ -817,6 +859,122 @@
return (status);
} /* }}} int mb_config_add_host */
+static int mb_config_add_device (oconfig_item_t *ci) /* {{{ */
+{
+ mb_host_t *host;
+ int status;
+ int i;
+
+ host = malloc (sizeof (*host));
+ if (host == NULL)
+ return (ENOMEM);
+ memset (host, 0, sizeof (*host));
+ host->slaves = NULL;
+
+ status = cf_util_get_string_buffer (ci, host->host, sizeof (host->host));
+ if (status != 0)
+ return (status);
+ if (host->host[0] == 0)
+ return (EINVAL);
+ host->type_com = RTU;
+ for (i = 0; i < ci->children_num; i++)
+ {
+ oconfig_item_t *child = ci->children + i;
+ status = 0;
+
+ if (strcasecmp ("Device", child->key) == 0)
+ {
+ status = cf_util_get_string_buffer (child, host->device, sizeof (host->device));
+ if (status != 0)
+ return (status);
+ if (host->device[0] == 0)
+ return (EINVAL);
+ /* XXX: libmodbus doesn't allow device names with more than 15 characters for now */
+ if (strlen(host->device) > 15)
+ return (EINVAL);
+ }
+ else if (strcasecmp ("Baud", child->key) == 0)
+ {
+ status = cf_util_get_int (child, &host->baud);
+ if (host->baud < 110)
+ status = -1;
+ }
+ else if (strcasecmp ("DataBits", child->key) == 0)
+ {
+ status = cf_util_get_int (child, &host->data_bits);
+ if ((host->data_bits < 5) || (host->data_bits > 8))
+ status = -1;
+ }
+ else if (strcasecmp ("StopBits", child->key) == 0)
+ {
+ status = cf_util_get_int (child, &host->stop_bits);
+ if ((host->stop_bits < 1) || (host->stop_bits > 2))
+ status = -1;
+ }
+ else if (strcasecmp ("Parity", child->key) == 0)
+ {
+ status = cf_util_get_string_buffer (child, host->parity, sizeof (host->parity));
+ if (status != 0)
+ return (status);
+ if (host->parity[0] == 0)
+ return (EINVAL);
+ if ((strcmp ("even", host->parity) != 0)
+ && (strcmp ("odd", host->parity) != 0)
+ && (strcmp ("none", host->parity) != 0))
+ return (EINVAL);
+
+ }
+ else if (strcasecmp ("Interval", child->key) == 0)
+ status = cf_util_get_int (child, &host->interval);
+ else if (strcasecmp ("Slave", child->key) == 0)
+ /* Don't set status: Gracefully continue if a slave fails. */
+ mb_config_add_slave (host, child);
+ else
+ {
+ ERROR ("Modbus plugin: Unknown configuration option: %s", child->key);
+ status = -1;
+ }
+
+ if (status != 0)
+ break;
+ } /* for (i = 0; i < ci->children_num; i++) */
+
+ assert (host->host[0] != 0);
+ if (host->host[0] == 0)
+ {
+ ERROR ("Modbus plugin: Data block \"%s\": No type has been specified.",
+ host->host);
+ status = -1;
+ }
+
+ if (status == 0)
+ {
+ user_data_t ud;
+ char name[1024];
+ struct timespec interval;
+
+ ud.data = host;
+ ud.free_func = host_free;
+
+ ssnprintf (name, sizeof (name), "modbus-%s", host->host);
+
+ interval.tv_nsec = 0;
+ if (host->interval > 0)
+ interval.tv_sec = host->interval;
+ else
+ interval.tv_sec = 0;
+
+ plugin_register_complex_read (/* group = */ NULL, name,
+ mb_read, (interval.tv_sec > 0) ? &interval : NULL, &ud);
+ }
+ else
+ {
+ host_free (host);
+ }
+
+ return (status);
+} /* }}} int mb_config_add_device */
+
static int mb_config (oconfig_item_t *ci) /* {{{ */
{
int i;
@@ -832,6 +990,8 @@
mb_config_add_data (child);
else if (strcasecmp ("Host", child->key) == 0)
mb_config_add_host (child);
+ else if (strcasecmp ("Device", child->key) == 0)
+ mb_config_add_device (child);
else
ERROR ("Modbus plugin: Unknown configuration option: %s", child->key);
}
--- a/src/collectd.conf.in.orig 2011-03-26 18:08:53.000000000 +0200
+++ b/src/collectd.conf.in 2011-04-25 11:23:03.985686002 +0300
@@ -445,6 +445,19 @@
# Collect "data_name"
# </Slave>
# </Host>
+# <Device "name">
+# Device "/dev/serial"
+# Baud 9600
+# DataBits 8
+# StopBits 1
+# Parity "none"
+# Interval 60
+#
+# <Slave 1>
+# Instance "foobar" # optional
+# Collect "data_name"
+# </Slave>
+# </Device>
#</Plugin>
#<Plugin mysql>
--- a/src/collectd.conf.pod.orig 2011-03-26 18:08:53.000000000 +0200
+++ b/src/collectd.conf.pod 2011-04-25 11:23:03.981686001 +0300
@@ -1731,10 +1731,11 @@
=head2 Plugin C<modbus>
-The B<modbus plugin> connects to a Modbus "slave" via Modbus/TCP and reads
-register values. It supports reading single registers (unsigned 16E<nbsp>bit
-values), large integer values (unsigned 32E<nbsp>bit values) and floating point
-values (two registers interpreted as IEEE floats in big endian notation).
+The B<modbus plugin> connects to a Modbus "slave" via Modbus/TCP or Modbus/RTU
+and reads register values.
+It supports reading single registers (unsigned 16E<nbsp>bit values),
+large integer values (unsigned 32E<nbsp>bit values) and floating point values
+(two registers interpreted as IEEE floats in big endian notation).
Synopsis:
@@ -1764,6 +1765,21 @@
</Slave>
</Host>
+ <Device "SerialDevice1">
+ Device "/dev/ttyS1"
+ Baud 9600
+ DataBits 8
+ StopBits 1
+ Parity "none"
+ Interval 60
+
+ <Slave 1>
+ Instance "power-supply"
+ Collect "voltage-input-1"
+ Collect "voltage-input-2"
+ </Slave>
+ </Device>
+
=over 4
=item E<lt>B<Data> I<Name>E<gt> blocks
_______________________________________________
collectd mailing list
[email protected]
http://mailman.verplant.org/listinfo/collectd
