sensors: Add support for additional

This patch adds support for additional sensor attributes to the sensors command. It is essentially a re-submit of a patch submitted earlier, plus support for additional sensor attributes which have been added to the sysfs ABI since the original patch was submitted.

This patch rewrites significant paths of chips.c to make it easier to add additional attributes. A thorough review would therefore be helpful.

--

Resubmitting this one as well. Question is how to proceed with it if no one has time for a thorough review. Maybe commit and clean up with subsequent patches if needed ?

Index: prog/sensors/chips.c
===================================================================

sensors/chips.c +2 -9

 {
-	if (fahrenheit) {
-		limit1 = deg_ctof(limit1);
-		limit2 = deg_ctof(limit2);
-        }
+	int i, j;
+	int first = 1;
 
-	if (name2) {
-		printf("(%-4s = %+5.1f%s, %-4s = %+5.1f%s)  ",
-		       name1, limit1, degstr,
-		       name2, limit2, degstr);
-	} else if (name1) {

sensors/chips.c +47 -90

+static const struct sensor_limits temp_alarms[] = {
+	{ SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
+	{ SENSORS_SUBFEATURE_TEMP_MIN_ALARM, NULL, NULL, 1, "MIN" },
+	{ SENSORS_SUBFEATURE_TEMP_MAX_ALARM, NULL, NULL, 1, "MAX" },
+	{ SENSORS_SUBFEATURE_TEMP_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+	{ SENSORS_SUBFEATURE_TEMP_EMERGENCY_ALARM, NULL, NULL, 1, "EMERGENCY" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_max_sensors[] = {
+	{ SENSORS_SUBFEATURE_TEMP_MAX_HYST, NULL, NULL, 0, "hyst" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_crit_sensors[] = {
+	{ SENSORS_SUBFEATURE_TEMP_CRIT_HYST, NULL, NULL, 0, "crit hyst" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_emergency_sensors[] = {
+	{ SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST, NULL, NULL, 0,
+	    "emergency hyst" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_sensors[] = {
+	{ SENSORS_SUBFEATURE_TEMP_ALARM, NULL, temp_alarms, 1, NULL },
+	{ SENSORS_SUBFEATURE_TEMP_MIN, NULL, NULL, 0, "low" },
+	{ SENSORS_SUBFEATURE_TEMP_MAX, temp_max_sensors, NULL, 0, "high" },
+	{ SENSORS_SUBFEATURE_TEMP_LCRIT, NULL, NULL, 0, "crit low" },
+	{ SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, NULL, 0, "crit" },
+	{ SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, NULL, 0,
+	    "emergency" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_temp(const sensors_chip_name *name,
 			    const sensors_feature *feature,
 			    int label_size)
 {
-	const sensors_subfeature *sf, *sfmin, *sfmax, *sfcrit, *sfhyst;
-	double val, limit1, limit2;
-	const char *s1, *s2;
-	int alarm, crit_displayed = 0;
+	struct sensor_limit_data sensors[8];
+	struct sensor_limit_data alarms[5];
+	int sensor_count, alarm_count;
+	const sensors_subfeature *sf;
+	double val;
 	char *label;
+	int i;
+	char fmt[32];
 
 	if (!(label = sensors_get_label(name, feature))) {
 		fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -168,80 +254,6 @@
 	free(label);
 
 	sf = sensors_get_subfeature(name, feature,
-				    SENSORS_SUBFEATURE_TEMP_ALARM);
-	alarm = sf && get_value(name, sf);
-
-	sfmin = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_TEMP_MIN);
-	sfmax = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_TEMP_MAX);
-	sfcrit = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_CRIT);
-	if (sfmax) {
-		sf = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_MAX_ALARM);
-		if (sf && get_value(name, sf))
-			alarm |= 1;
-
-     		if (sfmin) {
-			limit1 = get_value(name, sfmin);
-			s1 = "low";
-			limit2 = get_value(name, sfmax);
-			s2 = "high";
-
-			sf = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_MIN_ALARM);
-			if (sf && get_value(name, sf))
-				alarm |= 1;
-		} else {
-			limit1 = get_value(name, sfmax);
-			s1 = "high";
-
-			sfhyst = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_MAX_HYST);
-			if (sfhyst) {
-				limit2 = get_value(name, sfhyst);
-				s2 = "hyst";
-			} else if (sfcrit) {
-				limit2 = get_value(name, sfcrit);
-				s2 = "crit";
-
-				sf = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
-				if (sf && get_value(name, sf))
-					alarm |= 1;
-				crit_displayed = 1;
-			} else {
-				limit2 = 0;
-				s2 = NULL;
-			}
-		}
-	} else if (sfcrit) {
-		limit1 = get_value(name, sfcrit);
-		s1 = "crit";
-
-		sfhyst = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_CRIT_HYST);
-		if (sfhyst) {
-			limit2 = get_value(name, sfhyst);
-			s2 = "hyst";
-		} else {
-			limit2 = 0;
-			s2 = NULL;
-		}
-
-		sf = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
-		if (sf && get_value(name, sf))
-			alarm |= 1;
-		crit_displayed = 1;
-	} else {
-		limit1 = limit2 = 0;
-		s1 = s2 = NULL;
-	}
-
-
-	sf = sensors_get_subfeature(name, feature,
 				    SENSORS_SUBFEATURE_TEMP_FAULT);
 	if (sf && get_value(name, sf)) {
 		printf("   FAULT  ");

@@ -256,30 +268,21 @@
 		} else
 			printf("     N/A  ");
 	}
-	print_temp_limits(limit1, limit2, s1, s2, alarm);
 
-	if (!crit_displayed && sfcrit) {
-		limit1 = get_value(name, sfcrit);
-		s1 = "crit";
+	sensor_count = alarm_count = 0;
+	get_sensor_limit_data(name, feature, temp_sensors,
+			      sensors, &sensor_count,
+			      alarms, &alarm_count);
 
-		sfhyst = sensors_get_subfeature(name, feature,
-					SENSORS_SUBFEATURE_TEMP_CRIT_HYST);
-		if (sfhyst) {
-			limit2 = get_value(name, sfhyst);
-			s2 = "hyst";
-		} else {
-			limit2 = 0;
-			s2 = NULL;

sensors/chips.c +44 -46

+	print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
+		     fmt);
 
-		printf("\n%*s", label_size + 10, "");
-		print_temp_limits(limit1, limit2, s1, s2, alarm);
-	}
-
 	/* print out temperature sensor info */
 	sf = sensors_get_subfeature(name, feature,
 				    SENSORS_SUBFEATURE_TEMP_TYPE);

@@ -302,13 +305,33 @@
 	printf("\n");
 }
 
+static const struct sensor_limits voltage_alarms[] = {
+	{ SENSORS_SUBFEATURE_IN_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
+	{ SENSORS_SUBFEATURE_IN_MIN_ALARM, NULL, NULL, 1, "MIN" },
+	{ SENSORS_SUBFEATURE_IN_MAX_ALARM, NULL, NULL, 1, "MAX" },
+	{ SENSORS_SUBFEATURE_IN_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits voltage_sensors[] = {
+	{ SENSORS_SUBFEATURE_IN_ALARM, NULL, voltage_alarms, 1, NULL },
+	{ SENSORS_SUBFEATURE_IN_LCRIT, NULL, NULL, 0, "crit min" },
+	{ SENSORS_SUBFEATURE_IN_MIN, NULL, NULL, 0, "min" },
+	{ SENSORS_SUBFEATURE_IN_MAX, NULL, NULL, 0, "max" },
+	{ SENSORS_SUBFEATURE_IN_CRIT, NULL, NULL, 0, "crit max" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_in(const sensors_chip_name *name,
 			  const sensors_feature *feature,
 			  int label_size)
 {
-	const sensors_subfeature *sf, *sfmin, *sfmax;
-	double val, alarm_max, alarm_min;
+	const sensors_subfeature *sf;
 	char *label;
+	struct sensor_limit_data sensors[4];
+	struct sensor_limit_data alarms[4];
+	int sensor_count, alarm_count;
+	double val;
 
 	if (!(label = sensors_get_label(name, feature))) {
 		fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -321,50 +344,18 @@
 	sf = sensors_get_subfeature(name, feature,
 				    SENSORS_SUBFEATURE_IN_INPUT);
 	if (sf && get_input_value(name, sf, &val) == 0)
-		printf("%+6.2f V", val);
+		printf("%+6.2f V  ", val);
 	else
-		printf("     N/A");
+		printf("     N/A  ");
 
-	sfmin = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_IN_MIN);
-	sfmax = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_IN_MAX);
-	if (sfmin && sfmax)
-		printf("  (min = %+6.2f V, max = %+6.2f V)",
-		       get_value(name, sfmin),
-		       get_value(name, sfmax));
-	else if (sfmin)
-		printf("  (min = %+6.2f V)",
-		       get_value(name, sfmin));
-	else if (sfmax)
-		printf("  (max = %+6.2f V)",
-		       get_value(name, sfmax));
+	sensor_count = alarm_count = 0;
+	get_sensor_limit_data(name, feature, voltage_sensors,
+			      sensors, &sensor_count,
+			      alarms, &alarm_count);
 
-	sf = sensors_get_subfeature(name, feature,
-				    SENSORS_SUBFEATURE_IN_ALARM);
-	sfmin = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_IN_MIN_ALARM);
-	sfmax = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_IN_MAX_ALARM);
-	if (sfmin || sfmax) {
-		alarm_max = sfmax ? get_value(name, sfmax) : 0;
-		alarm_min = sfmin ? get_value(name, sfmin) : 0;
+	print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
+		     "%s = %+6.2f V");
 
-		if (alarm_min || alarm_max) {
-			printf(" ALARM (");
-
-			if (alarm_min)
-				printf("MIN");
-			if (alarm_max)
-				printf("%sMAX", (alarm_min) ? ", " : "");
-
-			printf(")");
-		}
-	} else if (sf) {
-		printf("   %s",
-		       get_value(name, sf) ? "ALARM" : "");
-	}
-
 	printf("\n");
 }
 

@@ -450,15 +441,53 @@
 	*prefixstr = scale->unit;
 }
 
+static const struct sensor_limits power_alarms[] = {
+	{ SENSORS_SUBFEATURE_POWER_CAP_ALARM, NULL, NULL, 1, "CAP" },
+	{ SENSORS_SUBFEATURE_POWER_MAX_ALARM, NULL, NULL, 1, "MAX" },
+	{ SENSORS_SUBFEATURE_POWER_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_highest[] = {
+	{ SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, NULL, 0, "max" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+

sensors/chips.c +28 -19

+	{ SENSORS_SUBFEATURE_POWER_CAP, NULL, NULL, 0, "cap" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_avg_sensors[] = {
+	{ SENSORS_SUBFEATURE_POWER_ALARM, NULL, NULL, 1, NULL },
+	{ SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, NULL, 0, "min" },
+	{ SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, NULL, 0, "max" },
+	{ SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, NULL, 0,
+	    "interval" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_power(const sensors_chip_name *name,
 			     const sensors_feature *feature,
 			     int label_size)
 {
 	double val;
-	int need_space = 0;
-	const sensors_subfeature *sf, *sfmin, *sfmax, *sfint;
+	const sensors_subfeature *sf;
+	struct sensor_limit_data sensors[4];
+	struct sensor_limit_data alarms[3];
+	int sensor_count, alarm_count;
 	char *label;
 	const char *unit;
+	int i;
 
 	if (!(label = sensors_get_label(name, feature))) {
 		fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -468,60 +497,36 @@
 	print_label(label, label_size);
 	free(label);
 
+	sensor_count = alarm_count = 0;
+
 	/* Power sensors come in 2 flavors: instantaneous and averaged.
 	   To keep things simple, we assume that each sensor only implements
 	   one flavor. */
 	sf = sensors_get_subfeature(name, feature,
 				    SENSORS_SUBFEATURE_POWER_INPUT);
 	if (sf) {
-		sfmin = sensors_get_subfeature(name, feature,
-					       SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST);
-		sfmax = sensors_get_subfeature(name, feature,
-					       SENSORS_SUBFEATURE_POWER_INPUT_LOWEST);
-		sfint = NULL;
+		get_sensor_limit_data(name, feature, power_inst_sensors,
+				      sensors, &sensor_count, alarms,
+				      &alarm_count);
 	} else {
-		sf = sensors_get_subfeature(name, feature,
-					    SENSORS_SUBFEATURE_POWER_AVERAGE);
-		sfmin = sensors_get_subfeature(name, feature,
-					       SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST);
-		sfmax = sensors_get_subfeature(name, feature,
-					       SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST);
-		sfint = sensors_get_subfeature(name, feature,
-					       SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL);
+		get_sensor_limit_data(name, feature, power_avg_sensors,
+				      sensors, &sensor_count, alarms,
+				      &alarm_count);
 	}
 
 	if (sf && get_input_value(name, sf, &val) == 0) {
 		scale_value(&val, &unit);
-		printf("%6.2f %sW", val, unit);
+		printf("%6.2f %sW  ", val, unit);
 	} else
-		printf("     N/A");
+		printf("     N/A  ");
 
-	if (sfmin || sfmax || sfint) {
-		printf("  (");

sensors/chips.c +32 -58

+		print_limits(sensors, sensor_count, alarms, alarm_count,
+			     label_size, "%s = %6.2f %sW");
 
-		if (sfmax) {
-			val = get_value(name, sfmax);
-			scale_value(&val, &unit);
-			printf("%smax = %6.2f %sW", (need_space ? ", " : ""),
-			       val, unit);
-			need_space = 1;
-		}
-
-		if (sfint) {
-			printf("%sinterval = %6.2f s", (need_space ? ", " : ""),
-			       get_value(name, sfint));
-			need_space = 1;
-		}
-		printf(")");
-	}
-
 	printf("\n");
 }
 

@@ -595,13 +600,33 @@
 	free(label);
 }
 
+static const struct sensor_limits current_alarms[] = {
+	{ SENSORS_SUBFEATURE_CURR_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
+	{ SENSORS_SUBFEATURE_CURR_MIN_ALARM, NULL, NULL, 1, "MIN" },
+	{ SENSORS_SUBFEATURE_CURR_MAX_ALARM, NULL, NULL, 1, "MAX" },
+	{ SENSORS_SUBFEATURE_CURR_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits current_sensors[] = {
+	{ SENSORS_SUBFEATURE_CURR_ALARM, NULL, current_alarms, 1, NULL },
+	{ SENSORS_SUBFEATURE_CURR_LCRIT, NULL, NULL, 0, "crit min" },
+	{ SENSORS_SUBFEATURE_CURR_MIN, NULL, NULL, 0, "min" },
+	{ SENSORS_SUBFEATURE_CURR_MAX, NULL, NULL, 0, "max" },
+	{ SENSORS_SUBFEATURE_CURR_CRIT, NULL, NULL, 0, "crit max" },
+	{ -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_curr(const sensors_chip_name *name,
 			    const sensors_feature *feature,
 			    int label_size)
 {
-	const sensors_subfeature *sf, *sfmin, *sfmax;
-	double alarm_max, alarm_min, val;
+	const sensors_subfeature *sf;
+	double val;
 	char *label;
+	struct sensor_limit_data sensors[4];
+	struct sensor_limit_data alarms[4];
+	int sensor_count, alarm_count;
 
 	if (!(label = sensors_get_label(name, feature))) {
 		fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -614,50 +639,18 @@
 	sf = sensors_get_subfeature(name, feature,
 				    SENSORS_SUBFEATURE_CURR_INPUT);
 	if (sf && get_input_value(name, sf, &val) == 0)
-		printf("%+6.2f A", val);
+		printf("%+6.2f A  ", val);
 	else
-		printf("     N/A");
+		printf("     N/A  ");
 
-	sfmin = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_CURR_MIN);
-	sfmax = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_CURR_MAX);
-	if (sfmin && sfmax)
-		printf("  (min = %+6.2f A, max = %+6.2f A)",
-		       get_value(name, sfmin),
-		       get_value(name, sfmax));
-	else if (sfmin)
-		printf("  (min = %+6.2f A)",
-		       get_value(name, sfmin));
-	else if (sfmax)
-		printf("  (max = %+6.2f A)",
-		       get_value(name, sfmax));
+	sensor_count = alarm_count = 0;
+	get_sensor_limit_data(name, feature, current_sensors,
+			      sensors, &sensor_count,
+			      alarms, &alarm_count);
 
-	sf = sensors_get_subfeature(name, feature,
-				    SENSORS_SUBFEATURE_CURR_ALARM);
-	sfmin = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_CURR_MIN_ALARM);
-	sfmax = sensors_get_subfeature(name, feature,
-				       SENSORS_SUBFEATURE_CURR_MAX_ALARM);
-	if (sfmin || sfmax) {
-		alarm_max = sfmax ? get_value(name, sfmax) : 0;
-		alarm_min = sfmin ? get_value(name, sfmin) : 0;
+	print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
+		     "%s = %+6.2f A");
 
-		if (alarm_min || alarm_max) {
-			printf(" ALARM (");
-
-			if (alarm_min)
-				printf("MIN");
-			if (alarm_max)
-				printf("%sMAX", (alarm_min) ? ", " : "");
-
-			printf(")");
-		}
-	} else if (sf) {
-		printf("   %s",
-		       get_value(name, sf) ? "ALARM" : "");
-	}
-
 	printf("\n");
 }
 

Index: prog/sensors/chips.h
===================================================================

Hi Guenter,

Sorry for the late answer.

Quoted 5 lines

This patch adds support for additional sensor attributes to the sensors command. It is essentially a re-submit of a patch submitted earlier, plus support for additional sensor attributes which have been added to the sysfs ABI since the original patch was submitted.

Quoted 8 lines

This patch rewrites significant paths of chips.c to make it easier to add additional attributes. A thorough review would therefore be helpful.

--

Resubmitting this one as well. Question is how to proceed with it if no one has time for a thorough review. Maybe commit and clean up with subsequent patches if needed ?

What would certainly have helped would be to split the patch into at least two pieces, one adding the generic mechanism and converting the already supported limits to use is, and another one adding support for the new limits. It would have made reviewing slightly easier, and would also have demonstrated your point that the new mechanism makes adding new attributes and limits easier.

Regarding the change itself, it is generally welcome, as the specific code had become difficult to maintain, and would have become totally horrible after adding support for the new limits. I wasn't especially proud of it in the first place, so I'm happy to see it go.

I've tested the new code with valgrind and no problems were reported in my case (chips covered: radeon, coretemp, w83795adg, emc6d102 and adm1032.)

I've also compared the output of "sensors" before and after the patch. Except for a decreasing amount trailing white space, which is definitely not an issue, I noticed two differences.

The first difference is "hyst" being changed to "crit hyst" for critical temperature limit hysteresis. The original code enforced every temperature limit label to 4 characters to keep all lines nicely aligned, and your change breaks this effort. As the hysteresis value was always printed right after the limit it related to, on the same line, the "crit" in front of "hyst" was redundant. With the new code, there might be a new line in between, which isn't so nice. This should be addressed in an incremental patch IMHO (I can take care.)

The second difference is the amount of space before the ALARM flags for voltage and temperature inputs. There used to be only two spaces, saving screen space and being in line with the temperature sensor type information. Now there are 4 spaces for voltages and temperatures, but still only 2 for fans. I presume this change wasn't made on purpose, so I'd like to see it reverted, with ALARM flags being preceded with 2 spaces for all sensor types. Should be very easy to address, see below.

Now, to the code itself:

Quoted 8 lines

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

(quoted diff) +0 -0

@@ -126,38 +126,124 @@
      return max_size + 2;
 }
 

Review line 133

Quoted 7 lines

(quoted diff) +5 -2

-static void print_temp_limits(double limit1, double limit2,
-                           const char *name1, const char *name2, int alarm)
+static void print_limits(struct sensor_limit_data *sensors,
+                      int sensor_count,
+                      struct sensor_limit_data *alarms,
+                      int alarm_count, int label_size,
+                      const char *fmt)

sensors and sensor_count are really misnomers here. These are limits and limit_count.

Quoted 13 lines

(quoted diff) +2 -9

 {
-     if (fahrenheit) {
-             limit1 = deg_ctof(limit1);
-             limit2 = deg_ctof(limit2);
-        }
+     int i, j;
+     int first = 1;
 
-     if (name2) {
-             printf("(%-4s = %+5.1f%s, %-4s = %+5.1f%s)  ",
-                    name1, limit1, degstr,
-                    name2, limit2, degstr);
-     } else if (name1) {

Review line 143

Quoted 10 lines

(quoted diff) +6 -4

-             printf("(%-4s = %+5.1f%s)                  ",
-                    name1, limit1, degstr);
-     } else {
-             printf("                                  ");
+     for (i = 0; i < sensor_count; i++) {
+             if (!(i & 1)) {
+                     if (i)
+                             printf("\n%*s", label_size + 10, "");
+                     printf("(");
+             } else if (i)

I don't get this conditional... it's always true, isn't it?

Review line 147

Quoted 4 lines

(quoted diff) +4 -0

+                     printf(", ");
+             printf(fmt, sensors[i].name, sensors[i].value, sensors[i].unit);
+             if ((i & 1) || i = sensor_count - 1) {
+                     printf(")  ");

The original code would insert many spaces here if only one limit was printed on this line, to align all potential ALARM flags regardless of each sensor having an odd or even limit count.

Review line 148

Quoted 1 line

(quoted diff) +1 -0

+                     if (first && alarm_count) {

With your current implementation, testing "first" is equivalent to testing "i < 2", so you don't really need "first".

Review line 149

Quoted 1 line

(quoted diff) +1 -0

+                             printf("  ALARM");

You want "ALARM", not " ALARM", otherwise this is too many spaces. Or alternatively change the ") " above to just ")".

Review line 150

Quoted 1 line

(quoted diff) +1 -0

+                             if (alarm_count > 1 || alarms[0].name) {

The second test is sufficient. As a matter of fact, you print alarms[0].name unconditionally in the loop below.

Review line 151

Quoted 1 line

(quoted diff) +1 -0

+                                     printf("(");

You want " (" here.

Review line 163

Quoted 12 lines

(quoted diff) +11 -0

+                                     for (j = 0; j < alarm_count; j++) {
+                                             printf("%s", alarms[j].name);
+                                             if (j < alarm_count - 1)
+                                                     printf(", ");
+                                     }
+                                     printf(")");
+                             }
+                             first = 0;
+                     }
+             }
      }
+}

You'll have to decide whether this function ends with printing a ")", or ") ". Right now, it does the former if there are alarms printed, and the latter if not. The calling code can't deal with this, obviously. It seems to me that ending on ")" would make more sense, to give the caller maximum flexibility.

Also, this function only prints the alarms if at least one limit was found. While it indeed makes little sense to have alarm flags without corresponding limits, well... there is weird hardware out there, so I'm not sure we can ignore this case.

Review line 170

Quoted 10 lines

(quoted diff) +7 -2

-     if (alarm)
-             printf("ALARM  ");
+static void get_sensor_limit_data(const sensors_chip_name *name,
+                               const sensors_feature *feature,
+                               const struct sensor_limits *ts,
+                               struct sensor_limit_data *ts_data,
+                               int *ts_sensors,
+                               struct sensor_limit_data *ts_alarm_data,
+                               int *ts_alarms)

The names of the last 4 parameters are really confusing... even worse than for print_limits()! Please use the same naming convention for both functions. And in the calling functions, too. And I'm not even sure what "ts" stands for... "temperature sensor", as a legacy of a time where this function was only used to print temperature limits?

Quoted 9 lines

(quoted diff) +9 -0

+{
+     const sensors_subfeature *sf;
+
+     for (; ts->subfeature >= 0; ts++) {
+             sf = sensors_get_subfeature(name, feature, ts->subfeature);
+             if (sf) {
+                     if (ts->alarm && get_value(name, sf)) {
+                             ts_alarm_data[*ts_alarms].name = ts->name;
+                             (*ts_alarms)++;

Review line 180

Quoted 1 line

(quoted diff) +1 -0

+                     } else if (!ts->alarm) {

This test is partly redundant. If you split the get_value() test above, you can skip the !ts->alarm test. BTW the test on get_value() definitely deserves a comment. You only queue alarm subfeatures if the alarm is active, and you don't store their value, while limit subfeatures are always queued, with their value. The function should be documented as such.

Review line 191

Quoted 11 lines

(quoted diff) +11 -0

+                             ts_data[*ts_sensors].value
+                               = get_value(name, sf);
+                             ts_data[*ts_sensors].name = ts->name;
+                             (*ts_sensors)++;
+                     }
+                     if (ts->exists) {
+                             get_sensor_limit_data(name, feature, ts->exists,
+                                                   ts_data, ts_sensors,
+                                                   ts_alarm_data, ts_alarms);
+                     }
+             } else if (!sf && ts->nexists)

The test for !sf is redundant here, it will always succeed.

Review line 196

Quoted 5 lines

(quoted diff) +4 -0

+                     get_sensor_limit_data(name, feature, ts->nexists,
+                                           ts_data, ts_sensors,
+                                           ts_alarm_data, ts_alarms);
+     }
 }

There is something I don't like about this function: you assume that the provided ts, ts_sensors and ts_alarms are such that all the subfeatures from ts (including exists and nexists recursions) will fit in ts_sensors and ts_alarms are. If this assumption fails, you'll overrun the arrays and corrupt the stack, which is pretty bad. The recursive relations between the various ts arrays make it easy to get wrong, methinks.

Unfortunately I see no way to check the sizes for correctness at build time, which means we'll have to do it at run time. This could be done with the current prototype by having the caller pass the array sizes as the initial values of ts_sensors and ts_alarms. The code would only have to be adjusted a little. Or maybe you have a better idea?

Quoted 164 lines

(quoted diff) +50 -91

+static const struct sensor_limits temp_alarms[] = {
+     { SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
+     { SENSORS_SUBFEATURE_TEMP_MIN_ALARM, NULL, NULL, 1, "MIN" },
+     { SENSORS_SUBFEATURE_TEMP_MAX_ALARM, NULL, NULL, 1, "MAX" },
+     { SENSORS_SUBFEATURE_TEMP_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+     { SENSORS_SUBFEATURE_TEMP_EMERGENCY_ALARM, NULL, NULL, 1, "EMERGENCY" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_max_sensors[] = {
+     { SENSORS_SUBFEATURE_TEMP_MAX_HYST, NULL, NULL, 0, "hyst" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_crit_sensors[] = {
+     { SENSORS_SUBFEATURE_TEMP_CRIT_HYST, NULL, NULL, 0, "crit hyst" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_emergency_sensors[] = {
+     { SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST, NULL, NULL, 0,
+         "emergency hyst" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits temp_sensors[] = {
+     { SENSORS_SUBFEATURE_TEMP_ALARM, NULL, temp_alarms, 1, NULL },
+     { SENSORS_SUBFEATURE_TEMP_MIN, NULL, NULL, 0, "low" },
+     { SENSORS_SUBFEATURE_TEMP_MAX, temp_max_sensors, NULL, 0, "high" },
+     { SENSORS_SUBFEATURE_TEMP_LCRIT, NULL, NULL, 0, "crit low" },
+     { SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, NULL, 0, "crit" },
+     { SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, NULL, 0,
+         "emergency" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_temp(const sensors_chip_name *name,
                          const sensors_feature *feature,
                          int label_size)
 {
-     const sensors_subfeature *sf, *sfmin, *sfmax, *sfcrit, *sfhyst;
-     double val, limit1, limit2;
-     const char *s1, *s2;
-     int alarm, crit_displayed = 0;
+     struct sensor_limit_data sensors[8];
+     struct sensor_limit_data alarms[5];
+     int sensor_count, alarm_count;
+     const sensors_subfeature *sf;
+     double val;
      char *label;
+     int i;
+     char fmt[32];
 
      if (!(label = sensors_get_label(name, feature))) {
              fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -168,80 +254,6 @@
      free(label);
 
      sf = sensors_get_subfeature(name, feature,
-                                 SENSORS_SUBFEATURE_TEMP_ALARM);
-     alarm = sf && get_value(name, sf);
-
-     sfmin = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_TEMP_MIN);
-     sfmax = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_TEMP_MAX);
-     sfcrit = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_CRIT);
-     if (sfmax) {
-             sf = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_MAX_ALARM);
-             if (sf && get_value(name, sf))
-                     alarm |= 1;
-
-                     if (sfmin) {
-                     limit1 = get_value(name, sfmin);
-                     s1 = "low";
-                     limit2 = get_value(name, sfmax);
-                     s2 = "high";
-
-                     sf = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_MIN_ALARM);
-                     if (sf && get_value(name, sf))
-                             alarm |= 1;
-             } else {
-                     limit1 = get_value(name, sfmax);
-                     s1 = "high";
-
-                     sfhyst = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_MAX_HYST);
-                     if (sfhyst) {
-                             limit2 = get_value(name, sfhyst);
-                             s2 = "hyst";
-                     } else if (sfcrit) {
-                             limit2 = get_value(name, sfcrit);
-                             s2 = "crit";
-
-                             sf = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
-                             if (sf && get_value(name, sf))
-                                     alarm |= 1;
-                             crit_displayed = 1;
-                     } else {
-                             limit2 = 0;
-                             s2 = NULL;
-                     }
-             }
-     } else if (sfcrit) {
-             limit1 = get_value(name, sfcrit);
-             s1 = "crit";
-
-             sfhyst = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_CRIT_HYST);
-             if (sfhyst) {
-                     limit2 = get_value(name, sfhyst);
-                     s2 = "hyst";
-             } else {
-                     limit2 = 0;
-                     s2 = NULL;
-             }
-
-             sf = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
-             if (sf && get_value(name, sf))
-                     alarm |= 1;
-             crit_displayed = 1;
-     } else {
-             limit1 = limit2 = 0;
-             s1 = s2 = NULL;
-     }
-
-
-     sf = sensors_get_subfeature(name, feature,
                                  SENSORS_SUBFEATURE_TEMP_FAULT);
      if (sf && get_value(name, sf)) {
              printf("   FAULT  ");

@@ -256,30 +268,21 @@
              } else
                      printf("     N/A  ");
      }
-     print_temp_limits(limit1, limit2, s1, s2, alarm);
 
-     if (!crit_displayed && sfcrit) {
-             limit1 = get_value(name, sfcrit);
-             s1 = "crit";
+     sensor_count = alarm_count = 0;
+     get_sensor_limit_data(name, feature, temp_sensors,
+                           sensors, &sensor_count,
+                           alarms, &alarm_count);
 
-             sfhyst = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_CRIT_HYST);
-             if (sfhyst) {
-                     limit2 = get_value(name, sfhyst);
-                     s2 = "hyst";
-             } else {
-                     limit2 = 0;
-                     s2 = NULL;
-             }
+     if (fahrenheit)
+             for (i = 0; i < sensor_count; i++)
+                     sensors[i].value = deg_ctof(sensors[i].value);
 

Review line 282

Quoted 5 lines

(quoted diff) +2 -3

-             sf = sensors_get_subfeature(name, feature,
-                                     SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
-             alarm = sf && get_value(name, sf);
+     strcpy(fmt, "%-4s = %+5.1f");
+     strcat(fmt, degstr);

I strongly discourage the use of strcpy and strcat, especially on stack buffers, for both safety and performance reasons. I would have suggested the use of snprintf instead, but...

You implemented a very nice mechanism to attach units to limit values, which you use for power limits, I fail to see why you don't just use it here as well? It would make the code more elegant IMHO (see attached patch.)

Quoted 129 lines

(quoted diff) +52 -46

+     print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
+                  fmt);
 
-             printf("\n%*s", label_size + 10, "");
-             print_temp_limits(limit1, limit2, s1, s2, alarm);
-     }
-
      /* print out temperature sensor info */
      sf = sensors_get_subfeature(name, feature,
                                  SENSORS_SUBFEATURE_TEMP_TYPE);

@@ -302,13 +305,33 @@
      printf("\n");
 }
 
+static const struct sensor_limits voltage_alarms[] = {
+     { SENSORS_SUBFEATURE_IN_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
+     { SENSORS_SUBFEATURE_IN_MIN_ALARM, NULL, NULL, 1, "MIN" },
+     { SENSORS_SUBFEATURE_IN_MAX_ALARM, NULL, NULL, 1, "MAX" },
+     { SENSORS_SUBFEATURE_IN_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits voltage_sensors[] = {
+     { SENSORS_SUBFEATURE_IN_ALARM, NULL, voltage_alarms, 1, NULL },
+     { SENSORS_SUBFEATURE_IN_LCRIT, NULL, NULL, 0, "crit min" },
+     { SENSORS_SUBFEATURE_IN_MIN, NULL, NULL, 0, "min" },
+     { SENSORS_SUBFEATURE_IN_MAX, NULL, NULL, 0, "max" },
+     { SENSORS_SUBFEATURE_IN_CRIT, NULL, NULL, 0, "crit max" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_in(const sensors_chip_name *name,
                        const sensors_feature *feature,
                        int label_size)
 {
-     const sensors_subfeature *sf, *sfmin, *sfmax;
-     double val, alarm_max, alarm_min;
+     const sensors_subfeature *sf;
      char *label;
+     struct sensor_limit_data sensors[4];
+     struct sensor_limit_data alarms[4];
+     int sensor_count, alarm_count;
+     double val;
 
      if (!(label = sensors_get_label(name, feature))) {
              fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -321,50 +344,18 @@
      sf = sensors_get_subfeature(name, feature,
                                  SENSORS_SUBFEATURE_IN_INPUT);
      if (sf && get_input_value(name, sf, &val) = 0)
-             printf("%+6.2f V", val);
+             printf("%+6.2f V  ", val);
      else
-             printf("     N/A");
+             printf("     N/A  ");
 
-     sfmin = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_IN_MIN);
-     sfmax = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_IN_MAX);
-     if (sfmin && sfmax)
-             printf("  (min = %+6.2f V, max = %+6.2f V)",
-                    get_value(name, sfmin),
-                    get_value(name, sfmax));
-     else if (sfmin)
-             printf("  (min = %+6.2f V)",
-                    get_value(name, sfmin));
-     else if (sfmax)
-             printf("  (max = %+6.2f V)",
-                    get_value(name, sfmax));
+     sensor_count = alarm_count = 0;
+     get_sensor_limit_data(name, feature, voltage_sensors,
+                           sensors, &sensor_count,
+                           alarms, &alarm_count);
 
-     sf = sensors_get_subfeature(name, feature,
-                                 SENSORS_SUBFEATURE_IN_ALARM);
-     sfmin = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_IN_MIN_ALARM);
-     sfmax = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_IN_MAX_ALARM);
-     if (sfmin || sfmax) {
-             alarm_max = sfmax ? get_value(name, sfmax) : 0;
-             alarm_min = sfmin ? get_value(name, sfmin) : 0;
+     print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
+                  "%s = %+6.2f V");
 
-             if (alarm_min || alarm_max) {
-                     printf(" ALARM (");
-
-                     if (alarm_min)
-                             printf("MIN");
-                     if (alarm_max)
-                             printf("%sMAX", (alarm_min) ? ", " : "");
-
-                     printf(")");
-             }
-     } else if (sf) {
-             printf("   %s",
-                    get_value(name, sf) ? "ALARM" : "");
-     }
-
      printf("\n");
 }
 

@@ -450,15 +441,53 @@
      *prefixstr = scale->unit;
 }
 
+static const struct sensor_limits power_alarms[] = {
+     { SENSORS_SUBFEATURE_POWER_CAP_ALARM, NULL, NULL, 1, "CAP" },
+     { SENSORS_SUBFEATURE_POWER_MAX_ALARM, NULL, NULL, 1, "MAX" },
+     { SENSORS_SUBFEATURE_POWER_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_highest[] = {
+     { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, NULL, 0, "max" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_max[] = {
+     { SENSORS_SUBFEATURE_POWER_MAX, NULL, NULL, 0, "max" },
+     { SENSORS_SUBFEATURE_POWER_CRIT, NULL, NULL, 0, "crit" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_sensors[] = {
+     { SENSORS_SUBFEATURE_POWER_ALARM, NULL, power_alarms, 1, NULL },

Review line 465

Quoted 2 lines

(quoted diff) +2 -0

+     { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, power_inst_highest,
+         power_inst_max, 0, "min" },

This deserves a comment at least... if it is correct at all. Why would INPUT_HIGHEST require INPUT_LOWEST? I can imagine a chip providing one without the other (this would even make a lot of sense if you ask me). I also don't get why MAX and CRIT would be mutually exclusive with INPUT_LOWEST and INPUT_HIGHEST. Please explain or fix.

Quoted 77 lines

(quoted diff) +30 -19

+     { SENSORS_SUBFEATURE_POWER_CAP, NULL, NULL, 0, "cap" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_avg_sensors[] = {
+     { SENSORS_SUBFEATURE_POWER_ALARM, NULL, NULL, 1, NULL },
+     { SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, NULL, 0, "min" },
+     { SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, NULL, 0, "max" },
+     { SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, NULL, 0,
+         "interval" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_power(const sensors_chip_name *name,
                           const sensors_feature *feature,
                           int label_size)
 {
      double val;
-     int need_space = 0;
-     const sensors_subfeature *sf, *sfmin, *sfmax, *sfint;
+     const sensors_subfeature *sf;
+     struct sensor_limit_data sensors[4];
+     struct sensor_limit_data alarms[3];
+     int sensor_count, alarm_count;
      char *label;
      const char *unit;
+     int i;
 
      if (!(label = sensors_get_label(name, feature))) {
              fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -468,60 +497,36 @@
      print_label(label, label_size);
      free(label);
 
+     sensor_count = alarm_count = 0;
+
      /* Power sensors come in 2 flavors: instantaneous and averaged.
         To keep things simple, we assume that each sensor only implements
         one flavor. */
      sf = sensors_get_subfeature(name, feature,
                                  SENSORS_SUBFEATURE_POWER_INPUT);
      if (sf) {
-             sfmin = sensors_get_subfeature(name, feature,
-                                            SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST);
-             sfmax = sensors_get_subfeature(name, feature,
-                                            SENSORS_SUBFEATURE_POWER_INPUT_LOWEST);
-             sfint = NULL;
+             get_sensor_limit_data(name, feature, power_inst_sensors,
+                                   sensors, &sensor_count, alarms,
+                                   &alarm_count);
      } else {
-             sf = sensors_get_subfeature(name, feature,
-                                         SENSORS_SUBFEATURE_POWER_AVERAGE);
-             sfmin = sensors_get_subfeature(name, feature,
-                                            SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST);
-             sfmax = sensors_get_subfeature(name, feature,
-                                            SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST);
-             sfint = sensors_get_subfeature(name, feature,
-                                            SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL);
+             get_sensor_limit_data(name, feature, power_avg_sensors,
+                                   sensors, &sensor_count, alarms,
+                                   &alarm_count);
      }
 
      if (sf && get_input_value(name, sf, &val) = 0) {
              scale_value(&val, &unit);
-             printf("%6.2f %sW", val, unit);
+             printf("%6.2f %sW  ", val, unit);
      } else
-             printf("     N/A");
+             printf("     N/A  ");
 
-     if (sfmin || sfmax || sfint) {
-             printf("  (");
+     for (i = 0; i < sensor_count; i++)
+             scale_value(&sensors[i].value, &sensors[i].unit);
 

Review line 526

Quoted 7 lines

(quoted diff) +1 -6

-             if (sfmin) {
-                     val = get_value(name, sfmin);
-                     scale_value(&val, &unit);
-                     printf("min = %6.2f %sW", val, unit);
-                     need_space = 1;
-             }
+     if (sensor_count)

This test isn't needed, print_limits() will deal with the !sensor_count case just fine (and as a matter of fact you don't have this test in any other function.)

Quoted 121 lines

(quoted diff) +32 -58

+             print_limits(sensors, sensor_count, alarms, alarm_count,
+                          label_size, "%s = %6.2f %sW");
 
-             if (sfmax) {
-                     val = get_value(name, sfmax);
-                     scale_value(&val, &unit);
-                     printf("%smax = %6.2f %sW", (need_space ? ", " : ""),
-                            val, unit);
-                     need_space = 1;
-             }
-
-             if (sfint) {
-                     printf("%sinterval = %6.2f s", (need_space ? ", " : ""),
-                            get_value(name, sfint));
-                     need_space = 1;
-             }
-             printf(")");
-     }
-
      printf("\n");
 }
 

@@ -595,13 +600,33 @@
      free(label);
 }
 
+static const struct sensor_limits current_alarms[] = {
+     { SENSORS_SUBFEATURE_CURR_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
+     { SENSORS_SUBFEATURE_CURR_MIN_ALARM, NULL, NULL, 1, "MIN" },
+     { SENSORS_SUBFEATURE_CURR_MAX_ALARM, NULL, NULL, 1, "MAX" },
+     { SENSORS_SUBFEATURE_CURR_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits current_sensors[] = {
+     { SENSORS_SUBFEATURE_CURR_ALARM, NULL, current_alarms, 1, NULL },
+     { SENSORS_SUBFEATURE_CURR_LCRIT, NULL, NULL, 0, "crit min" },
+     { SENSORS_SUBFEATURE_CURR_MIN, NULL, NULL, 0, "min" },
+     { SENSORS_SUBFEATURE_CURR_MAX, NULL, NULL, 0, "max" },
+     { SENSORS_SUBFEATURE_CURR_CRIT, NULL, NULL, 0, "crit max" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
 static void print_chip_curr(const sensors_chip_name *name,
                          const sensors_feature *feature,
                          int label_size)
 {
-     const sensors_subfeature *sf, *sfmin, *sfmax;
-     double alarm_max, alarm_min, val;
+     const sensors_subfeature *sf;
+     double val;
      char *label;
+     struct sensor_limit_data sensors[4];
+     struct sensor_limit_data alarms[4];
+     int sensor_count, alarm_count;
 
      if (!(label = sensors_get_label(name, feature))) {
              fprintf(stderr, "ERROR: Can't get label of feature %s!\n",

@@ -614,50 +639,18 @@
      sf = sensors_get_subfeature(name, feature,
                                  SENSORS_SUBFEATURE_CURR_INPUT);
      if (sf && get_input_value(name, sf, &val) = 0)
-             printf("%+6.2f A", val);
+             printf("%+6.2f A  ", val);
      else
-             printf("     N/A");
+             printf("     N/A  ");
 
-     sfmin = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_CURR_MIN);
-     sfmax = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_CURR_MAX);
-     if (sfmin && sfmax)
-             printf("  (min = %+6.2f A, max = %+6.2f A)",
-                    get_value(name, sfmin),
-                    get_value(name, sfmax));
-     else if (sfmin)
-             printf("  (min = %+6.2f A)",
-                    get_value(name, sfmin));
-     else if (sfmax)
-             printf("  (max = %+6.2f A)",
-                    get_value(name, sfmax));
+     sensor_count = alarm_count = 0;
+     get_sensor_limit_data(name, feature, current_sensors,
+                           sensors, &sensor_count,
+                           alarms, &alarm_count);
 
-     sf = sensors_get_subfeature(name, feature,
-                                 SENSORS_SUBFEATURE_CURR_ALARM);
-     sfmin = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_CURR_MIN_ALARM);
-     sfmax = sensors_get_subfeature(name, feature,
-                                    SENSORS_SUBFEATURE_CURR_MAX_ALARM);
-     if (sfmin || sfmax) {
-             alarm_max = sfmax ? get_value(name, sfmax) : 0;
-             alarm_min = sfmin ? get_value(name, sfmin) : 0;
+     print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
+                  "%s = %+6.2f A");
 
-             if (alarm_min || alarm_max) {
-                     printf(" ALARM (");
-
-                     if (alarm_min)
-                             printf("MIN");
-                     if (alarm_max)
-                             printf("%sMAX", (alarm_min) ? ", " : "");
-
-                     printf(")");
-             }
-     } else if (sf) {
-             printf("   %s",
-                    get_value(name, sf) ? "ALARM" : "");
-     }
-
      printf("\n");
 }
 

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

Review line 33

Quoted 10 lines

(quoted diff) +7 -0

@@ -24,6 +24,29 @@
 
 #include "lib/sensors.h"
 
+/*
+ * Retrieved limits
+ */
+struct sensor_limit_data {
+     double value;
+     const char *name;
+     const char *unit;

Please add a comment on what unit can be used for and the fact that it is optional.

Quoted 9 lines

(quoted diff) +9 -0

+};
+
+/*
+ * Limit data structure. Used to create a table of supported limits for a given
+ * feature.
+ */
+struct sensor_limits {
+     int subfeature;                         /* Limit we are looking for */
+     const struct sensor_limits *exists;     /* Secondary limits if limit

Review line 45

Quoted 3 lines

(quoted diff) +3 -0

+                                                exists */
+     const struct sensor_limits *nexists;    /* Secondary limits if limit
+                                                does not exist */

"Secondary" is a generic term. In the first case, you are talking about a complement of information. In the second case, you a talking about alternatives. I think the terms "Complementary" and "Alternative" would be more descriptive than "Secondary.

Review line 48

Quoted 3 lines

(quoted diff) +3 -0

+     int alarm;                              /* true if this is an alarm */
+     const char *name;                       /* limit name to be printed */
+};

Your use of the word "limit" in the above structures is somewhat misleading, as they are suitable for both limits and alarms, and could presumably be extended to other subfeatures as well in the future (in particular I start thinking that temperature sensor types would fit in there nicely.) I think you should use the word "subfeature" instead.

Review line 52

Quoted 4 lines

(quoted diff) +1 -0

+
 void print_chip_raw(const sensors_chip_name *name);
 void print_chip(const sensors_chip_name *name);
 

I really like the spirit of the patch. Once fixed, it'll be awesome.

Do you have any more patch pending for lm-sensors? I'd like to flush the queue and then schedule the release of lm-sensors 3.3.0.

Quoted 8 lines · Jean Delvare

The first difference is "hyst" being changed to "crit hyst" for critical temperature limit hysteresis. The original code enforced every temperature limit label to 4 characters to keep all lines nicely aligned, and your change breaks this effort. As the hysteresis value was always printed right after the limit it related to, on the same line, the "crit" in front of "hyst" was redundant. With the new code, there might be a new line in between, which isn't so nice. This should be addressed in an incremental patch IMHO (I can take care.)

ok. Problem though is that there are other sensor strings which don't fit into four characters (crit low for temperatures, more for other sensor types). We would have to change those as well, and possibly specify a fixed length for limit strings associated with other sensor types.

Quoted 2 lines

(...)
You want " (" here.

Ok. That means the output will be "ALARM (MIN, MAX)".

Quoted 9 lines

Review

Quoted 8 lines

(...)

(quoted diff) +7 -0

+static void get_sensor_limit_data(const sensors_chip_name *name,
+                               const sensors_feature *feature,
+                               const struct sensor_limits *ts,
+                               struct sensor_limit_data *ts_data,
+                               int *ts_sensors,
+                               struct sensor_limit_data *ts_alarm_data,
+                               int *ts_alarms)

Quoted 5 lines

The names of the last 4 parameters are really confusing... even worse than for print_limits()! Please use the same naming convention for both functions. And in the calling functions, too. And I'm not even sure what "ts" stands for... "temperature sensor", as a legacy of a time where this function was only used to print temperature limits?

Probably because I started with temperatures.

Changed to
const struct sensor_subfeatures *sf, struct sensor_subfeature_data *limits, int *num_limits,
struct sensor_subfeature_data *alarms, int *num_alarms)
which I hope is a bit better.

Quoted 9 lines

(...)
There is something I don't like about this function: you assume that the provided ts, ts_sensors and ts_alarms are such that all the subfeatures from ts (including exists and nexists recursions) will fit in ts_sensors and ts_alarms are. If this assumption fails, you'll overrun the arrays and corrupt the stack, which is pretty bad. The recursive relations between the various ts arrays make it easy to get wrong, methinks.

Quoted 6 lines

Unfortunately I see no way to check the sizes for correctness at build time, which means we'll have to do it at run time. This could be done with the current prototype by having the caller pass the array sizes as the initial values of ts_sensors and ts_alarms. The code would only have to be adjusted a little. Or maybe you have a better idea?

One other idea would be to dynamically allocate data as needed. Not sure if that would be much better.

For now, I added parameters for max_limits and max_alarms.

Quoted 7 lines

Review

Quoted 3 lines

(...)

(quoted diff) +2 -0

+     strcpy(fmt, "%-4s = %+5.1f");
+     strcat(fmt, degstr);

I strongly discourage the use of strcpy and strcat, especially on stack buffers, for both safety and performance reasons. I would have suggested the use of snprintf instead, but...

Sure snprintf would be faster ?

Quoted 14 lines

Quoted 14 lines

(...)

(quoted diff) +13 -0

+static const struct sensor_limits power_inst_highest[] = {
+     { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, NULL, 0, "max" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_max[] = {
+     { SENSORS_SUBFEATURE_POWER_MAX, NULL, NULL, 0, "max" },
+     { SENSORS_SUBFEATURE_POWER_CRIT, NULL, NULL, 0, "crit" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_sensors[] = {
+     { SENSORS_SUBFEATURE_POWER_ALARM, NULL, power_alarms, 1, NULL },

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, power_inst_highest,
+         power_inst_max, 0, "min" },

This deserves a comment at least... if it is correct at all. Why would INPUT_HIGHEST require INPUT_LOWEST? I can imagine a chip providing one without the other (this would even make a lot of sense if you ask me). I also don't get why MAX and CRIT would be mutually exclusive with INPUT_LOWEST and INPUT_HIGHEST. Please explain or fix.

Backwards compatibility issue. "max" and "min" were traditionally used to display "highest" and "lowest", and I did not want to change that.

However, that overlaps with "max" for the new SENSORS_SUBFEATURE_POWER_MAX.

I've changed the highest output to to "hmax" to lift the limitation. Not optimal, since there is still the average max which is also displayed as "max". Please let me know if you have a better idea.

Quoted 3 lines

(...)
Do you have any more patch pending for lm-sensors? I'd like to flush the queue and then schedule the release of lm-sensors 3.3.0.

Nothing major.

I have a driver for MAX16065 in the queue, but it looks like that won't be detectable.

I am also looking into support for DS1721, DS1631, and DS1731. DS1721 is supported by the ds1621 driver, though auto-detection does not work if the chip is in "standard" mode (the driver itself still works, though).

I am waiting for samples for the other chips.

Once I get those, I'll test the driver and update it as/if needed. I don't have an ETA for that, though.

Hi Guenter,

Quoted 10 lines · Guenter Roeck

Quoted 8 lines · Jean Delvare

The first difference is "hyst" being changed to "crit hyst" for critical temperature limit hysteresis. The original code enforced every temperature limit label to 4 characters to keep all lines nicely aligned, and your change breaks this effort. As the hysteresis value was always printed right after the limit it related to, on the same line, the "crit" in front of "hyst" was redundant. With the new code, there might be a new line in between, which isn't so nice. This should be addressed in an incremental patch IMHO (I can take care.)

Quoted 4 lines

ok. Problem though is that there are other sensor strings which don't fit into four characters (crit low for temperatures, more for other sensor types). We would have to change those as well, and possibly specify a fixed length for limit strings associated with other sensor types.

You're right, I've seen this problem too. I'll see if I have ideas how to solve it. This is independent from your patch though, so let's get your patch applied first.

Quoted 4 lines

Quoted 2 lines

(...)
You want " (" here.

Ok. That means the output will be "ALARM (MIN, MAX)".

Correct, as we has before.

Quoted 17 lines

Quoted 9 lines

Review

Quoted 8 lines

(...)

(quoted diff) +7 -0

+static void get_sensor_limit_data(const sensors_chip_name *name,
+                               const sensors_feature *feature,
+                               const struct sensor_limits *ts,
+                               struct sensor_limit_data *ts_data,
+                               int *ts_sensors,
+                               struct sensor_limit_data *ts_alarm_data,
+                               int *ts_alarms)

Quoted 5 lines

The names of the last 4 parameters are really confusing... even worse than for print_limits()! Please use the same naming convention for both functions. And in the calling functions, too. And I'm not even sure what "ts" stands for... "temperature sensor", as a legacy of a time where this function was only used to print temperature limits?

Probably because I started with temperatures.

Quoted 7 lines

Changed to
const struct sensor_subfeatures *sf, struct sensor_subfeature_data *limits, int *num_limits,
struct sensor_subfeature_data *alarms, int *num_alarms)
which I hope is a bit better.

Yes, much better.

Quoted 9 lines

Quoted 9 lines

(...)
There is something I don't like about this function: you assume that the provided ts, ts_sensors and ts_alarms are such that all the subfeatures from ts (including exists and nexists recursions) will fit in ts_sensors and ts_alarms are. If this assumption fails, you'll overrun the arrays and corrupt the stack, which is pretty bad. The recursive relations between the various ts arrays make it easy to get wrong, methinks.

Quoted 8 lines

Quoted 6 lines

Unfortunately I see no way to check the sizes for correctness at build time, which means we'll have to do it at run time. This could be done with the current prototype by having the caller pass the array sizes as the initial values of ts_sensors and ts_alarms. The code would only have to be adjusted a little. Or maybe you have a better idea?

One other idea would be to dynamically allocate data as needed. Not sure if that would be much better.

I thought about it, but I can't think of an efficient way to do it in one pass. And doing it in 2 passes will certainly perform badly.

Quoted 1 line

For now, I added parameters for max_limits and max_alarms.

Yes, this should work fine.

Quoted 9 lines

Quoted 7 lines

Review

Quoted 3 lines

(...)

(quoted diff) +2 -0

+     strcpy(fmt, "%-4s = %+5.1f");
+     strcat(fmt, degstr);

I strongly discourage the use of strcpy and strcat, especially on stack buffers, for both safety and performance reasons. I would have suggested the use of snprintf instead, but...

Sure snprintf would be faster ?

Probably not in this case, no, because the strings your manipulate are small enough. But with larger strings, repeated strcat are pretty slow.

The main issue if the risk of buffer overflow anyway.

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

(...)

(quoted diff) +13 -0

+static const struct sensor_limits power_inst_highest[] = {
+     { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, NULL, 0, "max" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_max[] = {
+     { SENSORS_SUBFEATURE_POWER_MAX, NULL, NULL, 0, "max" },
+     { SENSORS_SUBFEATURE_POWER_CRIT, NULL, NULL, 0, "crit" },
+     { -1, NULL, NULL, 0, NULL }
+};
+
+static const struct sensor_limits power_inst_sensors[] = {
+     { SENSORS_SUBFEATURE_POWER_ALARM, NULL, power_alarms, 1, NULL },

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, power_inst_highest,
+         power_inst_max, 0, "min" },

This deserves a comment at least... if it is correct at all. Why would INPUT_HIGHEST require INPUT_LOWEST? I can imagine a chip providing one without the other (this would even make a lot of sense if you ask me). I also don't get why MAX and CRIT would be mutually exclusive with INPUT_LOWEST and INPUT_HIGHEST. Please explain or fix.

Quoted 2 lines

Backwards compatibility issue. "max" and "min" were traditionally used to display "highest" and "lowest", and I did not want to change that.

But the origial code supported max (highest) without min (lowest) just fine, which you code does not.

Quite frankly, I wouldn't bother about backwards compatibility here. For one thing, power sensors are relatively recent, so users probably don't expect things to be carved in stone yet. For another, using "min" and "max" for "lowest" and "highest" was a bad choice in the first place. We have traditionally used "min" and "max" for limits, while what we have here are measurement extremes. Re-using "min" and "max" can only confuse the user. So I would use "lowest" and "highest" to clearly show the difference.

Quoted 5 lines

However, that overlaps with "max" for the new SENSORS_SUBFEATURE_POWER_MAX.

I've changed the highest output to to "hmax" to lift the limitation. Not optimal, since there is still the average max which is also displayed as "max". Please let me know if you have a better idea.

Instant and average power sensors are supposed to be mutually exclusive, and so are their limits, so I don't see any problem here.

Quoted 11 lines

Quoted 3 lines

(...)
Do you have any more patch pending for lm-sensors? I'd like to flush the queue and then schedule the release of lm-sensors 3.3.0.

Nothing major.

I have a driver for MAX16065 in the queue, but it looks like that won't be detectable.

I am also looking into support for DS1721, DS1631, and DS1731. DS1721 is supported by the ds1621 driver, though auto-detection does not work if the chip is in "standard" mode (the driver itself still works, though).

The ds1621 driver shouldn't do device detection in the first place, it is hardly reliable as the devices lack an ID register.

Quoted 1 line

I am waiting for samples for the other chips.

I have a DS1631 somewhere in a drawer. I asked Maxim for the samples long ago and never got to add support for these. Shame on me. If you work on this, I'll be happy to help with review and testing.

Quoted 2 lines

Once I get those, I'll test the driver and update it as/if needed. I don't have an ETA for that, though.

And all this is on the kernel side and not lm-sensors anyway. So let's get your patch finished, add a few improvements on top of it, and prepare for the release.

Quoted 7 lines · Jean Delvare

Quoted 2 lines · Guenter Roeck

Backwards compatibility issue. "max" and "min" were traditionally used to display "highest" and "lowest", and I did not want to change that.

But the origial code supported max (highest) without min (lowest) just fine, which you code does not.

Quoted 9 lines

Quite frankly, I wouldn't bother about backwards compatibility here. For one thing, power sensors are relatively recent, so users probably don't expect things to be carved in stone yet. For another, using "min" and "max" for "lowest" and "highest" was a bad choice in the first place. We have traditionally used "min" and "max" for limits, while what we have here are measurement extremes. Re-using "min" and "max" can only confuse the user. So I would use "lowest" and "highest" to clearly show the difference.

Ok, I'll luse "lowest" and "highest". Really makes much more sense.

Quoted 2 lines

Quoted 2 lines

However, that overlaps with "max" for the new SENSORS_SUBFEATURE_POWER_MAX.

Quoted 7 lines

Quoted 3 lines

I've changed the highest output to to "hmax" to lift the limitation. Not optimal, since there is still the average max which is also displayed as "max". Please let me know if you have a better idea.

Instant and average power sensors are supposed to be mutually exclusive, and so are their limits, so I don't see any problem here.

Should I use "lowest" and "highest" here as well for consistency ?

Quoted 5 lines

Quoted 4 lines

(...)
I am also looking into support for DS1721, DS1631, and DS1731. DS1721 is supported by the ds1621 driver, though auto-detection does not work if the chip is in "standard" mode (the driver itself still works, though).

Quoted 9 lines

The ds1621 driver shouldn't do device detection in the first place, it is hardly reliable as the devices lack an ID register.

Quoted 1 line

I am waiting for samples for the other chips.

I have a DS1631 somewhere in a drawer. I asked Maxim for the samples long ago and never got to add support for these. Shame on me. If you work on this, I'll be happy to help with review and testing.

Sounds good.

Quoted 3 lines

Quoted 2 lines

Once I get those, I'll test the driver and update it as/if needed. I don't have an ETA for that, though.

Quoted 4 lines

And all this is on the kernel side and not lm-sensors anyway. So let's get your patch finished, add a few improvements on top of it, and prepare for the release.

Mostly kernel, though I wanted to amend sensors-detect if I can for the DSxxxx chips. But that should not hold up the new version of lm-sensors.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

However, that overlaps with "max" for the new SENSORS_SUBFEATURE_POWER_MAX.

I've changed the highest output to to "hmax" to lift the limitation. Not optimal, since there is still the average max which is also displayed as "max". Please let me know if you have a better idea.

Quoted 4 lines

Quoted 3 lines

Instant and average power sensors are supposed to be mutually exclusive, and so are their limits, so I don't see any problem here.

Should I use "lowest" and "highest" here as well for consistency ?

Yes, please.

Regarding formatting, there is also "emergency" and "emergency hyst".

Also, there can be temperature limits above 100C, which affects formatting as well. So it won't be easy to keep everything aligned.

Some sample output:

max6696-i2c-1-19
Adapter: Phalanx i2c channel 1
temp1: +21.6 C (low = -55.0 C, high = +70.0 C)

                       (crit = +70.0 C, crit hyst = +60.0 C)
                       (emergency = +90.0 C, emergency hyst = +80.0 C)
temp2:          FAULT  (low  = -55.0 C, high = +70.0 C)  ALARM (MIN)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)
temp3:          FAULT  (low  = -55.0 C, high = +70.0 C)  ALARM (MIN)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)

max6696-i2c-100-18

Adapter: SMBus I801 adapter at 5080
temp1: +22.4 C (low = -55.0 C, high = +70.0 C)

                       (crit = +70.0 C, crit hyst = +60.0 C)
                       (emergency = +90.0 C, emergency hyst = +80.0 C)
temp2:        +81.1 C  (low  = -55.0 C, high = +70.0 C)  ALARM (MAX)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)
temp3:        +22.8 C  (low  = -55.0 C, high = +70.0 C)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)

jc42-i2c-100-1a
Adapter: SMBus I801 adapter at 5080
temp1: +26.2 C (low = +0.0 C, high = +0.0 C) ALARM (MAX, CRIT)

                       (hyst =  +0.0 C, crit =  +0.0 C)

                       (crit hyst =  +0.0 C)

lineage_pem-i2c-61-45
Adapter: i2c-55-mux (chan_id 0)
in1: +0.00 V ALARM (MAX)

in2:          +0.00 V                                    ALARM
fan1:             N/A
temp1:        +34.0 C  (high = +97.0 C, crit = +107.0 C)
power1:        0.00 nW  
curr1:        +0.00 A  

Quoted 1 line · Guenter Roeck

Regarding formatting, there is also "emergency" and "emergency hyst".

I'd seen this, yes. "emergency" could be shorten to "emerg" (after all we already shortened "critical" to "crit".) For hysteresis, my plan is to ensure it's always on the same line as the limit it relates to, so "hyst" will always be enough.

An alternative would be to compute the max limit name size for each sensor. However, this would not guarantee per-device alignment, only per-sensor. This would also increase the code complexity, and I'm not sure if it's worth it. Opinion?

Quoted 2 lines

Also, there can be temperature limits above 100C, which affects formatting as well. So it won't be easy to keep everything aligned.

This can be easily addressed by adding a digit to the values. This only adds 3 characters per line, it's probably acceptable. The only drawback is that it won't keep the different sensor types aligned any longer, unless we also do the same for them. Which might be an option after all...

Quoted 14 lines

Some sample output:

max6696-i2c-1-19
Adapter: Phalanx i2c channel 1
temp1: +21.6 C (low = -55.0 C, high = +70.0 C)

                       (crit = +70.0 C, crit hyst = +60.0 C)
                       (emergency = +90.0 C, emergency hyst = +80.0 C)
temp2:          FAULT  (low  = -55.0 C, high = +70.0 C)  ALARM (MIN)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)
temp3:          FAULT  (low  = -55.0 C, high = +70.0 C)  ALARM (MIN)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)

Quoted 1 line

max6696-i2c-100-18

You have interesting I2C bus numbers :p

Quoted 10 lines

Adapter: SMBus I801 adapter at 5080
temp1: +22.4 C (low = -55.0 C, high = +70.0 C)

                       (crit = +70.0 C, crit hyst = +60.0 C)
                       (emergency = +90.0 C, emergency hyst = +80.0 C)
temp2:        +81.1 C  (low  = -55.0 C, high = +70.0 C)  ALARM (MAX)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)
temp3:        +22.8 C  (low  = -55.0 C, high = +70.0 C)
                       (crit = +90.0 C, crit hyst = +80.0 C)
                       (emergency = +120.0 C, emergency hyst = +110.0 C)

Quoted 3 lines

jc42-i2c-100-1a
Adapter: SMBus I801 adapter at 5080
temp1: +26.2 C (low = +0.0 C, high = +0.0 C) ALARM (MAX, CRIT)

The "MAX" in alarm is inconsistent with the "high" label... We should use LOW and HIGH for temperature alarms, not MIN and MAX.

Quoted 1 line

                       (hyst =  +0.0 C, crit =  +0.0 C)

Here hyst is separated from high, which makes it non-obvious that they relate to each other. I'll improve this, and I'm glad you have a test case ;)

Quoted 5 lines

                       (crit hyst =  +0.0 C)

lineage_pem-i2c-61-45
Adapter: i2c-55-mux (chan_id 0)
in1: +0.00 V ALARM (MAX)

Max alarm without max limit? Did you hack the driver for testing purpose?

I think you're missing one space here, as an ALARM on the temp1 line would have 2 spaces before ALARM.

Quoted 5 lines

in2:          +0.00 V                                    ALARM
fan1:             N/A
temp1:        +34.0 C  (high = +97.0 C, crit = +107.0 C)
power1:        0.00 nW  
curr1:        +0.00 A  

Quoted 5 lines · Jean Delvare

I'd seen this, yes. "emergency" could be shorten to "emerg" (after all we already shortened "critical" to "crit".) For hysteresis, my plan is to ensure it's always on the same line as the limit it relates to, so "hyst" will always be enough.

Ok, I'll make it "emerg". Or maybe "emrg" to make it fit into four characters ?

Quoted 2 lines

(...)
You have interesting I2C bus numbers :p

The system has more than 50 virtual (ie multiplexed) and real I2C busses. There are some gaps to keep numbers aligned. I can send you the complete sensors output if you like ... must be the best monitored system in the world.

Quoted 8 lines

Quoted 4 lines

(...)
jc42-i2c-100-1a
Adapter: SMBus I801 adapter at 5080
temp1: +26.2 C (low = +0.0 C, high = +0.0 C) ALARM (MAX, CRIT)

The "MAX" in alarm is inconsistent with the "high" label... We should use LOW and HIGH for temperature alarms, not MIN and MAX.

Fine with me. No backwards compatibility concerns ?

Quoted 4 lines

(...)
I think you're missing one space here, as an ALARM on the temp1 line would have 2 spaces before ALARM.

That is because the "crit" temperature has three digits.

Quoted 4 lines

Quoted 4 lines

in2:          +0.00 V                                    ALARM
fan1:             N/A
temp1:        +34.0 C  (high = +97.0 C, crit = +107.0 C)
power1:        0.00 nW  

Quoted 1 line

Quoted 1 line

curr1: +0.00 A

Quoted 8 lines · Guenter Roeck

Quoted 5 lines · Jean Delvare

I'd seen this, yes. "emergency" could be shorten to "emerg" (after all we already shortened "critical" to "crit".) For hysteresis, my plan is to ensure it's always on the same line as the limit it relates to, so "hyst" will always be enough.

Ok, I'll make it "emerg". Or maybe "emrg" to make it fit into four characters ?

"emrg" is certainly the easiest approach, yes. I just wasn't sure if it would be clear enough for the users.

Note that there are still "lowest" and "highest" which are longer than 4 chars, and which we won't be able to shorten, so focusing on the "emergency" case may not be the best thing to do.

One thing worth noting is that neither of these 3 long strings are relevant for the typical PC user (which I admit is the one I mostly care about) so in fact I don't personally care if they break alignment, and it is quite possible that the affected users don't care either.

Quoted 7 lines

Quoted 2 lines

(...)
You have interesting I2C bus numbers :p

The system has more than 50 virtual (ie multiplexed) and real I2C busses. There are some gaps to keep numbers aligned. I can send you the complete sensors output if you like ... must be the best monitored system in the world.

I'm very happy to see that apparently the i2c core is able to cope nicely with this amount of devices :)

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

(...)
jc42-i2c-100-1a
Adapter: SMBus I801 adapter at 5080
temp1: +26.2 C (low = +0.0 C, high = +0.0 C) ALARM (MAX, CRIT)

Quoted 4 lines

Quoted 3 lines

The "MAX" in alarm is inconsistent with the "high" label... We should use LOW and HIGH for temperature alarms, not MIN and MAX.

Fine with me. No backwards compatibility concerns ?

No. Limit-specific alarm flags are relatively recent, and most often not available on PC mainboard monitoring devices, so the impact of the change is low.

Quoted 5 lines

Quoted 4 lines

(...)
I think you're missing one space here, as an ALARM on the temp1 line would have 2 spaces before ALARM.

That is because the "crit" temperature has three digits.

Ah, yes, the very point you were making; sorry for being distracted.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

in2:          +0.00 V                                    ALARM
fan1:             N/A
temp1:        +34.0 C  (high = +97.0 C, crit = +107.0 C)
power1:        0.00 nW  

Apparently our unit selection algorithm picks the smallest unit if value is 0? Wouldn't it make more sense to pick the base unit instead (W in this case)?

Quoted 4 lines · Jean Delvare

Apparently our unit selection algorithm picks the smallest unit if value is 0? Wouldn't it make more sense to pick the base unit instead (W in this case)?

I'll add

	if (abs_value = 0) {
                *prefixstr = "";
                return;
        }

to the beginning of scale_value(). Seems to be the easiest fix.