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