CoreTemp module #1664
CoreTemp module #1664
Conversation
read /sys/devices/platform/coretemp* provided by coretemp kernel module Config "MaxValues", // [ true ], only, none "ValuesPercentage", // [ true ], false "ValuesDegrees", // [ true ], false MaxValues Provides values only for the hottest core found. Useful if you have a lot cores but only need to see if there is problems, the single hottest core reading is good enough for that ValuesPercentage Reads the tjmax from coretemp and shows temperature/tjmax*100 rather then actual temperature ValuesDegrees Provides core value in degrees
configure.ac
Outdated
| @@ -5390,6 +5390,7 @@ plugin_numa="no" | |||
| plugin_perl="no" | |||
| plugin_processes="no" | |||
| plugin_protocols="no" | |||
| plugin_coretemp="no" | |||
There was a problem hiding this comment.
Please keep this list sorted.
indent -bli 0 src/coretemp.c
This reverts commit 27b9cbb.
|
cleaned up code via indent -bli 0 src/coretemp.c |
|
and fixed sorting in autoconf/automake config |
|
That made a mess of things. Please look over your changes again. You also missed some of my review comments, which are now gone. |
|
Would you be able to add standard set of parameters to run thru indent to get the code to comply I am sure it would help everyone and speed up the process of styling code. https://collectd.org/wiki/index.php/Coding_style ? I run it thru indent hopefully with sane options |
|
Apart from "If you're starting a new file, we recommend to indent with two spaces." we don't have many rules. Any decent editor can do that, I don't think it's worth the effort to codify this for indent. There's also the danger that your indent tool breaks things, as you can see from your last change. Please look at it again. This diff is just plain wrong. |
src/coretemp.c
Outdated
| }; | ||
|
|
||
| struct coretemp_core **c = 0; | ||
| unsigned int core_count = 0; | ||
| unsigned int core_count = 0; |
src/coretemp.c
Outdated
| socket = atoi (&de->d_name[CORETEMP_STRLEN]); | ||
| if (chdir (de->d_name)) | ||
| { | ||
| ERROR ("coretemp plugin: unable to chdir into [ %s ] for coretemp data", |
There was a problem hiding this comment.
spaces instead of tabs for this line and the next 3.
|
I went through the file for the fourth time now, but I give up. Please check everything once more before you resubmit. |
| char *label; | ||
| }; | ||
|
|
||
| struct coretemp_core **c = 0; |
There was a problem hiding this comment.
I commented on this already in my first review.
There's no need to initialise static variables, but if you insist in doing it anyway, please set pointers to NULL, not 0. Please mark as static while you're at it. Can you also pick a bit more descriptive name for a global variable than 'c'?
|
Sounds like an interesting plugin. What's missing to get it merged ? |
|
The plugin is interesting, but somebody needs to continue work on code so it meet project requirements and standards. After short review I found another flaws, such as incorrect use of |
Read coretemp values provided by the kernel into collectd
from /sys/devices/platform/coretemp*
Config
"MaxValues", // [ true ], only, none
"ValuesPercentage", // [ true ], false
"ValuesDegrees", // [ true ], false
MaxValues
Provides values only for the hottest core found. Useful if you
have a lot cores but only need to see if there is problems, the single
hottest core reading is good enough for that
ValuesPercentage
Reads the tjmax from coretemp and shows temperature/tjmax*100 rather then actual temperature
ValuesDegrees
Provides core value in degrees