[collectd] [PATCH] support for Modbus/RTU

[Florian Forster]

Hi Daniel,

thank you very much for your patch!

On Mon, Mar 28, 2011 at 08:20:59PM +0200, Daniel Golle wrote:
>  struct mb_host_s /* {{{ */
>  {
> […]
> +#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.

> +static inline int mb_init_connection_tcp (mb_host_t *host) /* {{{ */

Please don't use the "inline" keyword. The compilers do a pretty good
job at determining which functions to inline and are allowed to ignore
this keyword, making it kind of pointless.

> +  if (host->baud < 300)
> +    host->baud = 9600;
> +
> +  if ((host->data_bits < 7) || (host->data_bits > 16))
> +    host->data_bits = 8;
> +
> +  if ((host->stop_bits < 1) || (host->stop_bits > 16))
> +    host->stop_bits = 1;

Please add short (one line) comments explaining where this magic comes
from. Why are 42 stop-bits reduced to one? Why are four data bits
increased to eight?

Some of these conditions are handled in the config handling already --
maybe they could be replaced by an assertion. For example:

  assert (host->data_bits >= 8);

Of course, this only holds true if such a config option actually exists.
What happends if one passes a zero to libmodbus?

> +  if ( ! (strcmp(host->parity, "even") || strcmp(host->parity, "odd") || strcmp(host->parity, "none")))

Please write this as:

  if ((strcmp ("even", host->parity) != 0)
      && (strcmp ("odd", "host->parity) != 0)
      && (strcmp ("none", "host->parity) != 0))

> +    memcpy(host->parity, "none", sizeof("none"));

Please use instead:

  sstrncpy (host->parity, "none", sizeof (host->parity));

> +      /* 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.

> +    else if ( host->type_com == RTU )

I don't see where this member is ever set to "RTU".

> +    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.

> +    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.

Best regards,
—octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x0C705A15
http://octo.it/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
URL: <http://mailman.verplant.org/pipermail/collectd/attachments/20110415/fdfc6f18/attachment.pgp>