[collectd] New plugin - mbus

[Florian Forster]

Hi Tomas,

On Mon, Nov 07, 2011 at 10:08:17PM +0100, Tomas wrote:
> I have made a new plugin for reading data from MBus devices.

thank you very much for your patch! :) It looks great, thank you very
much for also updating the manpage!

Please see my comments inline:

> +Only numerical records are supported and all are stored as GAUGE. The output structure is:
> +
> +  host                         = <hostname>
> +     plugin                    = "mbus"
> +        plugin_instance        = <MBus slave address>  e.g. 237483A847B34800
> +           value_type          = <Record quantity>     e.g. "Volume flow"
> +           value_type_instance = <Record number>       e.g. 1

Could you please document that MBus provides information on which data
type is submits (the quantity setting) and that this is used to chose a
"type" automatically? This wasn't obvious for me.

Regarding type handling, I'd prefer to have a "MBus type" to "collectd
type" map within the plugin, rather than relying on the library to
provide a string which we then massage to get something compatible to
collectd types. E.g.:

  "External_temperature" -> "temperature"
  "Response_delay_time"  -> "delay"
  "Dimensionless"        -> "gauge"
  "Bus_Address"          -> IGNORE

> +Synopsis of the configuration is:
> +
> + <Plugin mbus>
> +     IsSerial     true
> +     SerialDevice "/dev/ttyUSB0"
> +     Host         "none"
> +     Port         256

Please document ports as strings, i.e. 'Port "256"'. They are generally
called "services" and strings. You can use cf_util_get_port_number()
from configfile.h or service_name_to_port_number() from common.h to
convert a string to a number if needed.

> +     <Slave 59>
> +         IgnoreSelected true
> +         Record         2
> +         Record         5
> +         Record         6
> +     </Slave>
> +     <Slave 237483A847B34800>

I'd prefer to use '<Slave "237483A847B34800">'. If the argument looks
numeric (i.e. no A-F present), it will be parsed as a double, which may
cause trouble.

> +=item B<IsSerial> I<Boolean>

Can't we tell what the user wants from the presence of the
"SerialDevice" vs. "Host" settings?

> +Slave blocks define individual MBus slaves - devices t be read. B<Address> is either primary

t -> to

> --- src/mbus_utils.c	(.../collectd-5.0.1)	(revision 0)
> +++ src/mbus_utils.c	(.../branches/collectd-5.0.1_mbus)	(revision 14)

Why is a separate file necessary for these functions?

> +mbus_slave * mbus_slave_new(void)
> +{
> +    DEBUG("mbus: mbus_slave_new - creating new slave");
> +    mbus_slave * slave = (mbus_slave *) malloc(sizeof(mbus_slave));

Please use 'malloc (sizeof (*slave))'.

> +    memset(slave, 0, sizeof(mbus_slave));

Dito.

> Index: src/mbus.c
> ===================================================================
> --- src/mbus.c	(.../collectd-5.0.1)	(revision 0)
> +++ src/mbus.c	(.../branches/collectd-5.0.1_mbus)	(revision 14)
> @@ -0,0 +1,596 @@
> +#include "collectd.h"
> +#include "common.h"
> +#include "plugin.h"

You're missing a copyright and license header. Please copy the header
from a file that is licensed under the desired license and adapt it.

> +static pthread_mutex_t plugin_lock; /**< Global lock fof the MBus access */

I don't think this is necessary. The configure and init functions are
called before worker threads are created, i.e. these callbacks are
called sequentially. The read function must be thread-safe, but not
reentrant-safe, i.e. it is never called twice by two different threads.

> +/* Note that the MBus library is not thread safe. On the other hand given the MBus */
> +/* nature - bus, synchronous communication (i.e. we can have only one operation in */
> +/* progress and have ot wait till it is finished) this is not a problem here for us. */

How is the library not thread-safe? Does it use strtok(3) or strerror(3)
or similar non-thread-safe functions?

> +static _Bool conf_is_serial = -1;

_Bool can't hold -1, please use 0 or 1. (If you want to keep this
variable, see comment above.)

