* [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon
@ 2010-07-08 5:43 Guenter Roeck
2010-12-09 15:06 ` Jean Delvare
` (5 more replies)
0 siblings, 6 replies; 7+ messages in thread
From: Guenter Roeck @ 2010-07-08 5:43 UTC (permalink / raw)
To: lm-sensors
This patch adds support for the following new attributes to libsensors and
to the sensors command.
inX_lcrit
inX_crit
inX_lcrit_alarm
inX_crit_alarm
temp_lcrit
temp_emergency
temp_lcrit_alarm
powerX_cap
powerX_max
powerX_crit
powerX_alarm
powerX_crit_alarm
currX_lcrit
currX_crit
currX_lcrit_alarm
currX_crit_alarm
I reworked print_temp_limits() significantly, which asks for a detailed review.
Idea is to make the function better scalable for a large number of attributes
while retaining at least some compatibility with legacy output.
sensord does not yet support the new attributes. The sensord code is a bit more
complex than sensors, and I don't have a clean solution for it yet.
---
Index: lib/sensors.h
=================================--- lib/sensors.h (revision 5851)
+++ lib/sensors.h (working copy)
@@ -151,10 +151,14 @@ typedef enum sensors_subfeature_type {
SENSORS_SUBFEATURE_IN_INPUT = SENSORS_FEATURE_IN << 8,
SENSORS_SUBFEATURE_IN_MIN,
SENSORS_SUBFEATURE_IN_MAX,
+ SENSORS_SUBFEATURE_IN_LCRIT,
+ SENSORS_SUBFEATURE_IN_CRIT,
SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
SENSORS_SUBFEATURE_IN_MIN_ALARM,
SENSORS_SUBFEATURE_IN_MAX_ALARM,
SENSORS_SUBFEATURE_IN_BEEP,
+ SENSORS_SUBFEATURE_IN_LCRIT_ALARM,
+ SENSORS_SUBFEATURE_IN_CRIT_ALARM,
SENSORS_SUBFEATURE_FAN_INPUT = SENSORS_FEATURE_FAN << 8,
SENSORS_SUBFEATURE_FAN_MIN,
@@ -169,6 +173,8 @@ typedef enum sensors_subfeature_type {
SENSORS_SUBFEATURE_TEMP_MIN,
SENSORS_SUBFEATURE_TEMP_CRIT,
SENSORS_SUBFEATURE_TEMP_CRIT_HYST,
+ SENSORS_SUBFEATURE_TEMP_LCRIT,
+ SENSORS_SUBFEATURE_TEMP_EMERGENCY,
SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
SENSORS_SUBFEATURE_TEMP_MIN_ALARM,
@@ -177,6 +183,7 @@ typedef enum sensors_subfeature_type {
SENSORS_SUBFEATURE_TEMP_TYPE,
SENSORS_SUBFEATURE_TEMP_OFFSET,
SENSORS_SUBFEATURE_TEMP_BEEP,
+ SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM,
SENSORS_SUBFEATURE_POWER_AVERAGE = SENSORS_FEATURE_POWER << 8,
SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST,
@@ -184,6 +191,11 @@ typedef enum sensors_subfeature_type {
SENSORS_SUBFEATURE_POWER_INPUT,
SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST,
SENSORS_SUBFEATURE_POWER_INPUT_LOWEST,
+ SENSORS_SUBFEATURE_POWER_CAP,
+ SENSORS_SUBFEATURE_POWER_MAX,
+ SENSORS_SUBFEATURE_POWER_CRIT,
+ SENSORS_SUBFEATURE_POWER_ALARM,
+ SENSORS_SUBFEATURE_POWER_CRIT_ALARM,
SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL = (SENSORS_FEATURE_POWER << 8) | 0x80,
SENSORS_SUBFEATURE_ENERGY_INPUT = SENSORS_FEATURE_ENERGY << 8,
@@ -191,10 +203,14 @@ typedef enum sensors_subfeature_type {
SENSORS_SUBFEATURE_CURR_INPUT = SENSORS_FEATURE_CURR << 8,
SENSORS_SUBFEATURE_CURR_MIN,
SENSORS_SUBFEATURE_CURR_MAX,
+ SENSORS_SUBFEATURE_CURR_LCRIT,
+ SENSORS_SUBFEATURE_CURR_CRIT,
SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
SENSORS_SUBFEATURE_CURR_MIN_ALARM,
SENSORS_SUBFEATURE_CURR_MAX_ALARM,
SENSORS_SUBFEATURE_CURR_BEEP,
+ SENSORS_SUBFEATURE_CURR_LCRIT_ALARM,
+ SENSORS_SUBFEATURE_CURR_CRIT_ALARM,
SENSORS_SUBFEATURE_VID = SENSORS_FEATURE_VID << 8,
Index: lib/sensors.conf.5
=================================--- lib/sensors.conf.5 (revision 5851)
+++ lib/sensors.conf.5 (working copy)
@@ -402,7 +402,7 @@ really cool before the fan stops, so that it will
again immediately.
So, in addition to tempX_max, many chips have a tempX_max_hyst
-sub-feature. Likewise, tempX_crit often comes with tempX_max_crit.
+sub-feature. Likewise, tempX_crit often comes with tempX_crit_hyst.
Example:
.RS
Index: lib/sysfs.c
=================================--- lib/sysfs.c (revision 5851)
+++ lib/sysfs.c (working copy)
@@ -219,11 +219,14 @@ static const struct subfeature_type_match temp_mat
{ "max", SENSORS_SUBFEATURE_TEMP_MAX },
{ "max_hyst", SENSORS_SUBFEATURE_TEMP_MAX_HYST },
{ "min", SENSORS_SUBFEATURE_TEMP_MIN },
+ { "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
{ "crit", SENSORS_SUBFEATURE_TEMP_CRIT },
{ "crit_hyst", SENSORS_SUBFEATURE_TEMP_CRIT_HYST },
+ { "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
{ "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
{ "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
{ "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },
+ { "lcrit_alarm", SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM },
{ "crit_alarm", SENSORS_SUBFEATURE_TEMP_CRIT_ALARM },
{ "fault", SENSORS_SUBFEATURE_TEMP_FAULT },
{ "type", SENSORS_SUBFEATURE_TEMP_TYPE },
@@ -236,9 +239,13 @@ static const struct subfeature_type_match in_match
{ "input", SENSORS_SUBFEATURE_IN_INPUT },
{ "min", SENSORS_SUBFEATURE_IN_MIN },
{ "max", SENSORS_SUBFEATURE_IN_MAX },
+ { "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
+ { "crit", SENSORS_SUBFEATURE_IN_CRIT },
{ "alarm", SENSORS_SUBFEATURE_IN_ALARM },
{ "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
{ "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },
+ { "lcrit_alarm", SENSORS_SUBFEATURE_IN_LCRIT_ALARM },
+ { "crit_alarm", SENSORS_SUBFEATURE_IN_CRIT_ALARM },
{ "beep", SENSORS_SUBFEATURE_IN_BEEP },
{ NULL, 0 }
};
@@ -260,6 +267,11 @@ static const struct subfeature_type_match power_ma
{ "input", SENSORS_SUBFEATURE_POWER_INPUT },
{ "input_highest", SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST },
{ "input_lowest", SENSORS_SUBFEATURE_POWER_INPUT_LOWEST },
+ { "cap", SENSORS_SUBFEATURE_POWER_CAP },
+ { "max", SENSORS_SUBFEATURE_POWER_MAX },
+ { "crit", SENSORS_SUBFEATURE_POWER_CRIT },
+ { "alarm", SENSORS_SUBFEATURE_POWER_ALARM },
+ { "crit_alarm", SENSORS_SUBFEATURE_POWER_CRIT_ALARM },
{ "average_interval", SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL },
{ NULL, 0 }
};
@@ -273,9 +285,13 @@ static const struct subfeature_type_match curr_mat
{ "input", SENSORS_SUBFEATURE_CURR_INPUT },
{ "min", SENSORS_SUBFEATURE_CURR_MIN },
{ "max", SENSORS_SUBFEATURE_CURR_MAX },
+ { "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
+ { "crit", SENSORS_SUBFEATURE_CURR_CRIT },
{ "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
{ "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
{ "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },
+ { "lcrit_alarm", SENSORS_SUBFEATURE_CURR_LCRIT_ALARM },
+ { "crit_alarm", SENSORS_SUBFEATURE_CURR_CRIT_ALARM },
{ "beep", SENSORS_SUBFEATURE_CURR_BEEP },
{ NULL, 0 }
};
Index: prog/sensors/chips.c
=================================--- prog/sensors/chips.c (revision 5851)
+++ prog/sensors/chips.c (working copy)
@@ -123,37 +123,44 @@ static int get_label_size(const sensors_chip_name
return max_size + 1;
}
-static void print_temp_limits(double limit1, double limit2,
- const char *name1, const char *name2, int alarm)
+static void print_temp_limits(double limit[], const char *name[],
+ int count, int alarm, int label_size)
{
- if (fahrenheit) {
- limit1 = deg_ctof(limit1);
- limit2 = deg_ctof(limit2);
- }
+ int i;
- if (name2) {
- printf("(%-4s = %+5.1f%s, %-4s = %+5.1f%s) ",
- name1, limit1, degstr,
- name2, limit2, degstr);
- } else if (name1) {
- printf("(%-4s = %+5.1f%s) ",
- name1, limit1, degstr);
- } else {
- printf(" ");
+ if (fahrenheit)
+ for (i=0; i<count; i++)
+ limit[i] = deg_ctof(limit[i]);
+
+ for (i=0; i<count; i++) {
+ if (!(i&1)) {
+ if (i)
+ printf("\n%*s", label_size + 10, "");
+ printf("(");
+ } else if (i)
+ printf(", ");
+ printf("%-4s = %+5.1f%s", name[i], limit[i], degstr);
+ if ((i&1) || i = count-1)
+ printf(") ");
}
- if (alarm)
- printf("ALARM ");
+ if (alarm & (2|8)) /* min/max (crit) alarm */
+ printf(" %sALARM(MIN%s)", (alarm & 8) ? "CRIT " : "",
+ (alarm &(1|4)) ? ", MAX" : "");
+ else if (alarm & (1|4)) /* max (crit) alarm */
+ printf(" %sALARM ", (alarm & 4) ? "CRIT " : "");
}
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;
+ const sensors_subfeature *sf, *sfmin, *sfmax, *sflcrit, *sfcrit,
+ *sfhyst, *sfemerg;
+ double val, limit[7];
+ const char *s[7];
+ int alarm;
+ int count = 0;
char *label;
if (!(label = sensors_get_label(name, feature))) {
@@ -172,8 +179,12 @@ static void print_chip_temp(const sensors_chip_nam
SENSORS_SUBFEATURE_TEMP_MIN);
sfmax = sensors_get_subfeature(name, feature,
SENSORS_SUBFEATURE_TEMP_MAX);
+ sflcrit = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_TEMP_LCRIT);
sfcrit = sensors_get_subfeature(name, feature,
SENSORS_SUBFEATURE_TEMP_CRIT);
+ sfemerg = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_TEMP_EMERGENCY);
if (sfmax) {
sf = sensors_get_subfeature(name, feature,
SENSORS_SUBFEATURE_TEMP_MAX_ALARM);
@@ -181,63 +192,56 @@ static void print_chip_temp(const sensors_chip_nam
alarm |= 1;
if (sfmin) {
- limit1 = get_value(name, sfmin);
- s1 = "low";
- limit2 = get_value(name, sfmax);
- s2 = "high";
+ limit[count] = get_value(name, sfmin);
+ s[count++] = "low";
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";
+ alarm |= 2;
+ }
+ limit[count] = get_value(name, sfmax);
+ s[count++] = "high";
- sfhyst = sensors_get_subfeature(name, feature,
+ 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;
- }
+ if (sfhyst) {
+ limit[count] = get_value(name, sfhyst);
+ s[count++] = "hyst";
}
- } else if (sfcrit) {
- limit1 = get_value(name, sfcrit);
- s1 = "crit";
+ }
+ if (sflcrit) {
+ limit[count] = get_value(name, sflcrit);
+ s[count++] = "crit low";
+ }
+ if (sfcrit) {
+ limit[count] = get_value(name, sfcrit);
+ s[count++] = sflcrit ? "crit high" : "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;
+ limit[count] = get_value(name, sfhyst);
+ s[count++] = "crit hyst";
}
-
+ }
+ if (sfcrit) {
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;
+ alarm |= 4;
}
+ if (sflcrit) {
+ sf = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM);
+ if (sf && get_value(name, sf))
+ alarm |= 8;
+ }
+ if (sfemerg) {
+ limit[count] = get_value(name, sfemerg);
+ s[count++] = "emergency";
+ }
-
sf = sensors_get_subfeature(name, feature,
SENSORS_SUBFEATURE_TEMP_FAULT);
if (sf && get_value(name, sf)) {
@@ -253,30 +257,9 @@ static void print_chip_temp(const sensors_chip_nam
} else
printf(" N/A ");
}
- print_temp_limits(limit1, limit2, s1, s2, alarm);
- if (!crit_displayed && sfcrit) {
- limit1 = get_value(name, sfcrit);
- s1 = "crit";
+ print_temp_limits(limit, s, count, alarm, label_size);
- 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);
- alarm = sf && get_value(name, sf);
-
- 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);
@@ -303,9 +286,10 @@ static void print_chip_in(const sensors_chip_name
const sensors_feature *feature,
int label_size)
{
- const sensors_subfeature *sf, *sfmin, *sfmax;
+ const sensors_subfeature *sf, *sfmin, *sfmax, *sflcrit, *sfcrit;
double val, alarm_max, alarm_min;
char *label;
+ int crit;
if (!(label = sensors_get_label(name, feature))) {
fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
@@ -337,13 +321,61 @@ static void print_chip_in(const sensors_chip_name
printf(" (max = %+6.2f V)",
get_value(name, sfmax));
+ sflcrit = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_IN_LCRIT);
+ sfcrit = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_IN_CRIT);
+ if (sflcrit && sfcrit)
+ printf("\n%*s(crit min = %+6.2f V, crit max = %+6.2f V)",
+ label_size+10, "",
+ get_value(name, sflcrit),
+ get_value(name, sfcrit));
+ else if (sflcrit)
+ printf("\n%*s(crit min = %+6.2f V)",
+ label_size + 10, "",
+ get_value(name, sflcrit));
+ else if (sfcrit)
+ printf("\n%*s(crit max = %+6.2f V)",
+ label_size + 10, "",
+ get_value(name, sfcrit));
+
+ crit = 0;
+ sfmin = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_IN_LCRIT_ALARM);
+ sfmax = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_IN_CRIT_ALARM);
+ if (sfmin && sfmax) {
+ alarm_max = sfmax ? get_value(name, sfmax) : 0;
+ alarm_min = sfmin ? get_value(name, sfmin) : 0;
+
+ if (alarm_min || alarm_max) {
+ printf(" CRIT ALARM (");
+
+ if (alarm_min)
+ printf("MIN");
+ if (alarm_max)
+ printf("%sMAX", alarm_min ? ", " : "");
+
+ printf(")");
+ crit = 1;
+ }
+ } else if (sfmax) {
+ alarm_max = get_value(name, sf);
+ if (alarm_max) {
+ printf(" CRIT ALARM");
+ crit = 1;
+ }
+ }
+
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) {
+
+ /* Only display non-critical alarms if there were no critical alarms */
+ if (!crit && (sfmin || sfmax)) {
alarm_max = sfmax ? get_value(name, sfmax) : 0;
alarm_min = sfmin ? get_value(name, sfmin) : 0;
@@ -357,7 +389,7 @@ static void print_chip_in(const sensors_chip_name
printf(")");
}
- } else if (sf) {
+ } else if (!crit && sf) {
printf(" %s",
get_value(name, sf) ? "ALARM" : "");
}
@@ -453,9 +485,10 @@ static void print_chip_power(const sensors_chip_na
{
double val;
int need_space = 0;
- const sensors_subfeature *sf, *sfmin, *sfmax, *sfint;
+ const sensors_subfeature *sf, *sfmin, *sfmax, *sfint, *sfcrit, *sfcap;
char *label;
const char *unit;
+ double crit = 0;
if (!(label = sensors_get_label(name, feature))) {
fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
@@ -475,7 +508,20 @@ static void print_chip_power(const sensors_chip_na
SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST);
sfmax = sensors_get_subfeature(name, feature,
SENSORS_SUBFEATURE_POWER_INPUT_LOWEST);
+ sfcap = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_POWER_CAP);
sfint = NULL;
+ sfcrit = NULL;
+ /*
+ * Maybe we have power_max and power_crit instead of highest/lowest
+ */
+ if (!sfmax) {
+ sfmin = NULL;
+ sfmax = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_POWER_MAX);
+ sfcrit = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_POWER_CRIT);
+ }
} else {
sf = sensors_get_subfeature(name, feature,
SENSORS_SUBFEATURE_POWER_AVERAGE);
@@ -485,6 +531,8 @@ static void print_chip_power(const sensors_chip_na
SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST);
sfint = sensors_get_subfeature(name, feature,
SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL);
+ sfcrit = NULL;
+ sfcap = NULL;
}
if (sf && get_input_value(name, sf, &val) = 0) {
@@ -493,7 +541,7 @@ static void print_chip_power(const sensors_chip_na
} else
printf(" N/A");
- if (sfmin || sfmax || sfint) {
+ if (sfmin || sfmax || sfcap || sfint || sfcrit) {
printf(" (");
if (sfmin) {
@@ -511,6 +559,22 @@ static void print_chip_power(const sensors_chip_na
need_space = 1;
}
+ if (sfcap) {
+ val = get_value(name, sfcap);
+ scale_value(&val, &unit);
+ printf("%scap = %6.2f %sW", (need_space ? ", " : ""),
+ val, unit);
+ need_space = 1;
+ }
+
+ if (sfcrit) {
+ val = get_value(name, sfcrit);
+ scale_value(&val, &unit);
+ printf("%scrit = %6.2f %sW", (need_space ? ", " : ""),
+ val, unit);
+ need_space = 1;
+ }
+
if (sfint) {
printf("%sinterval = %6.2f s", (need_space ? ", " : ""),
get_value(name, sfint));
@@ -519,6 +583,20 @@ static void print_chip_power(const sensors_chip_na
printf(")");
}
+ sf = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_POWER_CRIT_ALARM);
+ if (sf) {
+ crit = get_value(name, sf);
+ if (crit)
+ printf(" %s", "CRIT ALARM");
+ }
+ if (!crit) {
+ sf = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_POWER_ALARM);
+ if (sf)
+ printf(" %s", get_value(name, sf) ? "ALARM" : "");
+ }
+
printf("\n");
}
@@ -596,7 +674,7 @@ static void print_chip_curr(const sensors_chip_nam
const sensors_feature *feature,
int label_size)
{
- const sensors_subfeature *sf, *sfmin, *sfmax;
+ const sensors_subfeature *sf, *sfmin, *sfmax, *sfcrit, *sflcrit;
double alarm_max, alarm_min, val;
char *label;
@@ -630,18 +708,35 @@ static void print_chip_curr(const sensors_chip_nam
printf(" (max = %+6.2f A)",
get_value(name, sfmax));
- sf = sensors_get_subfeature(name, feature,
- SENSORS_SUBFEATURE_CURR_ALARM);
+ sflcrit = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_CURR_LCRIT);
+ sfcrit = sensors_get_subfeature(name, feature,
+ SENSORS_SUBFEATURE_CURR_CRIT);
+ if (sflcrit && sfcrit)
+ printf("\n%*s(crit min = %+6.2f A, crit max = %+6.2f A)",
+ label_size+10, "",
+ get_value(name, sflcrit),
+ get_value(name, sfcrit));
+ else if (sflcrit)
+ printf("\n%*s(crit min = %+6.2f A)",
+ label_size + 10, "",
+ get_value(name, sflcrit));
+ else if (sfcrit)
+ printf("\n%*s(crit max = %+6.2f A)",
+ label_size + 10, "",
+ get_value(name, sfcrit));
+
+ alarm_min = alarm_max = 0;
sfmin = sensors_get_subfeature(name, feature,
- SENSORS_SUBFEATURE_CURR_MIN_ALARM);
+ SENSORS_SUBFEATURE_CURR_LCRIT_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;
+ SENSORS_SUBFEATURE_CURR_CRIT_ALARM);
+ if (sfmin && sfmax) {
+ alarm_max = get_value(name, sfmax);
+ alarm_min = get_value(name, sfmin);
if (alarm_min || alarm_max) {
- printf(" ALARM (");
+ printf(" CRIT ALARM (");
if (alarm_min)
printf("MIN");
@@ -650,11 +745,40 @@ static void print_chip_curr(const sensors_chip_nam
printf(")");
}
- } else if (sf) {
- printf(" %s",
- get_value(name, sf) ? "ALARM" : "");
+ } else if (sfmax) {
+ alarm_max = get_value(name, sfmax);
+ if (alarm_max)
+ printf(" CRIT ALARM");
}
+ /* Only display non-critical alarms if there were no critical alarms */
+ if (!alarm_min && !alarm_max) {
+ 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;
+
+ 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");
}
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon
2010-07-08 5:43 [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon Guenter Roeck
@ 2010-12-09 15:06 ` Jean Delvare
2010-12-09 15:40 ` Guenter Roeck
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2010-12-09 15:06 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
Sorry for the very late reply <sigh>.
On Wed, 7 Jul 2010 22:43:33 -0700, Guenter Roeck wrote:
> This patch adds support for the following new attributes to libsensors and
> to the sensors command.
>
> inX_lcrit
> inX_crit
> inX_lcrit_alarm
> inX_crit_alarm
> temp_lcrit
> temp_emergency
> temp_lcrit_alarm
> powerX_cap
> powerX_max
> powerX_crit
> powerX_alarm
> powerX_crit_alarm
> currX_lcrit
> currX_crit
> currX_lcrit_alarm
> currX_crit_alarm
Many of these (inX_lcrit_alarm, inX_crit_alarm, powerX_max,
powerX_crit, powerX_crit_alarm, currX_lcrit, currX_crit,
currX_lcrit_alarm, currX_crit_alarm) aren't in
Documentation/hwmon/sysfs-interface, so we can't add support in
libsensors yet.
OTOH some attributes that have been added to
Documentation/hwmon/sysfs-interface aren't present in your patch:
temp[1-*]_emergency_hyst, temp[1-*]_emergency_alarm,
power[1-*]_cap_hyst. Maybe you want to add them.
It is very important that libsensors sticks to what is documented in
file sysfs-interface, otherwise drivers will implement attributes
libsensors will ignore, and vice versa. So please add the missing
attributes to Documentation/hwmon/sysfs-interface, and then adjust the
libsensors patch to be in line with this.
It is entirely possible that you submitted such changes in the past and
I overlooked them, in which case I apologize. Please resend. I'm doing
what I can in the little spare time I have...
> I reworked print_temp_limits() significantly, which asks for a detailed review.
> Idea is to make the function better scalable for a large number of attributes
> while retaining at least some compatibility with legacy output.
This is tricky, and I admit I don't know what to do here. The balance
between nice output and nice code is hard to find. FWIW, I don't mind a
minor "regression" in the output if this makes the code much simpler.
BTW, feel free to split the patch into libsensors changes and sensors
changes. The libsensors part should be relatively straightforward to
get right and merge, so my limited review time doesn't block it. The
sensors part is a totally different beast.
I suspect you'll have to rebase your patch anyway, as this version is
getting old and there were quite a few commits to both libsensors and
sensors meanwhile.
> sensord does not yet support the new attributes. The sensord code is a bit more
> complex than sensors, and I don't have a clean solution for it yet.
You really don't have to care about sensord if you do not have any use
for it. This is just one libsensors-based application amongst many
others, it should even live outside of the main lm-sensors package
IMHO. Anyone interested in adding support for (some of) the new
attributes can spend their own time on this.
>
> ---
> Index: lib/sensors.h
> =================================> --- lib/sensors.h (revision 5851)
> +++ lib/sensors.h (working copy)
> @@ -151,10 +151,14 @@ typedef enum sensors_subfeature_type {
> SENSORS_SUBFEATURE_IN_INPUT = SENSORS_FEATURE_IN << 8,
> SENSORS_SUBFEATURE_IN_MIN,
> SENSORS_SUBFEATURE_IN_MAX,
> + SENSORS_SUBFEATURE_IN_LCRIT,
> + SENSORS_SUBFEATURE_IN_CRIT,
> SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
> SENSORS_SUBFEATURE_IN_MIN_ALARM,
> SENSORS_SUBFEATURE_IN_MAX_ALARM,
> SENSORS_SUBFEATURE_IN_BEEP,
> + SENSORS_SUBFEATURE_IN_LCRIT_ALARM,
> + SENSORS_SUBFEATURE_IN_CRIT_ALARM,
>
> SENSORS_SUBFEATURE_FAN_INPUT = SENSORS_FEATURE_FAN << 8,
> SENSORS_SUBFEATURE_FAN_MIN,
> @@ -169,6 +173,8 @@ typedef enum sensors_subfeature_type {
> SENSORS_SUBFEATURE_TEMP_MIN,
> SENSORS_SUBFEATURE_TEMP_CRIT,
> SENSORS_SUBFEATURE_TEMP_CRIT_HYST,
> + SENSORS_SUBFEATURE_TEMP_LCRIT,
> + SENSORS_SUBFEATURE_TEMP_EMERGENCY,
> SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
> SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
> SENSORS_SUBFEATURE_TEMP_MIN_ALARM,
> @@ -177,6 +183,7 @@ typedef enum sensors_subfeature_type {
> SENSORS_SUBFEATURE_TEMP_TYPE,
> SENSORS_SUBFEATURE_TEMP_OFFSET,
> SENSORS_SUBFEATURE_TEMP_BEEP,
> + SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM,
Seems odd to not have SENSORS_SUBFEATURE_TEMP_EMERGENCY_ALARM.
>
> SENSORS_SUBFEATURE_POWER_AVERAGE = SENSORS_FEATURE_POWER << 8,
> SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST,
> @@ -184,6 +191,11 @@ typedef enum sensors_subfeature_type {
> SENSORS_SUBFEATURE_POWER_INPUT,
> SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST,
> SENSORS_SUBFEATURE_POWER_INPUT_LOWEST,
> + SENSORS_SUBFEATURE_POWER_CAP,
> + SENSORS_SUBFEATURE_POWER_MAX,
> + SENSORS_SUBFEATURE_POWER_CRIT,
> + SENSORS_SUBFEATURE_POWER_ALARM,
> + SENSORS_SUBFEATURE_POWER_CRIT_ALARM,
Alarms are second-class subfeatures, they must go after
SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL. Otherwise they will be
affected by compute statements, with confusing results.
> SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL = (SENSORS_FEATURE_POWER << 8) | 0x80,
>
> SENSORS_SUBFEATURE_ENERGY_INPUT = SENSORS_FEATURE_ENERGY << 8,
> @@ -191,10 +203,14 @@ typedef enum sensors_subfeature_type {
> SENSORS_SUBFEATURE_CURR_INPUT = SENSORS_FEATURE_CURR << 8,
> SENSORS_SUBFEATURE_CURR_MIN,
> SENSORS_SUBFEATURE_CURR_MAX,
> + SENSORS_SUBFEATURE_CURR_LCRIT,
> + SENSORS_SUBFEATURE_CURR_CRIT,
> SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
> SENSORS_SUBFEATURE_CURR_MIN_ALARM,
> SENSORS_SUBFEATURE_CURR_MAX_ALARM,
> SENSORS_SUBFEATURE_CURR_BEEP,
> + SENSORS_SUBFEATURE_CURR_LCRIT_ALARM,
> + SENSORS_SUBFEATURE_CURR_CRIT_ALARM,
>
> SENSORS_SUBFEATURE_VID = SENSORS_FEATURE_VID << 8,
>
> Index: lib/sensors.conf.5
> =================================> --- lib/sensors.conf.5 (revision 5851)
> +++ lib/sensors.conf.5 (working copy)
> @@ -402,7 +402,7 @@ really cool before the fan stops, so that it will
> again immediately.
>
> So, in addition to tempX_max, many chips have a tempX_max_hyst
> -sub-feature. Likewise, tempX_crit often comes with tempX_max_crit.
> +sub-feature. Likewise, tempX_crit often comes with tempX_crit_hyst.
> Example:
This is a bug fix, independent from the other changes, so it should be
committed right now.
>
> .RS
> Index: lib/sysfs.c
> =================================> --- lib/sysfs.c (revision 5851)
> +++ lib/sysfs.c (working copy)
> @@ -219,11 +219,14 @@ static const struct subfeature_type_match temp_mat
> { "max", SENSORS_SUBFEATURE_TEMP_MAX },
> { "max_hyst", SENSORS_SUBFEATURE_TEMP_MAX_HYST },
> { "min", SENSORS_SUBFEATURE_TEMP_MIN },
> + { "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
> { "crit", SENSORS_SUBFEATURE_TEMP_CRIT },
> { "crit_hyst", SENSORS_SUBFEATURE_TEMP_CRIT_HYST },
> + { "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
> { "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
> { "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
> { "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },
> + { "lcrit_alarm", SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM },
> { "crit_alarm", SENSORS_SUBFEATURE_TEMP_CRIT_ALARM },
> { "fault", SENSORS_SUBFEATURE_TEMP_FAULT },
> { "type", SENSORS_SUBFEATURE_TEMP_TYPE },
> @@ -236,9 +239,13 @@ static const struct subfeature_type_match in_match
> { "input", SENSORS_SUBFEATURE_IN_INPUT },
> { "min", SENSORS_SUBFEATURE_IN_MIN },
> { "max", SENSORS_SUBFEATURE_IN_MAX },
> + { "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
> + { "crit", SENSORS_SUBFEATURE_IN_CRIT },
> { "alarm", SENSORS_SUBFEATURE_IN_ALARM },
> { "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
> { "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },
> + { "lcrit_alarm", SENSORS_SUBFEATURE_IN_LCRIT_ALARM },
> + { "crit_alarm", SENSORS_SUBFEATURE_IN_CRIT_ALARM },
> { "beep", SENSORS_SUBFEATURE_IN_BEEP },
> { NULL, 0 }
> };
> @@ -260,6 +267,11 @@ static const struct subfeature_type_match power_ma
> { "input", SENSORS_SUBFEATURE_POWER_INPUT },
> { "input_highest", SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST },
> { "input_lowest", SENSORS_SUBFEATURE_POWER_INPUT_LOWEST },
> + { "cap", SENSORS_SUBFEATURE_POWER_CAP },
> + { "max", SENSORS_SUBFEATURE_POWER_MAX },
> + { "crit", SENSORS_SUBFEATURE_POWER_CRIT },
> + { "alarm", SENSORS_SUBFEATURE_POWER_ALARM },
> + { "crit_alarm", SENSORS_SUBFEATURE_POWER_CRIT_ALARM },
> { "average_interval", SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL },
> { NULL, 0 }
> };
> @@ -273,9 +285,13 @@ static const struct subfeature_type_match curr_mat
> { "input", SENSORS_SUBFEATURE_CURR_INPUT },
> { "min", SENSORS_SUBFEATURE_CURR_MIN },
> { "max", SENSORS_SUBFEATURE_CURR_MAX },
> + { "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
> + { "crit", SENSORS_SUBFEATURE_CURR_CRIT },
> { "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
> { "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
> { "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },
> + { "lcrit_alarm", SENSORS_SUBFEATURE_CURR_LCRIT_ALARM },
> + { "crit_alarm", SENSORS_SUBFEATURE_CURR_CRIT_ALARM },
> { "beep", SENSORS_SUBFEATURE_CURR_BEEP },
> { NULL, 0 }
> };
> Index: prog/sensors/chips.c
> =================================> --- prog/sensors/chips.c (revision 5851)
> +++ prog/sensors/chips.c (working copy)
> @@ -123,37 +123,44 @@ static int get_label_size(const sensors_chip_name
> return max_size + 1;
> }
>
> -static void print_temp_limits(double limit1, double limit2,
> - const char *name1, const char *name2, int alarm)
> +static void print_temp_limits(double limit[], const char *name[],
> + int count, int alarm, int label_size)
> {
> - if (fahrenheit) {
> - limit1 = deg_ctof(limit1);
> - limit2 = deg_ctof(limit2);
> - }
> + int i;
>
> - if (name2) {
> - printf("(%-4s = %+5.1f%s, %-4s = %+5.1f%s) ",
> - name1, limit1, degstr,
> - name2, limit2, degstr);
> - } else if (name1) {
> - printf("(%-4s = %+5.1f%s) ",
> - name1, limit1, degstr);
> - } else {
> - printf(" ");
> + if (fahrenheit)
> + for (i=0; i<count; i++)
> + limit[i] = deg_ctof(limit[i]);
> +
> + for (i=0; i<count; i++) {
> + if (!(i&1)) {
> + if (i)
> + printf("\n%*s", label_size + 10, "");
> + printf("(");
> + } else if (i)
> + printf(", ");
> + printf("%-4s = %+5.1f%s", name[i], limit[i], degstr);
> + if ((i&1) || i = count-1)
> + printf(") ");
> }
>
> - if (alarm)
> - printf("ALARM ");
> + if (alarm & (2|8)) /* min/max (crit) alarm */
> + printf(" %sALARM(MIN%s)", (alarm & 8) ? "CRIT " : "",
> + (alarm &(1|4)) ? ", MAX" : "");
> + else if (alarm & (1|4)) /* max (crit) alarm */
> + printf(" %sALARM ", (alarm & 4) ? "CRIT " : "");
> }
>
> 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;
> + const sensors_subfeature *sf, *sfmin, *sfmax, *sflcrit, *sfcrit,
> + *sfhyst, *sfemerg;
> + double val, limit[7];
> + const char *s[7];
> + int alarm;
> + int count = 0;
> char *label;
>
> if (!(label = sensors_get_label(name, feature))) {
> @@ -172,8 +179,12 @@ static void print_chip_temp(const sensors_chip_nam
> SENSORS_SUBFEATURE_TEMP_MIN);
> sfmax = sensors_get_subfeature(name, feature,
> SENSORS_SUBFEATURE_TEMP_MAX);
> + sflcrit = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_TEMP_LCRIT);
> sfcrit = sensors_get_subfeature(name, feature,
> SENSORS_SUBFEATURE_TEMP_CRIT);
> + sfemerg = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_TEMP_EMERGENCY);
> if (sfmax) {
> sf = sensors_get_subfeature(name, feature,
> SENSORS_SUBFEATURE_TEMP_MAX_ALARM);
> @@ -181,63 +192,56 @@ static void print_chip_temp(const sensors_chip_nam
> alarm |= 1;
>
> if (sfmin) {
> - limit1 = get_value(name, sfmin);
> - s1 = "low";
> - limit2 = get_value(name, sfmax);
> - s2 = "high";
> + limit[count] = get_value(name, sfmin);
> + s[count++] = "low";
>
> 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";
> + alarm |= 2;
> + }
> + limit[count] = get_value(name, sfmax);
> + s[count++] = "high";
>
> - sfhyst = sensors_get_subfeature(name, feature,
> + 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;
> - }
> + if (sfhyst) {
> + limit[count] = get_value(name, sfhyst);
> + s[count++] = "hyst";
> }
> - } else if (sfcrit) {
> - limit1 = get_value(name, sfcrit);
> - s1 = "crit";
> + }
> + if (sflcrit) {
> + limit[count] = get_value(name, sflcrit);
> + s[count++] = "crit low";
> + }
> + if (sfcrit) {
> + limit[count] = get_value(name, sfcrit);
> + s[count++] = sflcrit ? "crit high" : "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;
> + limit[count] = get_value(name, sfhyst);
> + s[count++] = "crit hyst";
> }
> -
> + }
> + if (sfcrit) {
> 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;
> + alarm |= 4;
> }
> + if (sflcrit) {
> + sf = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM);
> + if (sf && get_value(name, sf))
> + alarm |= 8;
> + }
> + if (sfemerg) {
> + limit[count] = get_value(name, sfemerg);
> + s[count++] = "emergency";
> + }
>
> -
> sf = sensors_get_subfeature(name, feature,
> SENSORS_SUBFEATURE_TEMP_FAULT);
> if (sf && get_value(name, sf)) {
> @@ -253,30 +257,9 @@ static void print_chip_temp(const sensors_chip_nam
> } else
> printf(" N/A ");
> }
> - print_temp_limits(limit1, limit2, s1, s2, alarm);
>
> - if (!crit_displayed && sfcrit) {
> - limit1 = get_value(name, sfcrit);
> - s1 = "crit";
> + print_temp_limits(limit, s, count, alarm, label_size);
>
> - 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);
> - alarm = sf && get_value(name, sf);
> -
> - 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);
I confess I did not read the temperature part of your patch, yet. See
why at the end of my reply.
> @@ -303,9 +286,10 @@ static void print_chip_in(const sensors_chip_name
> const sensors_feature *feature,
> int label_size)
> {
> - const sensors_subfeature *sf, *sfmin, *sfmax;
> + const sensors_subfeature *sf, *sfmin, *sfmax, *sflcrit, *sfcrit;
> double val, alarm_max, alarm_min;
> char *label;
> + int crit;
Use tabs for indentation please.
>
> if (!(label = sensors_get_label(name, feature))) {
> fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> @@ -337,13 +321,61 @@ static void print_chip_in(const sensors_chip_name
> printf(" (max = %+6.2f V)",
> get_value(name, sfmax));
>
> + sflcrit = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_IN_LCRIT);
> + sfcrit = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_IN_CRIT);
> + if (sflcrit && sfcrit)
> + printf("\n%*s(crit min = %+6.2f V, crit max = %+6.2f V)",
> + label_size+10, "",
Spaces around "+".
> + get_value(name, sflcrit),
> + get_value(name, sfcrit));
> + else if (sflcrit)
> + printf("\n%*s(crit min = %+6.2f V)",
> + label_size + 10, "",
> + get_value(name, sflcrit));
> + else if (sfcrit)
> + printf("\n%*s(crit max = %+6.2f V)",
> + label_size + 10, "",
> + get_value(name, sfcrit));
> +
> + crit = 0;
Bad indentation.
> + sfmin = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_IN_LCRIT_ALARM);
> + sfmax = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_IN_CRIT_ALARM);
> + if (sfmin && sfmax) {
> + alarm_max = sfmax ? get_value(name, sfmax) : 0;
> + alarm_min = sfmin ? get_value(name, sfmin) : 0;
> +
> + if (alarm_min || alarm_max) {
> + printf(" CRIT ALARM (");
> +
> + if (alarm_min)
> + printf("MIN");
> + if (alarm_max)
> + printf("%sMAX", alarm_min ? ", " : "");
> +
> + printf(")");
> + crit = 1;
Bad indentation.
> + }
> + } else if (sfmax) {
> + alarm_max = get_value(name, sf);
> + if (alarm_max) {
> + printf(" CRIT ALARM");
> + crit = 1;
> + }
> + }
Bad indentation (whole block).
I don't think handling the "crit but not lcrit" case is worth the extra
complexity. Critical voltage limits are pretty rare already. I'd prefer
that you stick to the min/max alarm approach (and then maybe we find a
way to move the common code to a separate helper function.)
> +
> 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) {
> +
> + /* Only display non-critical alarms if there were no critical alarms */
> + if (!crit && (sfmin || sfmax)) {
> alarm_max = sfmax ? get_value(name, sfmax) : 0;
> alarm_min = sfmin ? get_value(name, sfmin) : 0;
>
Looks inconsistent to me. The approach taken for temperatures was to
display the alarm on the same line as the corresponding limit. This
ensured that all alarm flags were displayed. Your approach differs, and
I wouldn't say it's bad, but I want us to be consistent.
> @@ -357,7 +389,7 @@ static void print_chip_in(const sensors_chip_name
>
> printf(")");
> }
> - } else if (sf) {
> + } else if (!crit && sf) {
> printf(" %s",
> get_value(name, sf) ? "ALARM" : "");
> }
> @@ -453,9 +485,10 @@ static void print_chip_power(const sensors_chip_na
> {
> double val;
> int need_space = 0;
> - const sensors_subfeature *sf, *sfmin, *sfmax, *sfint;
> + const sensors_subfeature *sf, *sfmin, *sfmax, *sfint, *sfcrit, *sfcap;
> char *label;
> const char *unit;
> + double crit = 0;
>
> if (!(label = sensors_get_label(name, feature))) {
> fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> @@ -475,7 +508,20 @@ static void print_chip_power(const sensors_chip_na
> SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST);
> sfmax = sensors_get_subfeature(name, feature,
> SENSORS_SUBFEATURE_POWER_INPUT_LOWEST);
> + sfcap = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_POWER_CAP);
> sfint = NULL;
> + sfcrit = NULL;
> + /*
> + * Maybe we have power_max and power_crit instead of highest/lowest
> + */
I don't get why this should be "instead". These are different features
and I fail to see what prevents a given sensor from implementing them
all.
Furthermore, I find it utterly confusing that "sensors" may label two
different power features (maximum historical measurement and high
limit) with the same "max" label. How does the user know what he/she is
seeing?
> + if (!sfmax) {
> + sfmin = NULL;
> + sfmax = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_POWER_MAX);
> + sfcrit = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_POWER_CRIT);
> + }
> } else {
> sf = sensors_get_subfeature(name, feature,
> SENSORS_SUBFEATURE_POWER_AVERAGE);
> @@ -485,6 +531,8 @@ static void print_chip_power(const sensors_chip_na
> SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST);
> sfint = sensors_get_subfeature(name, feature,
> SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL);
> + sfcrit = NULL;
> + sfcap = NULL;
> }
>
> if (sf && get_input_value(name, sf, &val) = 0) {
> @@ -493,7 +541,7 @@ static void print_chip_power(const sensors_chip_na
> } else
> printf(" N/A");
>
> - if (sfmin || sfmax || sfint) {
> + if (sfmin || sfmax || sfcap || sfint || sfcrit) {
> printf(" (");
>
> if (sfmin) {
> @@ -511,6 +559,22 @@ static void print_chip_power(const sensors_chip_na
> need_space = 1;
> }
>
> + if (sfcap) {
> + val = get_value(name, sfcap);
> + scale_value(&val, &unit);
> + printf("%scap = %6.2f %sW", (need_space ? ", " : ""),
> + val, unit);
> + need_space = 1;
> + }
> +
> + if (sfcrit) {
> + val = get_value(name, sfcrit);
> + scale_value(&val, &unit);
> + printf("%scrit = %6.2f %sW", (need_space ? ", " : ""),
> + val, unit);
> + need_space = 1;
> + }
> +
> if (sfint) {
> printf("%sinterval = %6.2f s", (need_space ? ", " : ""),
> get_value(name, sfint));
> @@ -519,6 +583,20 @@ static void print_chip_power(const sensors_chip_na
> printf(")");
> }
>
> + sf = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_POWER_CRIT_ALARM);
> + if (sf) {
> + crit = get_value(name, sf);
> + if (crit)
> + printf(" %s", "CRIT ALARM");
> + }
> + if (!crit) {
> + sf = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_POWER_ALARM);
> + if (sf)
> + printf(" %s", get_value(name, sf) ? "ALARM" : "");
> + }
> +
> printf("\n");
> }
>
> @@ -596,7 +674,7 @@ static void print_chip_curr(const sensors_chip_nam
> const sensors_feature *feature,
> int label_size)
> {
> - const sensors_subfeature *sf, *sfmin, *sfmax;
> + const sensors_subfeature *sf, *sfmin, *sfmax, *sfcrit, *sflcrit;
> double alarm_max, alarm_min, val;
> char *label;
>
> @@ -630,18 +708,35 @@ static void print_chip_curr(const sensors_chip_nam
> printf(" (max = %+6.2f A)",
> get_value(name, sfmax));
>
> - sf = sensors_get_subfeature(name, feature,
> - SENSORS_SUBFEATURE_CURR_ALARM);
> + sflcrit = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_CURR_LCRIT);
> + sfcrit = sensors_get_subfeature(name, feature,
> + SENSORS_SUBFEATURE_CURR_CRIT);
> + if (sflcrit && sfcrit)
> + printf("\n%*s(crit min = %+6.2f A, crit max = %+6.2f A)",
> + label_size+10, "",
Spaces around "+" please.
> + get_value(name, sflcrit),
> + get_value(name, sfcrit));
> + else if (sflcrit)
> + printf("\n%*s(crit min = %+6.2f A)",
> + label_size + 10, "",
> + get_value(name, sflcrit));
> + else if (sfcrit)
> + printf("\n%*s(crit max = %+6.2f A)",
> + label_size + 10, "",
> + get_value(name, sfcrit));
> +
> + alarm_min = alarm_max = 0;
> sfmin = sensors_get_subfeature(name, feature,
> - SENSORS_SUBFEATURE_CURR_MIN_ALARM);
> + SENSORS_SUBFEATURE_CURR_LCRIT_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;
> + SENSORS_SUBFEATURE_CURR_CRIT_ALARM);
> + if (sfmin && sfmax) {
> + alarm_max = get_value(name, sfmax);
> + alarm_min = get_value(name, sfmin);
>
> if (alarm_min || alarm_max) {
> - printf(" ALARM (");
> + printf(" CRIT ALARM (");
>
> if (alarm_min)
> printf("MIN");
> @@ -650,11 +745,40 @@ static void print_chip_curr(const sensors_chip_nam
>
> printf(")");
> }
> - } else if (sf) {
> - printf(" %s",
> - get_value(name, sf) ? "ALARM" : "");
> + } else if (sfmax) {
> + alarm_max = get_value(name, sfmax);
> + if (alarm_max)
> + printf(" CRIT ALARM");
> }
>
> + /* Only display non-critical alarms if there were no critical alarms */
> + if (!alarm_min && !alarm_max) {
> + 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;
> +
> + 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");
> }
>
I think we have reached a point of code complexity where it's better to
just step back and rethink the whole thing. I had a hard time reading
your patch - even skipped the temperature part completely due to lack
of time - and I am the one supposed to maintain this piece of code (and
I wrote a large part of it.) This tells a lot.
I think we want to first define clearly how we want to display the
various information. In the case of alarms, we also want to decide what
we want to display, as apparently we don't agree on this. Both
decisions should be common to all sensor types, for consistency. And we
want to document it all at the top of the source file, as a reference
for both of us and for future contributors too.
Then I think we need more helper functions. This is the only way to
ensure that the output is consistent. There are too many special cases
in the code right now. I'm not saying we can do entirely without these,
but they should be the exception, not the rule.
I am fully aware that I am asking for a lot of changes, and maybe you
don't have the time and interest in doing this. But this seems the best
approach to me in the long run.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon
2010-07-08 5:43 [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon Guenter Roeck
2010-12-09 15:06 ` Jean Delvare
@ 2010-12-09 15:40 ` Guenter Roeck
2010-12-09 20:31 ` Jean Delvare
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2010-12-09 15:40 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On Thu, Dec 09, 2010 at 10:06:26AM -0500, Jean Delvare wrote:
> Hi Guenter,
>
> Sorry for the very late reply <sigh>.
>
No problem ...
> On Wed, 7 Jul 2010 22:43:33 -0700, Guenter Roeck wrote:
> > This patch adds support for the following new attributes to libsensors and
> > to the sensors command.
> >
> > inX_lcrit
> > inX_crit
> > inX_lcrit_alarm
> > inX_crit_alarm
> > temp_lcrit
> > temp_emergency
> > temp_lcrit_alarm
> > powerX_cap
> > powerX_max
> > powerX_crit
> > powerX_alarm
> > powerX_crit_alarm
> > currX_lcrit
> > currX_crit
> > currX_lcrit_alarm
> > currX_crit_alarm
>
> Many of these (inX_lcrit_alarm, inX_crit_alarm, powerX_max,
> powerX_crit, powerX_crit_alarm, currX_lcrit, currX_crit,
> currX_lcrit_alarm, currX_crit_alarm) aren't in
> Documentation/hwmon/sysfs-interface, so we can't add support in
> libsensors yet.
>
> OTOH some attributes that have been added to
> Documentation/hwmon/sysfs-interface aren't present in your patch:
> temp[1-*]_emergency_hyst, temp[1-*]_emergency_alarm,
> power[1-*]_cap_hyst. Maybe you want to add them.
>
> It is very important that libsensors sticks to what is documented in
> file sysfs-interface, otherwise drivers will implement attributes
> libsensors will ignore, and vice versa. So please add the missing
> attributes to Documentation/hwmon/sysfs-interface, and then adjust the
> libsensors patch to be in line with this.
>
> It is entirely possible that you submitted such changes in the past and
> I overlooked them, in which case I apologize. Please resend. I'm doing
> what I can in the little spare time I have...
>
> > I reworked print_temp_limits() significantly, which asks for a detailed review.
> > Idea is to make the function better scalable for a large number of attributes
> > while retaining at least some compatibility with legacy output.
>
> This is tricky, and I admit I don't know what to do here. The balance
> between nice output and nice code is hard to find. FWIW, I don't mind a
> minor "regression" in the output if this makes the code much simpler.
>
> BTW, feel free to split the patch into libsensors changes and sensors
> changes. The libsensors part should be relatively straightforward to
> get right and merge, so my limited review time doesn't block it. The
> sensors part is a totally different beast.
>
> I suspect you'll have to rebase your patch anyway, as this version is
> getting old and there were quite a few commits to both libsensors and
> sensors meanwhile.
>
Actually, I think I'll have to re-do the whole patch anyway. As you pointed out,
it does not reflect the documented attributes, and there are other problems
such as how and when to display alarms. Without going into details,
I don't think we really disagree on anything - the idea was to retain
the existing output. I realized after I submitted this patch that I did not
always accomplish this goal.
> > sensord does not yet support the new attributes. The sensord code is a bit more
> > complex than sensors, and I don't have a clean solution for it yet.
>
> You really don't have to care about sensord if you do not have any use
> for it. This is just one libsensors-based application amongst many
> others, it should even live outside of the main lm-sensors package
> IMHO. Anyone interested in adding support for (some of) the new
> attributes can spend their own time on this.
>
Ok.
> >
> > ---
> > Index: lib/sensors.h
> > =================================> > --- lib/sensors.h (revision 5851)
> > +++ lib/sensors.h (working copy)
> > @@ -151,10 +151,14 @@ typedef enum sensors_subfeature_type {
> > SENSORS_SUBFEATURE_IN_INPUT = SENSORS_FEATURE_IN << 8,
> > SENSORS_SUBFEATURE_IN_MIN,
> > SENSORS_SUBFEATURE_IN_MAX,
> > + SENSORS_SUBFEATURE_IN_LCRIT,
> > + SENSORS_SUBFEATURE_IN_CRIT,
> > SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
> > SENSORS_SUBFEATURE_IN_MIN_ALARM,
> > SENSORS_SUBFEATURE_IN_MAX_ALARM,
> > SENSORS_SUBFEATURE_IN_BEEP,
> > + SENSORS_SUBFEATURE_IN_LCRIT_ALARM,
> > + SENSORS_SUBFEATURE_IN_CRIT_ALARM,
> >
> > SENSORS_SUBFEATURE_FAN_INPUT = SENSORS_FEATURE_FAN << 8,
> > SENSORS_SUBFEATURE_FAN_MIN,
> > @@ -169,6 +173,8 @@ typedef enum sensors_subfeature_type {
> > SENSORS_SUBFEATURE_TEMP_MIN,
> > SENSORS_SUBFEATURE_TEMP_CRIT,
> > SENSORS_SUBFEATURE_TEMP_CRIT_HYST,
> > + SENSORS_SUBFEATURE_TEMP_LCRIT,
> > + SENSORS_SUBFEATURE_TEMP_EMERGENCY,
> > SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
> > SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
> > SENSORS_SUBFEATURE_TEMP_MIN_ALARM,
> > @@ -177,6 +183,7 @@ typedef enum sensors_subfeature_type {
> > SENSORS_SUBFEATURE_TEMP_TYPE,
> > SENSORS_SUBFEATURE_TEMP_OFFSET,
> > SENSORS_SUBFEATURE_TEMP_BEEP,
> > + SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM,
>
> Seems odd to not have SENSORS_SUBFEATURE_TEMP_EMERGENCY_ALARM.
>
> >
> > SENSORS_SUBFEATURE_POWER_AVERAGE = SENSORS_FEATURE_POWER << 8,
> > SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST,
> > @@ -184,6 +191,11 @@ typedef enum sensors_subfeature_type {
> > SENSORS_SUBFEATURE_POWER_INPUT,
> > SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST,
> > SENSORS_SUBFEATURE_POWER_INPUT_LOWEST,
> > + SENSORS_SUBFEATURE_POWER_CAP,
> > + SENSORS_SUBFEATURE_POWER_MAX,
> > + SENSORS_SUBFEATURE_POWER_CRIT,
> > + SENSORS_SUBFEATURE_POWER_ALARM,
> > + SENSORS_SUBFEATURE_POWER_CRIT_ALARM,
>
> Alarms are second-class subfeatures, they must go after
> SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL. Otherwise they will be
> affected by compute statements, with confusing results.
>
> > SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL = (SENSORS_FEATURE_POWER << 8) | 0x80,
> >
> > SENSORS_SUBFEATURE_ENERGY_INPUT = SENSORS_FEATURE_ENERGY << 8,
> > @@ -191,10 +203,14 @@ typedef enum sensors_subfeature_type {
> > SENSORS_SUBFEATURE_CURR_INPUT = SENSORS_FEATURE_CURR << 8,
> > SENSORS_SUBFEATURE_CURR_MIN,
> > SENSORS_SUBFEATURE_CURR_MAX,
> > + SENSORS_SUBFEATURE_CURR_LCRIT,
> > + SENSORS_SUBFEATURE_CURR_CRIT,
> > SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
> > SENSORS_SUBFEATURE_CURR_MIN_ALARM,
> > SENSORS_SUBFEATURE_CURR_MAX_ALARM,
> > SENSORS_SUBFEATURE_CURR_BEEP,
> > + SENSORS_SUBFEATURE_CURR_LCRIT_ALARM,
> > + SENSORS_SUBFEATURE_CURR_CRIT_ALARM,
> >
> > SENSORS_SUBFEATURE_VID = SENSORS_FEATURE_VID << 8,
> >
> > Index: lib/sensors.conf.5
> > =================================> > --- lib/sensors.conf.5 (revision 5851)
> > +++ lib/sensors.conf.5 (working copy)
> > @@ -402,7 +402,7 @@ really cool before the fan stops, so that it will
> > again immediately.
> >
> > So, in addition to tempX_max, many chips have a tempX_max_hyst
> > -sub-feature. Likewise, tempX_crit often comes with tempX_max_crit.
> > +sub-feature. Likewise, tempX_crit often comes with tempX_crit_hyst.
> > Example:
>
> This is a bug fix, independent from the other changes, so it should be
> committed right now.
>
> >
> > .RS
> > Index: lib/sysfs.c
> > =================================> > --- lib/sysfs.c (revision 5851)
> > +++ lib/sysfs.c (working copy)
> > @@ -219,11 +219,14 @@ static const struct subfeature_type_match temp_mat
> > { "max", SENSORS_SUBFEATURE_TEMP_MAX },
> > { "max_hyst", SENSORS_SUBFEATURE_TEMP_MAX_HYST },
> > { "min", SENSORS_SUBFEATURE_TEMP_MIN },
> > + { "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
> > { "crit", SENSORS_SUBFEATURE_TEMP_CRIT },
> > { "crit_hyst", SENSORS_SUBFEATURE_TEMP_CRIT_HYST },
> > + { "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
> > { "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },
> > + { "lcrit_alarm", SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM },
> > { "crit_alarm", SENSORS_SUBFEATURE_TEMP_CRIT_ALARM },
> > { "fault", SENSORS_SUBFEATURE_TEMP_FAULT },
> > { "type", SENSORS_SUBFEATURE_TEMP_TYPE },
> > @@ -236,9 +239,13 @@ static const struct subfeature_type_match in_match
> > { "input", SENSORS_SUBFEATURE_IN_INPUT },
> > { "min", SENSORS_SUBFEATURE_IN_MIN },
> > { "max", SENSORS_SUBFEATURE_IN_MAX },
> > + { "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
> > + { "crit", SENSORS_SUBFEATURE_IN_CRIT },
> > { "alarm", SENSORS_SUBFEATURE_IN_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },
> > + { "lcrit_alarm", SENSORS_SUBFEATURE_IN_LCRIT_ALARM },
> > + { "crit_alarm", SENSORS_SUBFEATURE_IN_CRIT_ALARM },
> > { "beep", SENSORS_SUBFEATURE_IN_BEEP },
> > { NULL, 0 }
> > };
> > @@ -260,6 +267,11 @@ static const struct subfeature_type_match power_ma
> > { "input", SENSORS_SUBFEATURE_POWER_INPUT },
> > { "input_highest", SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST },
> > { "input_lowest", SENSORS_SUBFEATURE_POWER_INPUT_LOWEST },
> > + { "cap", SENSORS_SUBFEATURE_POWER_CAP },
> > + { "max", SENSORS_SUBFEATURE_POWER_MAX },
> > + { "crit", SENSORS_SUBFEATURE_POWER_CRIT },
> > + { "alarm", SENSORS_SUBFEATURE_POWER_ALARM },
> > + { "crit_alarm", SENSORS_SUBFEATURE_POWER_CRIT_ALARM },
> > { "average_interval", SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL },
> > { NULL, 0 }
> > };
> > @@ -273,9 +285,13 @@ static const struct subfeature_type_match curr_mat
> > { "input", SENSORS_SUBFEATURE_CURR_INPUT },
> > { "min", SENSORS_SUBFEATURE_CURR_MIN },
> > { "max", SENSORS_SUBFEATURE_CURR_MAX },
> > + { "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
> > + { "crit", SENSORS_SUBFEATURE_CURR_CRIT },
> > { "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },
> > + { "lcrit_alarm", SENSORS_SUBFEATURE_CURR_LCRIT_ALARM },
> > + { "crit_alarm", SENSORS_SUBFEATURE_CURR_CRIT_ALARM },
> > { "beep", SENSORS_SUBFEATURE_CURR_BEEP },
> > { NULL, 0 }
> > };
> > Index: prog/sensors/chips.c
> > =================================> > --- prog/sensors/chips.c (revision 5851)
> > +++ prog/sensors/chips.c (working copy)
> > @@ -123,37 +123,44 @@ static int get_label_size(const sensors_chip_name
> > return max_size + 1;
> > }
> >
> > -static void print_temp_limits(double limit1, double limit2,
> > - const char *name1, const char *name2, int alarm)
> > +static void print_temp_limits(double limit[], const char *name[],
> > + int count, int alarm, int label_size)
> > {
> > - if (fahrenheit) {
> > - limit1 = deg_ctof(limit1);
> > - limit2 = deg_ctof(limit2);
> > - }
> > + int i;
> >
> > - if (name2) {
> > - printf("(%-4s = %+5.1f%s, %-4s = %+5.1f%s) ",
> > - name1, limit1, degstr,
> > - name2, limit2, degstr);
> > - } else if (name1) {
> > - printf("(%-4s = %+5.1f%s) ",
> > - name1, limit1, degstr);
> > - } else {
> > - printf(" ");
> > + if (fahrenheit)
> > + for (i=0; i<count; i++)
> > + limit[i] = deg_ctof(limit[i]);
> > +
> > + for (i=0; i<count; i++) {
> > + if (!(i&1)) {
> > + if (i)
> > + printf("\n%*s", label_size + 10, "");
> > + printf("(");
> > + } else if (i)
> > + printf(", ");
> > + printf("%-4s = %+5.1f%s", name[i], limit[i], degstr);
> > + if ((i&1) || i = count-1)
> > + printf(") ");
> > }
> >
> > - if (alarm)
> > - printf("ALARM ");
> > + if (alarm & (2|8)) /* min/max (crit) alarm */
> > + printf(" %sALARM(MIN%s)", (alarm & 8) ? "CRIT " : "",
> > + (alarm &(1|4)) ? ", MAX" : "");
> > + else if (alarm & (1|4)) /* max (crit) alarm */
> > + printf(" %sALARM ", (alarm & 4) ? "CRIT " : "");
> > }
> >
> > 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;
> > + const sensors_subfeature *sf, *sfmin, *sfmax, *sflcrit, *sfcrit,
> > + *sfhyst, *sfemerg;
> > + double val, limit[7];
> > + const char *s[7];
> > + int alarm;
> > + int count = 0;
> > char *label;
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > @@ -172,8 +179,12 @@ static void print_chip_temp(const sensors_chip_nam
> > SENSORS_SUBFEATURE_TEMP_MIN);
> > sfmax = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_TEMP_MAX);
> > + sflcrit = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_TEMP_LCRIT);
> > sfcrit = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_TEMP_CRIT);
> > + sfemerg = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_TEMP_EMERGENCY);
> > if (sfmax) {
> > sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_TEMP_MAX_ALARM);
> > @@ -181,63 +192,56 @@ static void print_chip_temp(const sensors_chip_nam
> > alarm |= 1;
> >
> > if (sfmin) {
> > - limit1 = get_value(name, sfmin);
> > - s1 = "low";
> > - limit2 = get_value(name, sfmax);
> > - s2 = "high";
> > + limit[count] = get_value(name, sfmin);
> > + s[count++] = "low";
> >
> > 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";
> > + alarm |= 2;
> > + }
> > + limit[count] = get_value(name, sfmax);
> > + s[count++] = "high";
> >
> > - sfhyst = sensors_get_subfeature(name, feature,
> > + 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;
> > - }
> > + if (sfhyst) {
> > + limit[count] = get_value(name, sfhyst);
> > + s[count++] = "hyst";
> > }
> > - } else if (sfcrit) {
> > - limit1 = get_value(name, sfcrit);
> > - s1 = "crit";
> > + }
> > + if (sflcrit) {
> > + limit[count] = get_value(name, sflcrit);
> > + s[count++] = "crit low";
> > + }
> > + if (sfcrit) {
> > + limit[count] = get_value(name, sfcrit);
> > + s[count++] = sflcrit ? "crit high" : "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;
> > + limit[count] = get_value(name, sfhyst);
> > + s[count++] = "crit hyst";
> > }
> > -
> > + }
> > + if (sfcrit) {
> > 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;
> > + alarm |= 4;
> > }
> > + if (sflcrit) {
> > + sf = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM);
> > + if (sf && get_value(name, sf))
> > + alarm |= 8;
> > + }
> > + if (sfemerg) {
> > + limit[count] = get_value(name, sfemerg);
> > + s[count++] = "emergency";
> > + }
> >
> > -
> > sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_TEMP_FAULT);
> > if (sf && get_value(name, sf)) {
> > @@ -253,30 +257,9 @@ static void print_chip_temp(const sensors_chip_nam
> > } else
> > printf(" N/A ");
> > }
> > - print_temp_limits(limit1, limit2, s1, s2, alarm);
> >
> > - if (!crit_displayed && sfcrit) {
> > - limit1 = get_value(name, sfcrit);
> > - s1 = "crit";
> > + print_temp_limits(limit, s, count, alarm, label_size);
> >
> > - 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);
> > - alarm = sf && get_value(name, sf);
> > -
> > - 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);
>
> I confess I did not read the temperature part of your patch, yet. See
> why at the end of my reply.
>
> > @@ -303,9 +286,10 @@ static void print_chip_in(const sensors_chip_name
> > const sensors_feature *feature,
> > int label_size)
> > {
> > - const sensors_subfeature *sf, *sfmin, *sfmax;
> > + const sensors_subfeature *sf, *sfmin, *sfmax, *sflcrit, *sfcrit;
> > double val, alarm_max, alarm_min;
> > char *label;
> > + int crit;
>
> Use tabs for indentation please.
>
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> > @@ -337,13 +321,61 @@ static void print_chip_in(const sensors_chip_name
> > printf(" (max = %+6.2f V)",
> > get_value(name, sfmax));
> >
> > + sflcrit = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_IN_LCRIT);
> > + sfcrit = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_IN_CRIT);
> > + if (sflcrit && sfcrit)
> > + printf("\n%*s(crit min = %+6.2f V, crit max = %+6.2f V)",
> > + label_size+10, "",
>
> Spaces around "+".
>
> > + get_value(name, sflcrit),
> > + get_value(name, sfcrit));
> > + else if (sflcrit)
> > + printf("\n%*s(crit min = %+6.2f V)",
> > + label_size + 10, "",
> > + get_value(name, sflcrit));
> > + else if (sfcrit)
> > + printf("\n%*s(crit max = %+6.2f V)",
> > + label_size + 10, "",
> > + get_value(name, sfcrit));
> > +
> > + crit = 0;
>
> Bad indentation.
>
> > + sfmin = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_IN_LCRIT_ALARM);
> > + sfmax = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_IN_CRIT_ALARM);
> > + if (sfmin && sfmax) {
> > + alarm_max = sfmax ? get_value(name, sfmax) : 0;
> > + alarm_min = sfmin ? get_value(name, sfmin) : 0;
> > +
> > + if (alarm_min || alarm_max) {
> > + printf(" CRIT ALARM (");
> > +
> > + if (alarm_min)
> > + printf("MIN");
> > + if (alarm_max)
> > + printf("%sMAX", alarm_min ? ", " : "");
> > +
> > + printf(")");
> > + crit = 1;
>
> Bad indentation.
>
> > + }
> > + } else if (sfmax) {
> > + alarm_max = get_value(name, sf);
> > + if (alarm_max) {
> > + printf(" CRIT ALARM");
> > + crit = 1;
> > + }
> > + }
>
> Bad indentation (whole block).
>
> I don't think handling the "crit but not lcrit" case is worth the extra
> complexity. Critical voltage limits are pretty rare already. I'd prefer
> that you stick to the min/max alarm approach (and then maybe we find a
> way to move the common code to a separate helper function.)
>
Ok, I'll think about it.
> > +
> > 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) {
> > +
> > + /* Only display non-critical alarms if there were no critical alarms */
> > + if (!crit && (sfmin || sfmax)) {
> > alarm_max = sfmax ? get_value(name, sfmax) : 0;
> > alarm_min = sfmin ? get_value(name, sfmin) : 0;
> >
>
> Looks inconsistent to me. The approach taken for temperatures was to
> display the alarm on the same line as the corresponding limit. This
> ensured that all alarm flags were displayed. Your approach differs, and
> I wouldn't say it's bad, but I want us to be consistent.
>
Not intentionally different.
> > @@ -357,7 +389,7 @@ static void print_chip_in(const sensors_chip_name
> >
> > printf(")");
> > }
> > - } else if (sf) {
> > + } else if (!crit && sf) {
> > printf(" %s",
> > get_value(name, sf) ? "ALARM" : "");
> > }
> > @@ -453,9 +485,10 @@ static void print_chip_power(const sensors_chip_na
> > {
> > double val;
> > int need_space = 0;
> > - const sensors_subfeature *sf, *sfmin, *sfmax, *sfint;
> > + const sensors_subfeature *sf, *sfmin, *sfmax, *sfint, *sfcrit, *sfcap;
> > char *label;
> > const char *unit;
> > + double crit = 0;
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> > @@ -475,7 +508,20 @@ static void print_chip_power(const sensors_chip_na
> > SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST);
> > sfmax = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_POWER_INPUT_LOWEST);
> > + sfcap = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_POWER_CAP);
> > sfint = NULL;
> > + sfcrit = NULL;
> > + /*
> > + * Maybe we have power_max and power_crit instead of highest/lowest
> > + */
>
> I don't get why this should be "instead". These are different features
> and I fail to see what prevents a given sensor from implementing them
> all.
>
> Furthermore, I find it utterly confusing that "sensors" may label two
> different power features (maximum historical measurement and high
> limit) with the same "max" label. How does the user know what he/she is
> seeing?
>
Good point.
> > + if (!sfmax) {
> > + sfmin = NULL;
> > + sfmax = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_POWER_MAX);
> > + sfcrit = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_POWER_CRIT);
> > + }
> > } else {
> > sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_POWER_AVERAGE);
> > @@ -485,6 +531,8 @@ static void print_chip_power(const sensors_chip_na
> > SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST);
> > sfint = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL);
> > + sfcrit = NULL;
> > + sfcap = NULL;
> > }
> >
> > if (sf && get_input_value(name, sf, &val) = 0) {
> > @@ -493,7 +541,7 @@ static void print_chip_power(const sensors_chip_na
> > } else
> > printf(" N/A");
> >
> > - if (sfmin || sfmax || sfint) {
> > + if (sfmin || sfmax || sfcap || sfint || sfcrit) {
> > printf(" (");
> >
> > if (sfmin) {
> > @@ -511,6 +559,22 @@ static void print_chip_power(const sensors_chip_na
> > need_space = 1;
> > }
> >
> > + if (sfcap) {
> > + val = get_value(name, sfcap);
> > + scale_value(&val, &unit);
> > + printf("%scap = %6.2f %sW", (need_space ? ", " : ""),
> > + val, unit);
> > + need_space = 1;
> > + }
> > +
> > + if (sfcrit) {
> > + val = get_value(name, sfcrit);
> > + scale_value(&val, &unit);
> > + printf("%scrit = %6.2f %sW", (need_space ? ", " : ""),
> > + val, unit);
> > + need_space = 1;
> > + }
> > +
> > if (sfint) {
> > printf("%sinterval = %6.2f s", (need_space ? ", " : ""),
> > get_value(name, sfint));
> > @@ -519,6 +583,20 @@ static void print_chip_power(const sensors_chip_na
> > printf(")");
> > }
> >
> > + sf = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_POWER_CRIT_ALARM);
> > + if (sf) {
> > + crit = get_value(name, sf);
> > + if (crit)
> > + printf(" %s", "CRIT ALARM");
> > + }
> > + if (!crit) {
> > + sf = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_POWER_ALARM);
> > + if (sf)
> > + printf(" %s", get_value(name, sf) ? "ALARM" : "");
> > + }
> > +
> > printf("\n");
> > }
> >
> > @@ -596,7 +674,7 @@ static void print_chip_curr(const sensors_chip_nam
> > const sensors_feature *feature,
> > int label_size)
> > {
> > - const sensors_subfeature *sf, *sfmin, *sfmax;
> > + const sensors_subfeature *sf, *sfmin, *sfmax, *sfcrit, *sflcrit;
> > double alarm_max, alarm_min, val;
> > char *label;
> >
> > @@ -630,18 +708,35 @@ static void print_chip_curr(const sensors_chip_nam
> > printf(" (max = %+6.2f A)",
> > get_value(name, sfmax));
> >
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_CURR_ALARM);
> > + sflcrit = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_CURR_LCRIT);
> > + sfcrit = sensors_get_subfeature(name, feature,
> > + SENSORS_SUBFEATURE_CURR_CRIT);
> > + if (sflcrit && sfcrit)
> > + printf("\n%*s(crit min = %+6.2f A, crit max = %+6.2f A)",
> > + label_size+10, "",
>
> Spaces around "+" please.
>
> > + get_value(name, sflcrit),
> > + get_value(name, sfcrit));
> > + else if (sflcrit)
> > + printf("\n%*s(crit min = %+6.2f A)",
> > + label_size + 10, "",
> > + get_value(name, sflcrit));
> > + else if (sfcrit)
> > + printf("\n%*s(crit max = %+6.2f A)",
> > + label_size + 10, "",
> > + get_value(name, sfcrit));
> > +
> > + alarm_min = alarm_max = 0;
> > sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_CURR_MIN_ALARM);
> > + SENSORS_SUBFEATURE_CURR_LCRIT_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;
> > + SENSORS_SUBFEATURE_CURR_CRIT_ALARM);
> > + if (sfmin && sfmax) {
> > + alarm_max = get_value(name, sfmax);
> > + alarm_min = get_value(name, sfmin);
> >
> > if (alarm_min || alarm_max) {
> > - printf(" ALARM (");
> > + printf(" CRIT ALARM (");
> >
> > if (alarm_min)
> > printf("MIN");
> > @@ -650,11 +745,40 @@ static void print_chip_curr(const sensors_chip_nam
> >
> > printf(")");
> > }
> > - } else if (sf) {
> > - printf(" %s",
> > - get_value(name, sf) ? "ALARM" : "");
> > + } else if (sfmax) {
> > + alarm_max = get_value(name, sfmax);
> > + if (alarm_max)
> > + printf(" CRIT ALARM");
> > }
> >
> > + /* Only display non-critical alarms if there were no critical alarms */
> > + if (!alarm_min && !alarm_max) {
> > + 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;
> > +
> > + 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");
> > }
> >
>
> I think we have reached a point of code complexity where it's better to
> just step back and rethink the whole thing. I had a hard time reading
> your patch - even skipped the temperature part completely due to lack
> of time - and I am the one supposed to maintain this piece of code (and
> I wrote a large part of it.) This tells a lot.
>
> I think we want to first define clearly how we want to display the
> various information. In the case of alarms, we also want to decide what
> we want to display, as apparently we don't agree on this. Both
Only unintentionally. The idea was to keep the output the same and only
add to it. Obviously I got this wrong.
> decisions should be common to all sensor types, for consistency. And we
> want to document it all at the top of the source file, as a reference
> for both of us and for future contributors too.
>
Makes sense.
> Then I think we need more helper functions. This is the only way to
> ensure that the output is consistent. There are too many special cases
> in the code right now. I'm not saying we can do entirely without these,
> but they should be the exception, not the rule.
>
Agreed. I'll have to think about it.
> I am fully aware that I am asking for a lot of changes, and maybe you
> don't have the time and interest in doing this. But this seems the best
> approach to me in the long run.
>
Interest yes, time may be another problem ;).
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon
2010-07-08 5:43 [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon Guenter Roeck
2010-12-09 15:06 ` Jean Delvare
2010-12-09 15:40 ` Guenter Roeck
@ 2010-12-09 20:31 ` Jean Delvare
2010-12-09 20:45 ` Guenter Roeck
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2010-12-09 20:31 UTC (permalink / raw)
To: lm-sensors
On Thu, 9 Dec 2010 07:40:46 -0800, Guenter Roeck wrote:
> Actually, I think I'll have to re-do the whole patch anyway. As you pointed out,
> it does not reflect the documented attributes, and there are other problems
> such as how and when to display alarms. Without going into details,
> I don't think we really disagree on anything
I agree.
> - the idea was to retain the existing output.
Up to a certain point, yes. Where the current output was decided
because it made sense, we want to keep it as is. But being identical to
the character isn't a goal per se. In particular, if switching to more
generic code is possible and makes the code smaller and/or more
readable and/or more maintainable, we definitely want to consider that,
even if this changes the output slightly for some chips.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon
2010-07-08 5:43 [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon Guenter Roeck
` (2 preceding siblings ...)
2010-12-09 20:31 ` Jean Delvare
@ 2010-12-09 20:45 ` Guenter Roeck
2010-12-10 17:37 ` Guenter Roeck
2010-12-10 17:48 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2010-12-09 20:45 UTC (permalink / raw)
To: lm-sensors
On Thu, 2010-12-09 at 15:31 -0500, Jean Delvare wrote:
> On Thu, 9 Dec 2010 07:40:46 -0800, Guenter Roeck wrote:
> > Actually, I think I'll have to re-do the whole patch anyway. As you pointed out,
> > it does not reflect the documented attributes, and there are other problems
> > such as how and when to display alarms. Without going into details,
> > I don't think we really disagree on anything
>
> I agree.
>
> > - the idea was to retain the existing output.
>
> Up to a certain point, yes. Where the current output was decided
> because it made sense, we want to keep it as is. But being identical to
> the character isn't a goal per se. In particular, if switching to more
> generic code is possible and makes the code smaller and/or more
> readable and/or more maintainable, we definitely want to consider that,
> even if this changes the output slightly for some chips.
>
Ok, makes sense.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon
2010-07-08 5:43 [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon Guenter Roeck
` (3 preceding siblings ...)
2010-12-09 20:45 ` Guenter Roeck
@ 2010-12-10 17:37 ` Guenter Roeck
2010-12-10 17:48 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2010-12-10 17:37 UTC (permalink / raw)
To: lm-sensors
On Thu, Dec 09, 2010 at 03:31:30PM -0500, Jean Delvare wrote:
> On Thu, 9 Dec 2010 07:40:46 -0800, Guenter Roeck wrote:
> > Actually, I think I'll have to re-do the whole patch anyway. As you pointed out,
> > it does not reflect the documented attributes, and there are other problems
> > such as how and when to display alarms. Without going into details,
> > I don't think we really disagree on anything
>
> I agree.
>
> > - the idea was to retain the existing output.
>
> Up to a certain point, yes. Where the current output was decided
> because it made sense, we want to keep it as is. But being identical to
> the character isn't a goal per se. In particular, if switching to more
> generic code is possible and makes the code smaller and/or more
> readable and/or more maintainable, we definitely want to consider that,
> even if this changes the output slightly for some chips.
>
Related question: When making libsensors API changes, do I update the API version
to 4.3.2, or stick with 4.3.1 ?
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon
2010-07-08 5:43 [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon Guenter Roeck
` (4 preceding siblings ...)
2010-12-10 17:37 ` Guenter Roeck
@ 2010-12-10 17:48 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2010-12-10 17:48 UTC (permalink / raw)
To: lm-sensors
On Fri, 10 Dec 2010 09:37:14 -0800, Guenter Roeck wrote:
> On Thu, Dec 09, 2010 at 03:31:30PM -0500, Jean Delvare wrote:
> > On Thu, 9 Dec 2010 07:40:46 -0800, Guenter Roeck wrote:
> > > Actually, I think I'll have to re-do the whole patch anyway. As you pointed out,
> > > it does not reflect the documented attributes, and there are other problems
> > > such as how and when to display alarms. Without going into details,
> > > I don't think we really disagree on anything
> >
> > I agree.
> >
> > > - the idea was to retain the existing output.
> >
> > Up to a certain point, yes. Where the current output was decided
> > because it made sense, we want to keep it as is. But being identical to
> > the character isn't a goal per se. In particular, if switching to more
> > generic code is possible and makes the code smaller and/or more
> > readable and/or more maintainable, we definitely want to consider that,
> > even if this changes the output slightly for some chips.
> >
> Related question: When making libsensors API changes, do I update the API version
> to 4.3.2, or stick with 4.3.1 ?
You stick with 4.3.1, as no version of libsensors was ever released
with this number. See doc/libsensors-API.txt.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-12-10 17:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-08 5:43 [lm-sensors] [PATCH v2] lm-sensors: Add support for new hwmon Guenter Roeck
2010-12-09 15:06 ` Jean Delvare
2010-12-09 15:40 ` Guenter Roeck
2010-12-09 20:31 ` Jean Delvare
2010-12-09 20:45 ` Guenter Roeck
2010-12-10 17:37 ` Guenter Roeck
2010-12-10 17:48 ` Jean Delvare
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.