[collectd] [PATCH 3/5] sigrok: Multiple device sections, and working acquisition

[Florian Forster]

Hi again,

well, seing how the config function was rewritten, I think a squished
together patch might actually make more sense. If you have a Github
account, opening a Pull Request would also work, since I can comment on
the overall changes, not just indivitual patches.

On Mon, Jul 22, 2013 at 06:21:19PM +0200, Bert Vermeulen wrote:
> +static int plugin_config_device(oconfig_item_t *ci)
>  {
> …
> +		if (!strcasecmp(item->key, "driver")) {
> +			cfdev->driver = strdup(item->values[0].value.string);

Please use cf_util_get_string() from src/configfile.h for string
arguments.

> +		} else if (!strcasecmp(item->key, "interval")) {
> +			cfdev->min_write_interval = item->values[0].value.number;

Please use cf_util_get_cdtime() from src/configfile.h for this.

> +static int elapsed_time(struct timespec *last_write, struct timespec *elapsed)

Please use cdtime_t from src/utils_time.h; you can simply do a 

  cdtime_t elapsed = now - previous;

with that time representation.

> +	if (packet->type == SR_DF_END) {
> +		/* TODO: try to restart acquisition after a delay? */
> +		INFO("oops! ended");

There's a specialized logginf function in the code. Why not use it here?

> +	if (!cfdev) {
> +		ERROR("Unknown device instance in sigrok driver %s.", sdi->driver->name);

Dito.

> +		if (elapsed_time(&cfdev->last_write, &elapsed) != 0)
> +			return;
> +		if (elapsed.tv_sec < cfdev->min_write_interval)
> +			return;

Can't you use the timing code from plugin.c? For example, can't you
register one read callback for each device? Then you don't have to care
about the interval at all.

> +			ERROR("malloc() failed.");

See above.

> +		if (clock_gettime(CLOCK_REALTIME, &cfdev->last_write) != 0) {

Please use cdtime() or, preferably, let the plugin infrastructure handle
all the timing.

>  static int plugin_init(void)

The "plugin_" prefix is reserved for functions defined in src/plugin.h.
Maybe you can use "csr_" or something like that instead?

> +		/* Do this only when we're sure there's hardware to talk to. */
> +		ts.tv_sec = 0;
> +		ts.tv_nsec = 1000000L;
> +		plugin_register_complex_read("sigrok", "sigrok", plugin_read,
> +				&ts, NULL);

This is fine, if you register one read callback for each device you
find. As it is, I would simply register this in module_register() below.
If the user loaded this plugin and no devices are found, something is
usually wrong.

Best regards,
—octo
-- 
collectd – The system statistics collection daemon
Website: http://collectd.org
Google+: http://collectd.org/+
GitHub:  https://github.com/collectd
Twitter: http://twitter.com/collectd
-------------- 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/20130722/716c479b/attachment-0001.pgp>