[collectd] [PATCH] added TED "The energy detective" plugin

[Florian Forster]

Hi Eric,

thank you very much for your patch :)

I've found a few small problems though, maybe you could fix them:

On Sun, Mar 01, 2009 at 11:21:22AM -0600, edreed at reedhome.net wrote:
> From: user <user at ubuntu810desktop.localdomain>

Apparently you didn't set your email address in the Git configuration.

> diff --git a/configure.in b/configure.in
[...]
> -AC_PLUGIN([java],        [$with_java],         [Embed the Java Virtual Machine])

This looks like you did a `diff master..HEAD' without rebasing the
current branch first. If this is too much Git magic, don't worry about
it, it's probably easy to figure out. In that case, the output of
  $ git merge-base master HEAD
would be interesting though.

> diff --git a/src/ted.c b/src/ted.c
> + * collectd - src/ted.c
> + * Copyright (C) 2005,2006  Peter Holik

Could you please add a copyright message for yourself here?

> +#define LINE_LENGTH 282
> +#define PKT_REQUEST  "\xAA"

These defines are never used. Did you forget to use them? If not, please
remove them.

> +#define CLIENT_LIST_PREFIX  "CLIENT_LIST,"

Dito.

> +static int ted_read_value(double *kv, double *voltage)
[...]
> +			if (status > 0) /* usually we succeed */

Could you please move the error handling up? This way, we can save a
level of indentation which makes this easier to read.

What I mean is, instead of this:
-- 8< --
  if (succes)
  {
    /* much code */
  }
  else if (some error)
  {
    print message;
    continue;
  }
  else if (serious error)
  {
    print message;
    break;
  }
-- >8 --

Do this instead:
-- 8< --
  if (some error)
  {
    print message;
    continue;
  }
  else if (serious error)
  {
    print message;
    break;
  }
  /* much code */
-- >8 --

This way the decision (the if-statements) are much closer to the source
of the (potential) error.

> +				if (package_length == 279)
> +				{
> +                                    *kv = ((package_buffer[248] * 256) + package_buffer[247])*10.0;
> +                                    INFO ("kv %f",*kv);
> +                                    *voltage = ((package_buffer[252] * 256) + package_buffer[251])/10.0;
> +                                    INFO ("voltage %f",*voltage);
> +                                    return (0); /* value received */
> +                                }
> +                                else
> +                                    INFO ("Not the correct package");
> +                                    usleep(700000);
> +                                    continue;
> +                                    //return (-1); /* Not pro package */

Did you miss brackets here? If not, please use another indentation level
to show that `usleep' is *supposed* to be outside the `else' block.

Why do you call `usleep' here? And where does `700000' come from?

Please don't print `INFO' messages inside `read' functions. They are
printed very often and are in most cases not relevant. It's probably
easiest to change that to `DEBUG'.

> +        char sCmd[1];

Could you use a more descriptive variable name here and AvoidWobblyCase?
Something like `init_cmd'? Ideally something like this:
-- 8< --
  char init_cmd[] = { 0xaa };
  ...
  status = swrite (fd, init_cmd, sizeof (init_cmd));
-- >8 --

> +		if ((fd = open(device, O_RDWR | O_NOCTTY | O_NDELAY | O_NONBLOCK)) > 0)

Another case of ``if (success) { /* very long block */ }''. Please check
for an error condition and beak, return, continue, whatever when an
error occurred. This is much easier to read.

> +                        usleep(900000);

Another mysterious sleep with constant. Please add a comment describing
why you sleep there and why for this period of time.

> +	sstrncpy (vl.type, "ted", sizeof (vl.type));

I think using `voltage' as type is better here.

> +	ted_submit ("kv", kv);	
> +        ted_submit ("voltage", voltage);

What's `kv'? Sounds like `kilo volts' to me..? In that case it'd be best
to do:
  ted_submit ((1000.0 * kv) + voltage);
(And hardcode the type `voltage' in `ted_submit'.

Thanks again for your patch, I'm looking forward to monitor some power
consumption :)

Regards,
-octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20090301/b83c3b40/attachment.pgp