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