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/
signature.asc
Description: Digital signature
_______________________________________________ collectd mailing list collectd@verplant.org http://mailman.verplant.org/listinfo/collectd