Add support for new attributes to

Add support for new sysfs attributes to libsensors and to the sensors command.

v2: Use defines for array sizes. Some of the resulting arrays are larger than necessary,
but that is better than missing an array size update if the number of attributes changes.

(quoted diff) +5 -0

+  enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_LOWEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_HIGHEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_AVERAGE
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_LOWEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_HIGHEST
 * Added support for intrusion detection
   enum sensors_feature_type SENSORS_FEATURE_INTRUSION
   enum sensors_subfeature_type SENSORS_SUBFEATURE_INTRUSION_ALARM

Index: lib/sensors.h
=================================> --- lib/sensors.h (revision 5996)
+++ lib/sensors.h (working copy)

(quoted diff) +9 -0

@@ -157,6 +157,9 @@
 	SENSORS_SUBFEATURE_IN_MAX,
 	SENSORS_SUBFEATURE_IN_LCRIT,
 	SENSORS_SUBFEATURE_IN_CRIT,
+	SENSORS_SUBFEATURE_IN_AVERAGE,
+	SENSORS_SUBFEATURE_IN_LOWEST,
+	SENSORS_SUBFEATURE_IN_HIGHEST,
 	SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
 	SENSORS_SUBFEATURE_IN_MIN_ALARM,
 	SENSORS_SUBFEATURE_IN_MAX_ALARM,

@@ -181,6 +184,9 @@
 	SENSORS_SUBFEATURE_TEMP_LCRIT,
 	SENSORS_SUBFEATURE_TEMP_EMERGENCY,
 	SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST,
+	SENSORS_SUBFEATURE_TEMP_AVERAGE,
+	SENSORS_SUBFEATURE_TEMP_LOWEST,
+	SENSORS_SUBFEATURE_TEMP_HIGHEST,
 	SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
 	SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
 	SENSORS_SUBFEATURE_TEMP_MIN_ALARM,

@@ -215,6 +221,9 @@
 	SENSORS_SUBFEATURE_CURR_MAX,
 	SENSORS_SUBFEATURE_CURR_LCRIT,
 	SENSORS_SUBFEATURE_CURR_CRIT,
+	SENSORS_SUBFEATURE_CURR_AVERAGE,
+	SENSORS_SUBFEATURE_CURR_LOWEST,
+	SENSORS_SUBFEATURE_CURR_HIGHEST,
 	SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
 	SENSORS_SUBFEATURE_CURR_MIN_ALARM,
 	SENSORS_SUBFEATURE_CURR_MAX_ALARM,

Index: lib/sysfs.c
=================================> --- lib/sysfs.c (revision 5996)
+++ lib/sysfs.c (working copy)

(quoted diff) +9 -0