> +static int collectd_mbus_config_slave (oconfig_item_t *ci)
...
> +    if (ci->values_num == 1)
/* about 40 lines */
> +    else
> +    {
> +        ERROR("mbus: collectd_mbus_config_slave - missing or wrong slave address");
> +        mbus_slave_free(slave);
> +        return -1;
> +    }

Please use:
  if (ci->values_num != 1) { /* handle error */; return (-1); }

> +    /* first sort out the selection logic; last setting wins */
> +    for (i = 0; i < ci->children_num; i++)
> +    {
> +        child = ci->children + i;
> +        if ((strcasecmp ("IgnoreSelected", child->key) == 0) && (!cf_util_get_boolean (child, &ignore_selected)))

Please don't hide function calls, like the one to cf_util_get_boolean()
like this. Rather do:

for (i = 0; i < ci->children_num; i++)
{
    child = ci->children + i;
    if (strcasecmp ("IgnoreSelected", child->key) != 0)
      continue;

    cf_util_get_boolean (child, &ignore_selected);
    DEBUG("mbus: collectd_mbus_config_slave - IgnoreSelected = %d", ignore_selected);
}

> +static int collectd_mbus_config (oconfig_item_t *ci)
...
> +        if ((strcasecmp ("IsSerial", child->key) == 0) && (!cf_util_get_boolean (child, &conf_is_serial)))
> +            ;
> +        else if ((strcasecmp ("SerialDevice", child->key) == 0) && (!cf_util_get_string (child, &conf_device)))
> +            ;
> +        else if ((strcasecmp ("Host", child->key) == 0) && (!cf_util_get_string (child, &conf_host)))
> +            ;
> +        else if ((strcasecmp ("Port", child->key) == 0) && (!cf_util_get_int (child, &conf_port)))
> +            ;
> +        else if ((strcasecmp ("Slave", child->key) == 0) && (!collectd_mbus_config_slave (child)))
> +            ;
> +        else
> +            WARNING ("mbus: collectd_mbus_config - unknown config option or unsupported config value: %s", child->key);

Dito:

if (strcasecmp ("IsSerial", child->key) == 0)
  cf_util_get_boolean (child, &conf_is_serial);
else if ...

This will print a warning if the value couldn't be handled (incorrect
type, for example), so no need to print anything here.

> +static int parse_and_submit (mbus_slave * slave, mbus_frame * frame)
...
> +    if (frame_data.type == MBUS_DATA_TYPE_FIXED)
> +    {
/* ca. 110 lines */
> +    }
> +    else
> +    {
> +        if (frame_data.type == MBUS_DATA_TYPE_VARIABLE)
> +        {
/* ca. 65 lines */
> +        }
> +    }

Can you please put this logic in separate functions? The huge if/else
construct is hard to read.

> +        tmp_int = mbus_data_bcd_decode(data->id_bcd, 4);

What does this 4 do? Please document this inline like:
  tmp_int = mbus_data_bcd_decode(data->id_bcd, /* bytes in int = */ 4);

> +        if(mbus_slave_record_check(slave,0))
> +        {
/* ca. 40 lines */
> +        }
> +        else
> +        {
> +            DEBUG("mbus: parse_and_submit -   Record #0 disabled by mask");
> +        }

Once this is in a separate function, use:

if (!mbus_slave_record_check(slave,0))
{
  DEBUG ("... disabled ...");
  return (0);
}

> +            if(NULL != (record = mbus_parse_fixed_record(data->status, data->cnt1_type, data->cnt1_val)))

Please don't put function calls into if conditions (functions like
strcmp are an exception, of course). Instead, do:

record = mbus_parse_fixed_record(data->status, data->cnt1_type, data->cnt1_val);
if (record == NULL)
{
  /* error handling */
  return (-1);
}
/* normal code */

> +                    if(record->is_numeric)
> +                        values[0].gauge = record->value.real_val;
> +                    else
> +                        values[0].gauge = 0;

Please use NAN if the value is not numeric.

Thanks again and best regards,
—octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x0C705A15
http://octo.it/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.verplant.org/pipermail/collectd/attachments/20111201/831c428f/attachment.pgp>