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@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