@@ -233,6 +233,9 @@
 	{ "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
 	{ "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
 	{ "emergency_hyst", SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST },
+	{ "average", SENSORS_SUBFEATURE_TEMP_AVERAGE },
+	{ "lowest", SENSORS_SUBFEATURE_TEMP_LOWEST },
+	{ "highest", SENSORS_SUBFEATURE_TEMP_HIGHEST },
 	{ "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
 	{ "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
 	{ "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },

@@ -252,6 +255,9 @@
 	{ "max", SENSORS_SUBFEATURE_IN_MAX },
 	{ "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
 	{ "crit", SENSORS_SUBFEATURE_IN_CRIT },
+	{ "average", SENSORS_SUBFEATURE_IN_AVERAGE },
+	{ "lowest", SENSORS_SUBFEATURE_IN_LOWEST },
+	{ "highest", SENSORS_SUBFEATURE_IN_HIGHEST },
 	{ "alarm", SENSORS_SUBFEATURE_IN_ALARM },
 	{ "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
 	{ "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },

@@ -302,6 +308,9 @@
 	{ "max", SENSORS_SUBFEATURE_CURR_MAX },
 	{ "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
 	{ "crit", SENSORS_SUBFEATURE_CURR_CRIT },
+	{ "average", SENSORS_SUBFEATURE_CURR_AVERAGE },
+	{ "lowest", SENSORS_SUBFEATURE_CURR_LOWEST },
+	{ "highest", SENSORS_SUBFEATURE_CURR_HIGHEST },
 	{ "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
 	{ "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
 	{ "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },

Index: prog/sensors/chips.c
=================================> --- prog/sensors/chips.c (revision 5996)
+++ prog/sensors/chips.c (working copy)

(quoted diff) +21 -4

@@ -280,15 +280,26 @@
 	{ SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, 0, "crit" },
 	{ SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, 0,
 	    "emerg" },
+	{ SENSORS_SUBFEATURE_TEMP_AVERAGE, NULL, 0, "avg" },
+	{ SENSORS_SUBFEATURE_TEMP_LOWEST, NULL, 0, "lowest" },
+	{ SENSORS_SUBFEATURE_TEMP_HIGHEST, NULL, 0, "highest" },
 	{ -1, NULL, 0, NULL }
 };
 
+#define NUM_TEMP_ALARMS		6
+#define NUM_TEMP_SENSORS	(ARRAY_SIZE(temp_sensors) \
+				 + ARRAY_SIZE(temp_max_sensors) \
+				 + ARRAY_SIZE(temp_crit_sensors) \
+				 + ARRAY_SIZE(temp_emergency_sensors) \
+				 - NUM_TEMP_ALARMS)
+				
+
 static void print_chip_temp(const sensors_chip_name *name,
 			    const sensors_feature *feature,
 			    int label_size)
 {
-	struct sensor_subfeature_data sensors[8];
-	struct sensor_subfeature_data alarms[5];
+	struct sensor_subfeature_data sensors[NUM_TEMP_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_TEMP_ALARMS];
 	int sensor_count, alarm_count;
 	const sensors_subfeature *sf;
 	double val;

@@ -365,17 +376,23 @@
 	{ SENSORS_SUBFEATURE_IN_MIN, NULL, 0, "min" },
 	{ SENSORS_SUBFEATURE_IN_MAX, NULL, 0, "max" },
 	{ SENSORS_SUBFEATURE_IN_CRIT, NULL, 0, "crit max" },
+	{ SENSORS_SUBFEATURE_IN_AVERAGE, NULL, 0, "avg" },
+	{ SENSORS_SUBFEATURE_IN_LOWEST, NULL, 0, "lowest" },
+	{ SENSORS_SUBFEATURE_IN_HIGHEST, NULL, 0, "highest" },
 	{ -1, NULL, 0, NULL }
 };
 
+#define NUM_IN_ALARMS	5
+#define NUM_IN_SENSORS	(ARRAY_SIZE(voltage_sensors) - NUM_IN_ALARMS)
+
 static void print_chip_in(const sensors_chip_name *name,
 			  const sensors_feature *feature,
 			  int label_size)
 {
 	const sensors_subfeature *sf;
 	char *label;
-	struct sensor_subfeature_data sensors[4];
-	struct sensor_subfeature_data alarms[4];
+	struct sensor_subfeature_data sensors[NUM_IN_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_IN_ALARMS];
 	int sensor_count, alarm_count;
 	double val;
 

(quoted diff) +11 -4

+
 static void print_chip_power(const sensors_chip_name *name,
 			     const sensors_feature *feature,
 			     int label_size)
 {
 	double val;
 	const sensors_subfeature *sf;
-	struct sensor_subfeature_data sensors[6];
-	struct sensor_subfeature_data alarms[3];
+	struct sensor_subfeature_data sensors[NUM_POWER_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_POWER_ALARMS];
 	int sensor_count, alarm_count;
 	char *label;
 	const char *unit;

@@ -653,9 +677,15 @@
 	{ SENSORS_SUBFEATURE_CURR_MIN, NULL, 0, "min" },
 	{ SENSORS_SUBFEATURE_CURR_MAX, NULL, 0, "max" },
 	{ SENSORS_SUBFEATURE_CURR_CRIT, NULL, 0, "crit max" },
+	{ SENSORS_SUBFEATURE_CURR_AVERAGE, NULL, 0, "avg" },
+	{ SENSORS_SUBFEATURE_CURR_LOWEST, NULL, 0, "lowest" },
+	{ SENSORS_SUBFEATURE_CURR_HIGHEST, NULL, 0, "highest" },
 	{ -1, NULL, 0, NULL }
 };
 
+#define NUM_CURR_ALARM		5
+#define NUM_CURR_SENSORS	(ARRAY_SIZE(current_sensors) - NUM_CURR_ALARM)
+
 static void print_chip_curr(const sensors_chip_name *name,
 			    const sensors_feature *feature,
 			    int label_size)

@@ -663,8 +693,8 @@
 	const sensors_subfeature *sf;
 	double val;
 	char *label;
-	struct sensor_subfeature_data sensors[4];
-	struct sensor_subfeature_data alarms[4];
+	struct sensor_subfeature_data sensors[NUM_CURR_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_CURR_ALARM];
 	int sensor_count, alarm_count;
 
 	if (!(label = sensors_get_label(name, feature))) {

Index: CHANGES
=================================> --- CHANGES (revision 5996)
+++ CHANGES (working copy)

(quoted diff) +0 -0

   sensors-detect: Stop calling for PIIX5 SMBus testers
                   Improve filtering of fake DMI data
                   Print DMI system/product version if available

Hi Guenter,

Sorry for the late review.

Quoted 5 lines · Guenter Roeck

Add support for new sysfs attributes to libsensors and to the sensors command.

v2: Use defines for array sizes. Some of the resulting arrays are larger than necessary,
but that is better than missing an array size update if the number of attributes changes.

I don't get the logic. I understand the point of computing the sizes automatically to avoid an overflow [1], but then why don't you compute the right values? It's only a matter of adding a "- 1" after every call to ARRAY_SIZE(), this seems fairly easy and cheap.

Or maybe you were referring to the fact that some attributes are mutually exclusive per Documentation/hwmon/sysfs-interface (in particular, general alarm flag vs. limit-specific alarm flags.) It was probably a bad idea to rely on this in the first place anyway, so I have no objection in changing this.

[1] Note that the overflows couldn't really happen, BTW, as we have code to catch them in get_sensor_limit_data(). Maybe these checks can go away if the array sizes are now guaranteed to be sufficient by construction?

This should really be a separate patch, BTW, as this is not related to the original purpose of the patch.

Quoted 6 lines

Index: doc/libsensors-API.txt
=================================> > --- doc/libsensors-API.txt (revision 5996)
+++ doc/libsensors-API.txt (working copy)

(quoted diff) +0 -0

@@ -7,6 +7,16 @@
 given new feature.
 

Review line 14

Quoted 6 lines

(quoted diff) +5 -0

 0x431	lm-sensors 3.3.0 to 3.3.1
+* Added support for new sysfs attributes
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_AVERAGE
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_LOWEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_HIGHEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_AVERAGE

I don't see this one in Documentation/hwmon/sysfs-interface.

Quoted 135 lines

(quoted diff) +5 -0

+  enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_LOWEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_HIGHEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_AVERAGE
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_LOWEST
+  enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_HIGHEST
 * Added support for intrusion detection
   enum sensors_feature_type SENSORS_FEATURE_INTRUSION
   enum sensors_subfeature_type SENSORS_SUBFEATURE_INTRUSION_ALARM

Index: lib/sensors.h
=================================> > --- lib/sensors.h (revision 5996)
+++ lib/sensors.h (working copy)

(quoted diff) +9 -0

@@ -157,6 +157,9 @@
 	SENSORS_SUBFEATURE_IN_MAX,
 	SENSORS_SUBFEATURE_IN_LCRIT,
 	SENSORS_SUBFEATURE_IN_CRIT,
+	SENSORS_SUBFEATURE_IN_AVERAGE,
+	SENSORS_SUBFEATURE_IN_LOWEST,
+	SENSORS_SUBFEATURE_IN_HIGHEST,
 	SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
 	SENSORS_SUBFEATURE_IN_MIN_ALARM,
 	SENSORS_SUBFEATURE_IN_MAX_ALARM,

@@ -181,6 +184,9 @@
 	SENSORS_SUBFEATURE_TEMP_LCRIT,
 	SENSORS_SUBFEATURE_TEMP_EMERGENCY,
 	SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST,
+	SENSORS_SUBFEATURE_TEMP_AVERAGE,
+	SENSORS_SUBFEATURE_TEMP_LOWEST,
+	SENSORS_SUBFEATURE_TEMP_HIGHEST,
 	SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
 	SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
 	SENSORS_SUBFEATURE_TEMP_MIN_ALARM,

@@ -215,6 +221,9 @@
 	SENSORS_SUBFEATURE_CURR_MAX,
 	SENSORS_SUBFEATURE_CURR_LCRIT,
 	SENSORS_SUBFEATURE_CURR_CRIT,
+	SENSORS_SUBFEATURE_CURR_AVERAGE,
+	SENSORS_SUBFEATURE_CURR_LOWEST,
+	SENSORS_SUBFEATURE_CURR_HIGHEST,
 	SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
 	SENSORS_SUBFEATURE_CURR_MIN_ALARM,
 	SENSORS_SUBFEATURE_CURR_MAX_ALARM,

Index: lib/sysfs.c
=================================> > --- lib/sysfs.c (revision 5996)
+++ lib/sysfs.c (working copy)

(quoted diff) +9 -0

@@ -233,6 +233,9 @@
 	{ "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
 	{ "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
 	{ "emergency_hyst", SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST },
+	{ "average", SENSORS_SUBFEATURE_TEMP_AVERAGE },
+	{ "lowest", SENSORS_SUBFEATURE_TEMP_LOWEST },
+	{ "highest", SENSORS_SUBFEATURE_TEMP_HIGHEST },
 	{ "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
 	{ "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
 	{ "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },

@@ -252,6 +255,9 @@
 	{ "max", SENSORS_SUBFEATURE_IN_MAX },
 	{ "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
 	{ "crit", SENSORS_SUBFEATURE_IN_CRIT },
+	{ "average", SENSORS_SUBFEATURE_IN_AVERAGE },
+	{ "lowest", SENSORS_SUBFEATURE_IN_LOWEST },
+	{ "highest", SENSORS_SUBFEATURE_IN_HIGHEST },
 	{ "alarm", SENSORS_SUBFEATURE_IN_ALARM },
 	{ "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
 	{ "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },

@@ -302,6 +308,9 @@
 	{ "max", SENSORS_SUBFEATURE_CURR_MAX },
 	{ "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
 	{ "crit", SENSORS_SUBFEATURE_CURR_CRIT },
+	{ "average", SENSORS_SUBFEATURE_CURR_AVERAGE },
+	{ "lowest", SENSORS_SUBFEATURE_CURR_LOWEST },
+	{ "highest", SENSORS_SUBFEATURE_CURR_HIGHEST },
 	{ "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
 	{ "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
 	{ "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },

Index: prog/sensors/chips.c
=================================> > --- prog/sensors/chips.c (revision 5996)
+++ prog/sensors/chips.c (working copy)

(quoted diff) +21 -4

@@ -280,15 +280,26 @@
 	{ SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, 0, "crit" },
 	{ SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, 0,
 	    "emerg" },
+	{ SENSORS_SUBFEATURE_TEMP_AVERAGE, NULL, 0, "avg" },
+	{ SENSORS_SUBFEATURE_TEMP_LOWEST, NULL, 0, "lowest" },
+	{ SENSORS_SUBFEATURE_TEMP_HIGHEST, NULL, 0, "highest" },
 	{ -1, NULL, 0, NULL }
 };
 
+#define NUM_TEMP_ALARMS		6
+#define NUM_TEMP_SENSORS	(ARRAY_SIZE(temp_sensors) \
+				 + ARRAY_SIZE(temp_max_sensors) \
+				 + ARRAY_SIZE(temp_crit_sensors) \
+				 + ARRAY_SIZE(temp_emergency_sensors) \
+				 - NUM_TEMP_ALARMS)
+				
+
 static void print_chip_temp(const sensors_chip_name *name,
 			    const sensors_feature *feature,
 			    int label_size)
 {
-	struct sensor_subfeature_data sensors[8];
-	struct sensor_subfeature_data alarms[5];
+	struct sensor_subfeature_data sensors[NUM_TEMP_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_TEMP_ALARMS];
 	int sensor_count, alarm_count;
 	const sensors_subfeature *sf;
 	double val;

@@ -365,17 +376,23 @@
 	{ SENSORS_SUBFEATURE_IN_MIN, NULL, 0, "min" },
 	{ SENSORS_SUBFEATURE_IN_MAX, NULL, 0, "max" },
 	{ SENSORS_SUBFEATURE_IN_CRIT, NULL, 0, "crit max" },
+	{ SENSORS_SUBFEATURE_IN_AVERAGE, NULL, 0, "avg" },
+	{ SENSORS_SUBFEATURE_IN_LOWEST, NULL, 0, "lowest" },
+	{ SENSORS_SUBFEATURE_IN_HIGHEST, NULL, 0, "highest" },
 	{ -1, NULL, 0, NULL }
 };
 
+#define NUM_IN_ALARMS	5
+#define NUM_IN_SENSORS	(ARRAY_SIZE(voltage_sensors) - NUM_IN_ALARMS)
+
 static void print_chip_in(const sensors_chip_name *name,
 			  const sensors_feature *feature,
 			  int label_size)
 {
 	const sensors_subfeature *sf;
 	char *label;
-	struct sensor_subfeature_data sensors[4];
-	struct sensor_subfeature_data alarms[4];
+	struct sensor_subfeature_data sensors[NUM_IN_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_IN_ALARMS];
 	int sensor_count, alarm_count;
 	double val;
 

@@ -504,6 +521,7 @@
 };
 

Review line 524

Quoted 2 lines

(quoted diff) +1 -0

 static const struct sensor_subfeature_list power_inst_sensors[] = {
+	{ SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },

I'm confused. This one was missing on purpose. For power sensors we consider instantaneous and averaging sensors as different types. The code in function print_chip_power is pretty clear about this. So, just as all _INPUT subfeatures aren't listed in limit arrays, SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.

Quoted 7 lines

(quoted diff) +0 -0

 	{ SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" },
 	{ SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" },
 	{ -1, NULL, 0, NULL }

@@ -517,14 +535,20 @@
 	{ -1, NULL, 0, NULL }
 };
 

Review line 542

Quoted 5 lines

(quoted diff) +5 -0

+#define NUM_POWER_ALARMS	4
+#define NUM_POWER_SENSORS	(ARRAY_SIZE(power_common_sensors) \
+				 + ARRAY_SIZE(power_inst_sensors) \
+				 + ARRAY_SIZE(power_avg_sensors) \
+				 - NUM_POWER_ALARMS)

Due to the specificity of the power sensors described above, the computed value is larger than needed. We'll use power_inst_sensors or power_avg_sensors but never both. You could define a MAX() macro to select the right one for to compute NUM_POWER_SENSORS.

If you want to change the code to display both types, this can be discussed, but please keep the definition of NUM_POWER_SENSORS in line with the code that uses it, for clarity.

Quoted 47 lines

(quoted diff) +11 -4

+
 static void print_chip_power(const sensors_chip_name *name,
 			     const sensors_feature *feature,
 			     int label_size)
 {
 	double val;
 	const sensors_subfeature *sf;
-	struct sensor_subfeature_data sensors[6];
-	struct sensor_subfeature_data alarms[3];
+	struct sensor_subfeature_data sensors[NUM_POWER_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_POWER_ALARMS];
 	int sensor_count, alarm_count;
 	char *label;
 	const char *unit;

@@ -653,9 +677,15 @@
 	{ SENSORS_SUBFEATURE_CURR_MIN, NULL, 0, "min" },
 	{ SENSORS_SUBFEATURE_CURR_MAX, NULL, 0, "max" },
 	{ SENSORS_SUBFEATURE_CURR_CRIT, NULL, 0, "crit max" },
+	{ SENSORS_SUBFEATURE_CURR_AVERAGE, NULL, 0, "avg" },
+	{ SENSORS_SUBFEATURE_CURR_LOWEST, NULL, 0, "lowest" },
+	{ SENSORS_SUBFEATURE_CURR_HIGHEST, NULL, 0, "highest" },
 	{ -1, NULL, 0, NULL }
 };
 
+#define NUM_CURR_ALARM		5
+#define NUM_CURR_SENSORS	(ARRAY_SIZE(current_sensors) - NUM_CURR_ALARM)
+
 static void print_chip_curr(const sensors_chip_name *name,
 			    const sensors_feature *feature,
 			    int label_size)

@@ -663,8 +693,8 @@
 	const sensors_subfeature *sf;
 	double val;
 	char *label;
-	struct sensor_subfeature_data sensors[4];
-	struct sensor_subfeature_data alarms[4];
+	struct sensor_subfeature_data sensors[NUM_CURR_SENSORS];
+	struct sensor_subfeature_data alarms[NUM_CURR_ALARM];
 	int sensor_count, alarm_count;
 
 	if (!(label = sensors_get_label(name, feature))) {

Index: CHANGES
=================================> > --- CHANGES (revision 5996)
+++ CHANGES (working copy)

(quoted diff) +0 -0

@@ -2,6 +2,10 @@
 -----------------------
 

Review line 8

Quoted 5 lines

(quoted diff) +4 -0

 SVN HEAD
+  libsensors: Added support for new sysfs attributes
+  sensors: Added support for new sysfs attributes
+           For power sensors, display both instantaneous and average data
+           if available

Your patch doesn't actually implement that second change. Which is good, BTW, this unrelated change would really belong to a separate patch.

Hi Guenter,

Review

Quoted 3 lines

static const struct sensor_subfeature_list power_inst_sensors[] = {

(quoted diff) +1 -0

+	{ SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },

I'm confused. This one was missing on purpose. For power sensors we consider instantaneous and averaging sensors as different types. The code in function print_chip_power is pretty clear about this. So, just as all _INPUT subfeatures aren't listed in limit arrays, SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.

Hi Jean,

Quoted 2 lines · Jean Delvare

Hi Guenter,

[ ... ]

Quoted 10 lines

Review

Quoted 3 lines

static const struct sensor_subfeature_list power_inst_sensors[] = {

(quoted diff) +1 -0

+	{ SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },

I'm confused. This one was missing on purpose. For power sensors we consider instantaneous and averaging sensors as different types. The code in function print_chip_power is pretty clear about this. So, just as all _INPUT subfeatures aren't listed in limit arrays, SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.

I looked this up again.

The idea was to support display of both instantaneous power and average power if both is provided by the chip/driver.

With the above, this is displayed as follows:

power: 55mW (avg: 50mW, highest: 70mW)

In other words, average power is displayed as limit, similar to lowest/highest values, if instantaneous power is supported as well. If not, it is still displayed as before.

power: 50mW (highest: 70mW)

Do you have a better idea how to handle this case ?

Quoted 23 lines · Guenter Roeck

Hi Jean,

Quoted 2 lines · Jean Delvare

Hi Guenter,

[ ... ]

Quoted 10 lines

Review

Quoted 3 lines

static const struct sensor_subfeature_list power_inst_sensors[] = {

(quoted diff) +1 -0

+	{ SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },

I'm confused. This one was missing on purpose. For power sensors we consider instantaneous and averaging sensors as different types. The code in function print_chip_power is pretty clear about this. So, just as all _INPUT subfeatures aren't listed in limit arrays, SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.

I looked this up again.

The idea was to support display of both instantaneous power and average power if both is provided by the chip/driver.

With the above, this is displayed as follows:

Quoted 8 lines

power: 55mW (avg: 50mW, highest: 70mW)

In other words, average power is displayed as limit, similar to lowest/highest values, if instantaneous power is supported as well. If not, it is still displayed as before.

power: 50mW (highest: 70mW)

Do you have a better idea how to handle this case ?

Oh, this is pretty nice. Now this all makes sense, thanks for the clarification.

However it looks like a specific hack for a specific problem, rather than a generic solution. We have two sets of highest/lowest power attributes, one for instant measurements and one for averaged measurements. While your change makes it possible to display both the instant and averaged value, it can only display one set of highest/lowest limit. I supposed that a monitoring device could expose both, even if yours does not.

So with this change you can't claim that you display all power attributes. Rather, you special-cased SENSORS_SUBFEATURE_POWER_AVERAGE

to be either an input value or a limit value. This is certainly acceptable as long as it is properly documented. But this makes me wonder if it wouldn't be better to assume that all attributes may be present, and input may be missing in which case we use average as the measurement.

Well that was a long speech for a short conclusion: as far as I can see, all we have to do is duplicate power_avg_sensors[] at the end of power_inst_sensors[] and adjust the labels to avoid ambiguous duplicates. As far as I can see you don't even need to touch the code itself, except for stripping out the comment which claims that power sensor flavors are assumed to be mutually exclusive.

Quoted 6 lines · Jean Delvare

Well that was a long speech for a short conclusion: as far as I can see, all we have to do is duplicate power_avg_sensors[] at the end of power_inst_sensors[] and adjust the labels to avoid ambiguous duplicates. As far as I can see you don't even need to touch the code itself, except for stripping out the comment which claims that power sensor flavors are assumed to be mutually exclusive.

Makes sense. Something like the following ?

static const struct sensor_subfeature_list power_avg_sensors[] = {
{ SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, 0,
"avg_lowest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, 0,
"avg_highest" },

        { SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, 0,
                "interval" },
        { -1, NULL, 0, NULL }
};

static const struct sensor_subfeature_list power_inst_sensors[] = {
{ SENSORS_SUBFEATURE_POWER_AVERAGE, power_avg_sensors, 0,
"avg" },
{ SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" }, { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" }, { -1, NULL, 0, NULL }
};

Any good idea how to name the labels ?