Hi Jean,
On Tue, Mar 15, 2011 at 06:27:34AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> Sorry for the late answer.
>
As always, thanks for the detailed review.
> On Wed, 9 Feb 2011 19:07:57 -0800, Guenter Roeck wrote:
> > This patch adds support for additional sensor attributes to the sensors command.
> > It is essentially a re-submit of a patch submitted earlier, plus support
> > for additional sensor attributes which have been added to the sysfs ABI
> > since the original patch was submitted.
> >
> > This patch rewrites significant paths of chips.c to make it easier to add
> > additional attributes. A thorough review would therefore be helpful.
> >
> > --
> >
> > Resubmitting this one as well. Question is how to proceed with it
> > if no one has time for a thorough review. Maybe commit and clean up
> > with subsequent patches if needed ?
>
> What would certainly have helped would be to split the patch into at
> least two pieces, one adding the generic mechanism and converting the
> already supported limits to use is, and another one adding support for
> the new limits. It would have made reviewing slightly easier, and would
> also have demonstrated your point that the new mechanism makes adding
> new attributes and limits easier.
>
> Regarding the change itself, it is generally welcome, as the specific
> code had become difficult to maintain, and would have become totally
> horrible after adding support for the new limits. I wasn't especially
> proud of it in the first place, so I'm happy to see it go.
>
> I've tested the new code with valgrind and no problems were reported in
> my case (chips covered: radeon, coretemp, w83795adg, emc6d102 and
> adm1032.)
>
> I've also compared the output of "sensors" before and after the patch.
> Except for a decreasing amount trailing white space, which is
> definitely not an issue, I noticed two differences.
>
> The first difference is "hyst" being changed to "crit hyst" for
> critical temperature limit hysteresis. The original code enforced every
> temperature limit label to 4 characters to keep all lines nicely
> aligned, and your change breaks this effort. As the hysteresis value
> was always printed right after the limit it related to, on the same
> line, the "crit" in front of "hyst" was redundant. With the new code,
> there might be a new line in between, which isn't so nice. This should
> be addressed in an incremental patch IMHO (I can take care.)
>
ok. Problem though is that there are other sensor strings which don't fit
into four characters (crit low for temperatures, more for other sensor types).
We would have to change those as well, and possibly specify a fixed length for
limit strings associated with other sensor types.
> The second difference is the amount of space before the ALARM flags for
> voltage and temperature inputs. There used to be only two spaces,
> saving screen space and being in line with the temperature sensor type
> information. Now there are 4 spaces for voltages and temperatures, but
> still only 2 for fans. I presume this change wasn't made on purpose, so
> I'd like to see it reverted, with ALARM flags being preceded with 2
> spaces for all sensor types. Should be very easy to address, see below.
>
No problem.
> Now, to the code itself:
>
> >
> > Index: prog/sensors/chips.c
> > ===================================================================
> > --- prog/sensors/chips.c (revision 5917)
> > +++ prog/sensors/chips.c (working copy)
> > @@ -126,38 +126,124 @@
> > return max_size + 2;
> > }
> >
> > -static void print_temp_limits(double limit1, double limit2,
> > - const char *name1, const char *name2, int alarm)
> > +static void print_limits(struct sensor_limit_data *sensors,
> > + int sensor_count,
> > + struct sensor_limit_data *alarms,
> > + int alarm_count, int label_size,
> > + const char *fmt)
>
> sensors and sensor_count are really misnomers here. These are limits
> and limit_count.
>
Changed.
> > {
> > - if (fahrenheit) {
> > - limit1 = deg_ctof(limit1);
> > - limit2 = deg_ctof(limit2);
> > - }
> > + int i, j;
> > + int first = 1;
> >
> > - if (name2) {
> > - printf("(%-4s = %+5.1f%s, %-4s = %+5.1f%s) ",
> > - name1, limit1, degstr,
> > - name2, limit2, degstr);
> > - } else if (name1) {
> > - printf("(%-4s = %+5.1f%s) ",
> > - name1, limit1, degstr);
> > - } else {
> > - printf(" ");
> > + for (i = 0; i < sensor_count; i++) {
> > + if (!(i & 1)) {
> > + if (i)
> > + printf("\n%*s", label_size + 10, "");
> > + printf("(");
> > + } else if (i)
>
> I don't get this conditional... it's always true, isn't it?
>
Yes, removed.
> > + printf(", ");
> > + printf(fmt, sensors[i].name, sensors[i].value, sensors[i].unit);
> > + if ((i & 1) || i == sensor_count - 1) {
> > + printf(") ");
>
> The original code would insert many spaces here if only one limit was
> printed on this line, to align all potential ALARM flags regardless of
> each sensor having an odd or even limit count.
>
Ok, changed.
> > + if (first && alarm_count) {
>
> With your current implementation, testing "first" is equivalent to
> testing "i < 2", so you don't really need "first".
>
Ok, removed first.
> > + printf(" ALARM");
>
> You want "ALARM", not " ALARM", otherwise this is too many spaces. Or
> alternatively change the ") " above to just ")".
>
Ok. Changed to ")" to avoid unnecessary spaces at the end of the line.
> > + if (alarm_count > 1 || alarms[0].name) {
>
> The second test is sufficient. As a matter of fact, you print
> alarms[0].name unconditionally in the loop below.
>
You are right. Fixed.
> > + printf("(");
>
> You want " (" here.
>
Ok. That means the output will be "ALARM (MIN, MAX)".
> > + for (j = 0; j < alarm_count; j++) {
> > + printf("%s", alarms[j].name);
> > + if (j < alarm_count - 1)
> > + printf(", ");
> > + }
> > + printf(")");
> > + }
> > + first = 0;
> > + }
> > + }
> > }
> > +}
>
> You'll have to decide whether this function ends with printing a ")", or
> ") ". Right now, it does the former if there are alarms printed, and
> the latter if not. The calling code can't deal with this, obviously. It
> seems to me that ending on ")" would make more sense, to give the
> caller maximum flexibility.
>
Changed to ")", which also fixes the problem with too many blanks between ")" and "ALARM".
> Also, this function only prints the alarms if at least one limit was
> found. While it indeed makes little sense to have alarm flags without
> corresponding limits, well... there is weird hardware out there, so I'm
> not sure we can ignore this case.
>
Should now always print alarms, and at least try to align the output
even if there is only one (or no) sensor.
> >
> > - if (alarm)
> > - printf("ALARM ");
> > +static void get_sensor_limit_data(const sensors_chip_name *name,
> > + const sensors_feature *feature,
> > + const struct sensor_limits *ts,
> > + struct sensor_limit_data *ts_data,
> > + int *ts_sensors,
> > + struct sensor_limit_data *ts_alarm_data,
> > + int *ts_alarms)
>
> The names of the last 4 parameters are really confusing... even worse
> than for print_limits()! Please use the same naming convention for both
> functions. And in the calling functions, too. And I'm not even sure
> what "ts" stands for... "temperature sensor", as a legacy of a time
> where this function was only used to print temperature limits?
>
Probably because I started with temperatures.
Changed to
const struct sensor_subfeatures *sf,
struct sensor_subfeature_data *limits,
int *num_limits,
struct sensor_subfeature_data *alarms,
int *num_alarms)
which I hope is a bit better.
> > +{
> > + const sensors_subfeature *sf;
> > +
> > + for (; ts->subfeature >= 0; ts++) {
> > + sf = sensors_get_subfeature(name, feature, ts->subfeature);
> > + if (sf) {
> > + if (ts->alarm && get_value(name, sf)) {
> > + ts_alarm_data[*ts_alarms].name = ts->name;
> > + (*ts_alarms)++;
> > + } else if (!ts->alarm) {
>
> This test is partly redundant. If you split the get_value() test above,
> you can skip the !ts->alarm test. BTW the test on get_value()
> definitely deserves a comment. You only queue alarm subfeatures if
> the alarm is active, and you don't store their value, while limit
> subfeatures are always queued, with their value. The function should be
> documented as such.
>
Ok, done.
> > + ts_data[*ts_sensors].value
> > + = get_value(name, sf);
> > + ts_data[*ts_sensors].name = ts->name;
> > + (*ts_sensors)++;
> > + }
> > + if (ts->exists) {
> > + get_sensor_limit_data(name, feature, ts->exists,
> > + ts_data, ts_sensors,
> > + ts_alarm_data, ts_alarms);
> > + }
> > + } else if (!sf && ts->nexists)
>
> The test for !sf is redundant here, it will always succeed.
>
Ok, fixed.
> > + get_sensor_limit_data(name, feature, ts->nexists,
> > + ts_data, ts_sensors,
> > + ts_alarm_data, ts_alarms);
> > + }
> > }
>
> There is something I don't like about this function: you assume that
> the provided ts, ts_sensors and ts_alarms are such that all the
> subfeatures from ts (including exists and nexists recursions) will fit
> in ts_sensors and ts_alarms are. If this assumption fails, you'll
> overrun the arrays and corrupt the stack, which is pretty bad. The
> recursive relations between the various ts arrays make it easy to get
> wrong, methinks.
>
> Unfortunately I see no way to check the sizes for correctness at build
> time, which means we'll have to do it at run time. This could be done
> with the current prototype by having the caller pass the array sizes as
> the initial values of ts_sensors and ts_alarms. The code would only
> have to be adjusted a little. Or maybe you have a better idea?
>
One other idea would be to dynamically allocate data as needed. Not sure if that
would be much better. For now, I added parameters for max_limits and max_alarms.
> >
> > +static const struct sensor_limits temp_alarms[] = {
> > + { SENSORS_SUBFEATURE_TEMP_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
> > + { SENSORS_SUBFEATURE_TEMP_MIN_ALARM, NULL, NULL, 1, "MIN" },
> > + { SENSORS_SUBFEATURE_TEMP_MAX_ALARM, NULL, NULL, 1, "MAX" },
> > + { SENSORS_SUBFEATURE_TEMP_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
> > + { SENSORS_SUBFEATURE_TEMP_EMERGENCY_ALARM, NULL, NULL, 1, "EMERGENCY" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits temp_max_sensors[] = {
> > + { SENSORS_SUBFEATURE_TEMP_MAX_HYST, NULL, NULL, 0, "hyst" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits temp_crit_sensors[] = {
> > + { SENSORS_SUBFEATURE_TEMP_CRIT_HYST, NULL, NULL, 0, "crit hyst" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits temp_emergency_sensors[] = {
> > + { SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST, NULL, NULL, 0,
> > + "emergency hyst" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits temp_sensors[] = {
> > + { SENSORS_SUBFEATURE_TEMP_ALARM, NULL, temp_alarms, 1, NULL },
> > + { SENSORS_SUBFEATURE_TEMP_MIN, NULL, NULL, 0, "low" },
> > + { SENSORS_SUBFEATURE_TEMP_MAX, temp_max_sensors, NULL, 0, "high" },
> > + { SENSORS_SUBFEATURE_TEMP_LCRIT, NULL, NULL, 0, "crit low" },
> > + { SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, NULL, 0, "crit" },
> > + { SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, NULL, 0,
> > + "emergency" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > static void print_chip_temp(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > - const sensors_subfeature *sf, *sfmin, *sfmax, *sfcrit, *sfhyst;
> > - double val, limit1, limit2;
> > - const char *s1, *s2;
> > - int alarm, crit_displayed = 0;
> > + struct sensor_limit_data sensors[8];
> > + struct sensor_limit_data alarms[5];
> > + int sensor_count, alarm_count;
> > + const sensors_subfeature *sf;
> > + double val;
> > char *label;
> > + int i;
> > + char fmt[32];
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> > @@ -168,80 +254,6 @@
> > free(label);
> >
> > sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_ALARM);
> > - alarm = sf && get_value(name, sf);
> > -
> > - sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_MIN);
> > - sfmax = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_MAX);
> > - sfcrit = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_CRIT);
> > - if (sfmax) {
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_MAX_ALARM);
> > - if (sf && get_value(name, sf))
> > - alarm |= 1;
> > -
> > - if (sfmin) {
> > - limit1 = get_value(name, sfmin);
> > - s1 = "low";
> > - limit2 = get_value(name, sfmax);
> > - s2 = "high";
> > -
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_MIN_ALARM);
> > - if (sf && get_value(name, sf))
> > - alarm |= 1;
> > - } else {
> > - limit1 = get_value(name, sfmax);
> > - s1 = "high";
> > -
> > - sfhyst = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_MAX_HYST);
> > - if (sfhyst) {
> > - limit2 = get_value(name, sfhyst);
> > - s2 = "hyst";
> > - } else if (sfcrit) {
> > - limit2 = get_value(name, sfcrit);
> > - s2 = "crit";
> > -
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
> > - if (sf && get_value(name, sf))
> > - alarm |= 1;
> > - crit_displayed = 1;
> > - } else {
> > - limit2 = 0;
> > - s2 = NULL;
> > - }
> > - }
> > - } else if (sfcrit) {
> > - limit1 = get_value(name, sfcrit);
> > - s1 = "crit";
> > -
> > - sfhyst = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_CRIT_HYST);
> > - if (sfhyst) {
> > - limit2 = get_value(name, sfhyst);
> > - s2 = "hyst";
> > - } else {
> > - limit2 = 0;
> > - s2 = NULL;
> > - }
> > -
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
> > - if (sf && get_value(name, sf))
> > - alarm |= 1;
> > - crit_displayed = 1;
> > - } else {
> > - limit1 = limit2 = 0;
> > - s1 = s2 = NULL;
> > - }
> > -
> > -
> > - sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_TEMP_FAULT);
> > if (sf && get_value(name, sf)) {
> > printf(" FAULT ");
> > @@ -256,30 +268,21 @@
> > } else
> > printf(" N/A ");
> > }
> > - print_temp_limits(limit1, limit2, s1, s2, alarm);
> >
> > - if (!crit_displayed && sfcrit) {
> > - limit1 = get_value(name, sfcrit);
> > - s1 = "crit";
> > + sensor_count = alarm_count = 0;
> > + get_sensor_limit_data(name, feature, temp_sensors,
> > + sensors, &sensor_count,
> > + alarms, &alarm_count);
> >
> > - sfhyst = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_CRIT_HYST);
> > - if (sfhyst) {
> > - limit2 = get_value(name, sfhyst);
> > - s2 = "hyst";
> > - } else {
> > - limit2 = 0;
> > - s2 = NULL;
> > - }
> > + if (fahrenheit)
> > + for (i = 0; i < sensor_count; i++)
> > + sensors[i].value = deg_ctof(sensors[i].value);
> >
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_TEMP_CRIT_ALARM);
> > - alarm = sf && get_value(name, sf);
> > + strcpy(fmt, "%-4s = %+5.1f");
> > + strcat(fmt, degstr);
>
> I strongly discourage the use of strcpy and strcat, especially on stack
> buffers, for both safety and performance reasons. I would have
> suggested the use of snprintf instead, but...
>
Sure snprintf would be faster ?
> You implemented a very nice mechanism to attach units to limit values,
> which you use for power limits, I fail to see why you don't just use it
> here as well? It would make the code more elegant IMHO (see attached
> patch.)
>
I applied your patch, so should be ok now.
> > + print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
> > + fmt);
> >
> > - printf("\n%*s", label_size + 10, "");
> > - print_temp_limits(limit1, limit2, s1, s2, alarm);
> > - }
> > -
> > /* print out temperature sensor info */
> > sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_TEMP_TYPE);
> > @@ -302,13 +305,33 @@
> > printf("\n");
> > }
> >
> > +static const struct sensor_limits voltage_alarms[] = {
> > + { SENSORS_SUBFEATURE_IN_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
> > + { SENSORS_SUBFEATURE_IN_MIN_ALARM, NULL, NULL, 1, "MIN" },
> > + { SENSORS_SUBFEATURE_IN_MAX_ALARM, NULL, NULL, 1, "MAX" },
> > + { SENSORS_SUBFEATURE_IN_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits voltage_sensors[] = {
> > + { SENSORS_SUBFEATURE_IN_ALARM, NULL, voltage_alarms, 1, NULL },
> > + { SENSORS_SUBFEATURE_IN_LCRIT, NULL, NULL, 0, "crit min" },
> > + { SENSORS_SUBFEATURE_IN_MIN, NULL, NULL, 0, "min" },
> > + { SENSORS_SUBFEATURE_IN_MAX, NULL, NULL, 0, "max" },
> > + { SENSORS_SUBFEATURE_IN_CRIT, NULL, NULL, 0, "crit max" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > static void print_chip_in(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > - const sensors_subfeature *sf, *sfmin, *sfmax;
> > - double val, alarm_max, alarm_min;
> > + const sensors_subfeature *sf;
> > char *label;
> > + struct sensor_limit_data sensors[4];
> > + struct sensor_limit_data alarms[4];
> > + int sensor_count, alarm_count;
> > + double val;
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> > @@ -321,50 +344,18 @@
> > sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_IN_INPUT);
> > if (sf && get_input_value(name, sf, &val) == 0)
> > - printf("%+6.2f V", val);
> > + printf("%+6.2f V ", val);
> > else
> > - printf(" N/A");
> > + printf(" N/A ");
> >
> > - sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_IN_MIN);
> > - sfmax = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_IN_MAX);
> > - if (sfmin && sfmax)
> > - printf(" (min = %+6.2f V, max = %+6.2f V)",
> > - get_value(name, sfmin),
> > - get_value(name, sfmax));
> > - else if (sfmin)
> > - printf(" (min = %+6.2f V)",
> > - get_value(name, sfmin));
> > - else if (sfmax)
> > - printf(" (max = %+6.2f V)",
> > - get_value(name, sfmax));
> > + sensor_count = alarm_count = 0;
> > + get_sensor_limit_data(name, feature, voltage_sensors,
> > + sensors, &sensor_count,
> > + alarms, &alarm_count);
> >
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_IN_ALARM);
> > - sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_IN_MIN_ALARM);
> > - sfmax = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_IN_MAX_ALARM);
> > - if (sfmin || sfmax) {
> > - alarm_max = sfmax ? get_value(name, sfmax) : 0;
> > - alarm_min = sfmin ? get_value(name, sfmin) : 0;
> > + print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
> > + "%s = %+6.2f V");
> >
> > - if (alarm_min || alarm_max) {
> > - printf(" ALARM (");
> > -
> > - if (alarm_min)
> > - printf("MIN");
> > - if (alarm_max)
> > - printf("%sMAX", (alarm_min) ? ", " : "");
> > -
> > - printf(")");
> > - }
> > - } else if (sf) {
> > - printf(" %s",
> > - get_value(name, sf) ? "ALARM" : "");
> > - }
> > -
> > printf("\n");
> > }
> >
> > @@ -450,15 +441,53 @@
> > *prefixstr = scale->unit;
> > }
> >
> > +static const struct sensor_limits power_alarms[] = {
> > + { SENSORS_SUBFEATURE_POWER_CAP_ALARM, NULL, NULL, 1, "CAP" },
> > + { SENSORS_SUBFEATURE_POWER_MAX_ALARM, NULL, NULL, 1, "MAX" },
> > + { SENSORS_SUBFEATURE_POWER_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits power_inst_highest[] = {
> > + { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, NULL, 0, "max" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits power_inst_max[] = {
> > + { SENSORS_SUBFEATURE_POWER_MAX, NULL, NULL, 0, "max" },
> > + { SENSORS_SUBFEATURE_POWER_CRIT, NULL, NULL, 0, "crit" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits power_inst_sensors[] = {
> > + { SENSORS_SUBFEATURE_POWER_ALARM, NULL, power_alarms, 1, NULL },
> > + { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, power_inst_highest,
> > + power_inst_max, 0, "min" },
>
> This deserves a comment at least... if it is correct at all. Why would
> INPUT_HIGHEST require INPUT_LOWEST? I can imagine a chip providing one
> without the other (this would even make a lot of sense if you ask me).
> I also don't get why MAX and CRIT would be mutually exclusive with
> INPUT_LOWEST and INPUT_HIGHEST. Please explain or fix.
>
Backwards compatibility issue. "max" and "min" were traditionally used
to display "highest" and "lowest", and I did not want to change that.
However, that overlaps with "max" for the new SENSORS_SUBFEATURE_POWER_MAX.
I've changed the highest output to to "hmax" to lift the limitation.
Not optimal, since there is still the average max which is also displayed
as "max". Please let me know if you have a better idea.
> > + { SENSORS_SUBFEATURE_POWER_CAP, NULL, NULL, 0, "cap" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits power_avg_sensors[] = {
> > + { SENSORS_SUBFEATURE_POWER_ALARM, NULL, NULL, 1, NULL },
> > + { SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, NULL, 0, "min" },
> > + { SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, NULL, 0, "max" },
> > + { SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, NULL, 0,
> > + "interval" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > static void print_chip_power(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > double val;
> > - int need_space = 0;
> > - const sensors_subfeature *sf, *sfmin, *sfmax, *sfint;
> > + const sensors_subfeature *sf;
> > + struct sensor_limit_data sensors[4];
> > + struct sensor_limit_data alarms[3];
> > + int sensor_count, alarm_count;
> > char *label;
> > const char *unit;
> > + int i;
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> > @@ -468,60 +497,36 @@
> > print_label(label, label_size);
> > free(label);
> >
> > + sensor_count = alarm_count = 0;
> > +
> > /* Power sensors come in 2 flavors: instantaneous and averaged.
> > To keep things simple, we assume that each sensor only implements
> > one flavor. */
> > sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_POWER_INPUT);
> > if (sf) {
> > - sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST);
> > - sfmax = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_POWER_INPUT_LOWEST);
> > - sfint = NULL;
> > + get_sensor_limit_data(name, feature, power_inst_sensors,
> > + sensors, &sensor_count, alarms,
> > + &alarm_count);
> > } else {
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_POWER_AVERAGE);
> > - sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST);
> > - sfmax = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST);
> > - sfint = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL);
> > + get_sensor_limit_data(name, feature, power_avg_sensors,
> > + sensors, &sensor_count, alarms,
> > + &alarm_count);
> > }
> >
> > if (sf && get_input_value(name, sf, &val) == 0) {
> > scale_value(&val, &unit);
> > - printf("%6.2f %sW", val, unit);
> > + printf("%6.2f %sW ", val, unit);
> > } else
> > - printf(" N/A");
> > + printf(" N/A ");
> >
> > - if (sfmin || sfmax || sfint) {
> > - printf(" (");
> > + for (i = 0; i < sensor_count; i++)
> > + scale_value(&sensors[i].value, &sensors[i].unit);
> >
> > - if (sfmin) {
> > - val = get_value(name, sfmin);
> > - scale_value(&val, &unit);
> > - printf("min = %6.2f %sW", val, unit);
> > - need_space = 1;
> > - }
> > + if (sensor_count)
>
> This test isn't needed, print_limits() will deal with the !sensor_count
> case just fine (and as a matter of fact you don't have this test in any
> other function.)
>
Removed the test.
> > + print_limits(sensors, sensor_count, alarms, alarm_count,
> > + label_size, "%s = %6.2f %sW");
> >
> > - if (sfmax) {
> > - val = get_value(name, sfmax);
> > - scale_value(&val, &unit);
> > - printf("%smax = %6.2f %sW", (need_space ? ", " : ""),
> > - val, unit);
> > - need_space = 1;
> > - }
> > -
> > - if (sfint) {
> > - printf("%sinterval = %6.2f s", (need_space ? ", " : ""),
> > - get_value(name, sfint));
> > - need_space = 1;
> > - }
> > - printf(")");
> > - }
> > -
> > printf("\n");
> > }
> >
> > @@ -595,13 +600,33 @@
> > free(label);
> > }
> >
> > +static const struct sensor_limits current_alarms[] = {
> > + { SENSORS_SUBFEATURE_CURR_LCRIT_ALARM, NULL, NULL, 1, "LCRIT" },
> > + { SENSORS_SUBFEATURE_CURR_MIN_ALARM, NULL, NULL, 1, "MIN" },
> > + { SENSORS_SUBFEATURE_CURR_MAX_ALARM, NULL, NULL, 1, "MAX" },
> > + { SENSORS_SUBFEATURE_CURR_CRIT_ALARM, NULL, NULL, 1, "CRIT" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > +static const struct sensor_limits current_sensors[] = {
> > + { SENSORS_SUBFEATURE_CURR_ALARM, NULL, current_alarms, 1, NULL },
> > + { SENSORS_SUBFEATURE_CURR_LCRIT, NULL, NULL, 0, "crit min" },
> > + { SENSORS_SUBFEATURE_CURR_MIN, NULL, NULL, 0, "min" },
> > + { SENSORS_SUBFEATURE_CURR_MAX, NULL, NULL, 0, "max" },
> > + { SENSORS_SUBFEATURE_CURR_CRIT, NULL, NULL, 0, "crit max" },
> > + { -1, NULL, NULL, 0, NULL }
> > +};
> > +
> > static void print_chip_curr(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > - const sensors_subfeature *sf, *sfmin, *sfmax;
> > - double alarm_max, alarm_min, val;
> > + const sensors_subfeature *sf;
> > + double val;
> > char *label;
> > + struct sensor_limit_data sensors[4];
> > + struct sensor_limit_data alarms[4];
> > + int sensor_count, alarm_count;
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > fprintf(stderr, "ERROR: Can't get label of feature %s!\n",
> > @@ -614,50 +639,18 @@
> > sf = sensors_get_subfeature(name, feature,
> > SENSORS_SUBFEATURE_CURR_INPUT);
> > if (sf && get_input_value(name, sf, &val) == 0)
> > - printf("%+6.2f A", val);
> > + printf("%+6.2f A ", val);
> > else
> > - printf(" N/A");
> > + printf(" N/A ");
> >
> > - sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_CURR_MIN);
> > - sfmax = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_CURR_MAX);
> > - if (sfmin && sfmax)
> > - printf(" (min = %+6.2f A, max = %+6.2f A)",
> > - get_value(name, sfmin),
> > - get_value(name, sfmax));
> > - else if (sfmin)
> > - printf(" (min = %+6.2f A)",
> > - get_value(name, sfmin));
> > - else if (sfmax)
> > - printf(" (max = %+6.2f A)",
> > - get_value(name, sfmax));
> > + sensor_count = alarm_count = 0;
> > + get_sensor_limit_data(name, feature, current_sensors,
> > + sensors, &sensor_count,
> > + alarms, &alarm_count);
> >
> > - sf = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_CURR_ALARM);
> > - sfmin = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_CURR_MIN_ALARM);
> > - sfmax = sensors_get_subfeature(name, feature,
> > - SENSORS_SUBFEATURE_CURR_MAX_ALARM);
> > - if (sfmin || sfmax) {
> > - alarm_max = sfmax ? get_value(name, sfmax) : 0;
> > - alarm_min = sfmin ? get_value(name, sfmin) : 0;
> > + print_limits(sensors, sensor_count, alarms, alarm_count, label_size,
> > + "%s = %+6.2f A");
> >
> > - if (alarm_min || alarm_max) {
> > - printf(" ALARM (");
> > -
> > - if (alarm_min)
> > - printf("MIN");
> > - if (alarm_max)
> > - printf("%sMAX", (alarm_min) ? ", " : "");
> > -
> > - printf(")");
> > - }
> > - } else if (sf) {
> > - printf(" %s",
> > - get_value(name, sf) ? "ALARM" : "");
> > - }
> > -
> > printf("\n");
> > }
> >
> > Index: prog/sensors/chips.h
> > ===================================================================
> > --- prog/sensors/chips.h (revision 5917)
> > +++ prog/sensors/chips.h (working copy)
> > @@ -24,6 +24,29 @@
> >
> > #include "lib/sensors.h"
> >
> > +/*
> > + * Retrieved limits
> > + */
> > +struct sensor_limit_data {
> > + double value;
> > + const char *name;
> > + const char *unit;
>
> Please add a comment on what unit can be used for and the fact that it
> is optional.
>
> > +};
> > +
> > +/*
> > + * Limit data structure. Used to create a table of supported limits for a given
> > + * feature.
> > + */
> > +struct sensor_limits {
> > + int subfeature; /* Limit we are looking for */
> > + const struct sensor_limits *exists; /* Secondary limits if limit
> > + exists */
> > + const struct sensor_limits *nexists; /* Secondary limits if limit
> > + does not exist */
>
> "Secondary" is a generic term. In the first case, you are talking about
> a complement of information. In the second case, you a talking about
> alternatives. I think the terms "Complementary" and "Alternative" would
> be more descriptive than "Secondary.
>
Makes sense. Changed.
> > + int alarm; /* true if this is an alarm */
> > + const char *name; /* limit name to be printed */
> > +};
>
> Your use of the word "limit" in the above structures is somewhat
> misleading, as they are suitable for both limits and alarms, and could
> presumably be extended to other subfeatures as well in the future (in
> particular I start thinking that temperature sensor types would fit in
> there nicely.) I think you should use the word "subfeature" instead.
>
Problem with that is that it results in "sensor_subfeatures"
which conflicts with the existing "sensors_subfeature". So I'll have to find
something different.
> > +
> > void print_chip_raw(const sensors_chip_name *name);
> > void print_chip(const sensors_chip_name *name);
> >
>
> I really like the spirit of the patch. Once fixed, it'll be awesome.
>
> Do you have any more patch pending for lm-sensors? I'd like to flush
> the queue and then schedule the release of lm-sensors 3.3.0.
>
Nothing major.
I have a driver for MAX16065 in the queue, but it looks like that won't be detectable.
I am also looking into support for DS1721, DS1631, and DS1731. DS1721 is supported
by the ds1621 driver, though auto-detection does not work if the chip is in "standard"
mode (the driver itself still works, though). I am waiting for samples for the other chips.
Once I get those, I'll test the driver and update it as/if needed. I don't have an ETA for
that, though.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
