[collectd] [PATCHv2] support for Modbus/RTU

[Daniel Golle]

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 <octo at collectd.org> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: collectd-4.10.3-modbus-rtu.patch
Type: text/x-patch
Size: 8217 bytes
Desc: not available
URL: <http://mailman.verplant.org/pipermail/collectd/attachments/20110425/a2e4d824/attachment.bin>