[collectd] New plugin - tempernet

[Florian Forster]

Hi Nicolas,

On Tue, Aug 10, 2010 at 07:43:19PM +1000, Nicolas Guillaumin wrote:
> I just wrote a new plugin to collect temperature readings from a cheap
> TemperNET USB sensor (
> http://www.pcsensor.com/index.php?_a=viewProd&productId=15 ).
> Please find attached the patch file + temper.c plugin source. The
> patch is against release v4.10.1

thank you very much for your patch :)

> Usual warning applies ;-)
> - I've not coded in C since my student years,

And I have a couple of requests for changes ;) Namely:

  * If you pass a numeric argument to a function directly, i.e. without
    assigning it to a variable first, please add the name of the
    argument in the call. For example, instead of "foo (1, 0);" do
    something like "foo (/* index = */ 1, /* flags = */ 0);". This makes
    calls such as
      usb_control_msg(sensor->handle, 0x21, 9, 0x200, 0x01, (char *) buf, 32, USB_TIMEOUT);
    readable.

  * In the above call, rather than passing "32" (presumable the size of
    the buffer) use "sizeof (buf)". Same same applies to other functions
    operating on the buffer, for example
    "memset (buf, 0, sizeof (buf));" [*]

  * Speaking of which: "bzero" is a BSD extension. Please use "memset"
    instead.

  * You only ever allocate one instance of "struct tempernet". It'd be
    easier to program and read if you'd simply use two static variables
    instead of the struct.

  * If you do allocate memory, please check the return value of "malloc"
    and friends.

  * You seem to copy several functions verbatim from the TEMPer driver
    by Robert Kavaler (linked in the header of the file). You should
    therefore list him and yourself as copyright holders. Also,
    originally the code is not under the GPL but some sort of 1-clause
    BSD license. Please do one of the following:

    * Provide your code under the same license.
    * Make it clear which parts of the plugin are GPL and which are the
      original license.
    * Contact the original author whether he agrees with the relicensing
      of his code. Please CC me or the collectd mailing list if possible.

Sorry this has become such a long list ;) I think that all the problems
are easy to fix, though.

Please consider to adopt the following coding style, though:
-- 8< --
  for (many iterations)
  {
    if (!condition)
    {
      error handling;
      continue;
    }
    
    many;
    more;
    code;
  }
-- >8 --

This is much easier on the eye than:
-- 8< --
  for (many iterations)
  {
    if (condition)
    {
      many;
      more;
      code;
    }
    else
    {
      error handling;
    }
  }
-- >8 --
  
> - I've never used autoconf/automake before.

Don't worry about it, I'll take care of that.

Regards,
—octo

[*] "memset" is such a common function that passing "0" as the second
    argument without description is okay as a special exception ;)
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x0C705A15
http://octo.it/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
URL: <http://mailman.verplant.org/pipermail/collectd/attachments/20100911/612b75eb/attachment.pgp>