* Re: [lm-sensors] [PATCH v2] Add support for new attributes to
2011-11-06 22:46 [lm-sensors] [PATCH v2] Add support for new attributes to Guenter Roeck
@ 2012-01-08 21:45 ` Jean Delvare
2012-01-08 22:20 ` Guenter Roeck
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2012-01-08 21:45 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
Sorry for the late review.
On Sun, 6 Nov 2011 14:46:22 -0800, Guenter Roeck wrote:
> Add support for new sysfs attributes to libsensors and to the sensors command.
>
> v2: Use defines for array sizes. Some of the resulting arrays are larger than necessary,
> but that is better than missing an array size update if the number of attributes
> changes.
I don't get the logic. I understand the point of computing the sizes
automatically to avoid an overflow [1], but then why don't you compute
the right values? It's only a matter of adding a "- 1" after every call
to ARRAY_SIZE(), this seems fairly easy and cheap.
Or maybe you were referring to the fact that some attributes are
mutually exclusive per Documentation/hwmon/sysfs-interface (in
particular, general alarm flag vs. limit-specific alarm flags.) It was
probably a bad idea to rely on this in the first place anyway, so I
have no objection in changing this.
[1] Note that the overflows couldn't really happen, BTW, as we have
code to catch them in get_sensor_limit_data(). Maybe these checks can
go away if the array sizes are now guaranteed to be sufficient by
construction?
This should really be a separate patch, BTW, as this is not related to
the original purpose of the patch.
> --
> Index: doc/libsensors-API.txt
> =================================> --- doc/libsensors-API.txt (revision 5996)
> +++ doc/libsensors-API.txt (working copy)
> @@ -7,6 +7,16 @@
> given new feature.
>
> 0x431 lm-sensors 3.3.0 to 3.3.1
> +* Added support for new sysfs attributes
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_AVERAGE
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_LOWEST
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_HIGHEST
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_AVERAGE
I don't see this one in Documentation/hwmon/sysfs-interface.
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_LOWEST
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_HIGHEST
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_AVERAGE
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_LOWEST
> + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_HIGHEST
> * Added support for intrusion detection
> enum sensors_feature_type SENSORS_FEATURE_INTRUSION
> enum sensors_subfeature_type SENSORS_SUBFEATURE_INTRUSION_ALARM
> Index: lib/sensors.h
> =================================> --- lib/sensors.h (revision 5996)
> +++ lib/sensors.h (working copy)
> @@ -157,6 +157,9 @@
> SENSORS_SUBFEATURE_IN_MAX,
> SENSORS_SUBFEATURE_IN_LCRIT,
> SENSORS_SUBFEATURE_IN_CRIT,
> + SENSORS_SUBFEATURE_IN_AVERAGE,
> + SENSORS_SUBFEATURE_IN_LOWEST,
> + SENSORS_SUBFEATURE_IN_HIGHEST,
> SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
> SENSORS_SUBFEATURE_IN_MIN_ALARM,
> SENSORS_SUBFEATURE_IN_MAX_ALARM,
> @@ -181,6 +184,9 @@
> SENSORS_SUBFEATURE_TEMP_LCRIT,
> SENSORS_SUBFEATURE_TEMP_EMERGENCY,
> SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST,
> + SENSORS_SUBFEATURE_TEMP_AVERAGE,
> + SENSORS_SUBFEATURE_TEMP_LOWEST,
> + SENSORS_SUBFEATURE_TEMP_HIGHEST,
> SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
> SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
> SENSORS_SUBFEATURE_TEMP_MIN_ALARM,
> @@ -215,6 +221,9 @@
> SENSORS_SUBFEATURE_CURR_MAX,
> SENSORS_SUBFEATURE_CURR_LCRIT,
> SENSORS_SUBFEATURE_CURR_CRIT,
> + SENSORS_SUBFEATURE_CURR_AVERAGE,
> + SENSORS_SUBFEATURE_CURR_LOWEST,
> + SENSORS_SUBFEATURE_CURR_HIGHEST,
> SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
> SENSORS_SUBFEATURE_CURR_MIN_ALARM,
> SENSORS_SUBFEATURE_CURR_MAX_ALARM,
> Index: lib/sysfs.c
> =================================> --- lib/sysfs.c (revision 5996)
> +++ lib/sysfs.c (working copy)
> @@ -233,6 +233,9 @@
> { "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
> { "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
> { "emergency_hyst", SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST },
> + { "average", SENSORS_SUBFEATURE_TEMP_AVERAGE },
> + { "lowest", SENSORS_SUBFEATURE_TEMP_LOWEST },
> + { "highest", SENSORS_SUBFEATURE_TEMP_HIGHEST },
> { "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
> { "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
> { "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },
> @@ -252,6 +255,9 @@
> { "max", SENSORS_SUBFEATURE_IN_MAX },
> { "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
> { "crit", SENSORS_SUBFEATURE_IN_CRIT },
> + { "average", SENSORS_SUBFEATURE_IN_AVERAGE },
> + { "lowest", SENSORS_SUBFEATURE_IN_LOWEST },
> + { "highest", SENSORS_SUBFEATURE_IN_HIGHEST },
> { "alarm", SENSORS_SUBFEATURE_IN_ALARM },
> { "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
> { "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },
> @@ -302,6 +308,9 @@
> { "max", SENSORS_SUBFEATURE_CURR_MAX },
> { "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
> { "crit", SENSORS_SUBFEATURE_CURR_CRIT },
> + { "average", SENSORS_SUBFEATURE_CURR_AVERAGE },
> + { "lowest", SENSORS_SUBFEATURE_CURR_LOWEST },
> + { "highest", SENSORS_SUBFEATURE_CURR_HIGHEST },
> { "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
> { "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
> { "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },
> Index: prog/sensors/chips.c
> =================================> --- prog/sensors/chips.c (revision 5996)
> +++ prog/sensors/chips.c (working copy)
> @@ -280,15 +280,26 @@
> { SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, 0, "crit" },
> { SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, 0,
> "emerg" },
> + { SENSORS_SUBFEATURE_TEMP_AVERAGE, NULL, 0, "avg" },
> + { SENSORS_SUBFEATURE_TEMP_LOWEST, NULL, 0, "lowest" },
> + { SENSORS_SUBFEATURE_TEMP_HIGHEST, NULL, 0, "highest" },
> { -1, NULL, 0, NULL }
> };
>
> +#define NUM_TEMP_ALARMS 6
> +#define NUM_TEMP_SENSORS (ARRAY_SIZE(temp_sensors) \
> + + ARRAY_SIZE(temp_max_sensors) \
> + + ARRAY_SIZE(temp_crit_sensors) \
> + + ARRAY_SIZE(temp_emergency_sensors) \
> + - NUM_TEMP_ALARMS)
> +
> +
> static void print_chip_temp(const sensors_chip_name *name,
> const sensors_feature *feature,
> int label_size)
> {
> - struct sensor_subfeature_data sensors[8];
> - struct sensor_subfeature_data alarms[5];
> + struct sensor_subfeature_data sensors[NUM_TEMP_SENSORS];
> + struct sensor_subfeature_data alarms[NUM_TEMP_ALARMS];
> int sensor_count, alarm_count;
> const sensors_subfeature *sf;
> double val;
> @@ -365,17 +376,23 @@
> { SENSORS_SUBFEATURE_IN_MIN, NULL, 0, "min" },
> { SENSORS_SUBFEATURE_IN_MAX, NULL, 0, "max" },
> { SENSORS_SUBFEATURE_IN_CRIT, NULL, 0, "crit max" },
> + { SENSORS_SUBFEATURE_IN_AVERAGE, NULL, 0, "avg" },
> + { SENSORS_SUBFEATURE_IN_LOWEST, NULL, 0, "lowest" },
> + { SENSORS_SUBFEATURE_IN_HIGHEST, NULL, 0, "highest" },
> { -1, NULL, 0, NULL }
> };
>
> +#define NUM_IN_ALARMS 5
> +#define NUM_IN_SENSORS (ARRAY_SIZE(voltage_sensors) - NUM_IN_ALARMS)
> +
> static void print_chip_in(const sensors_chip_name *name,
> const sensors_feature *feature,
> int label_size)
> {
> const sensors_subfeature *sf;
> char *label;
> - struct sensor_subfeature_data sensors[4];
> - struct sensor_subfeature_data alarms[4];
> + struct sensor_subfeature_data sensors[NUM_IN_SENSORS];
> + struct sensor_subfeature_data alarms[NUM_IN_ALARMS];
> int sensor_count, alarm_count;
> double val;
>
> @@ -504,6 +521,7 @@
> };
>
> static const struct sensor_subfeature_list power_inst_sensors[] = {
> + { SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },
I'm confused. This one was missing on purpose. For power sensors we
consider instantaneous and averaging sensors as different types. The
code in function print_chip_power is pretty clear about this. So, just
as all _INPUT subfeatures aren't listed in limit arrays,
SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.
> { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" },
> { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" },
> { -1, NULL, 0, NULL }
> @@ -517,14 +535,20 @@
> { -1, NULL, 0, NULL }
> };
>
> +#define NUM_POWER_ALARMS 4
> +#define NUM_POWER_SENSORS (ARRAY_SIZE(power_common_sensors) \
> + + ARRAY_SIZE(power_inst_sensors) \
> + + ARRAY_SIZE(power_avg_sensors) \
> + - NUM_POWER_ALARMS)
Due to the specificity of the power sensors described above, the
computed value is larger than needed. We'll use power_inst_sensors or
power_avg_sensors but never both. You could define a MAX() macro to
select the right one for to compute NUM_POWER_SENSORS.
If you want to change the code to display both types, this can be
discussed, but please keep the definition of NUM_POWER_SENSORS in line
with the code that uses it, for clarity.
> +
> static void print_chip_power(const sensors_chip_name *name,
> const sensors_feature *feature,
> int label_size)
> {
> double val;
> const sensors_subfeature *sf;
> - struct sensor_subfeature_data sensors[6];
> - struct sensor_subfeature_data alarms[3];
> + struct sensor_subfeature_data sensors[NUM_POWER_SENSORS];
> + struct sensor_subfeature_data alarms[NUM_POWER_ALARMS];
> int sensor_count, alarm_count;
> char *label;
> const char *unit;
> @@ -653,9 +677,15 @@
> { SENSORS_SUBFEATURE_CURR_MIN, NULL, 0, "min" },
> { SENSORS_SUBFEATURE_CURR_MAX, NULL, 0, "max" },
> { SENSORS_SUBFEATURE_CURR_CRIT, NULL, 0, "crit max" },
> + { SENSORS_SUBFEATURE_CURR_AVERAGE, NULL, 0, "avg" },
> + { SENSORS_SUBFEATURE_CURR_LOWEST, NULL, 0, "lowest" },
> + { SENSORS_SUBFEATURE_CURR_HIGHEST, NULL, 0, "highest" },
> { -1, NULL, 0, NULL }
> };
>
> +#define NUM_CURR_ALARM 5
> +#define NUM_CURR_SENSORS (ARRAY_SIZE(current_sensors) - NUM_CURR_ALARM)
> +
> static void print_chip_curr(const sensors_chip_name *name,
> const sensors_feature *feature,
> int label_size)
> @@ -663,8 +693,8 @@
> const sensors_subfeature *sf;
> double val;
> char *label;
> - struct sensor_subfeature_data sensors[4];
> - struct sensor_subfeature_data alarms[4];
> + struct sensor_subfeature_data sensors[NUM_CURR_SENSORS];
> + struct sensor_subfeature_data alarms[NUM_CURR_ALARM];
> int sensor_count, alarm_count;
>
> if (!(label = sensors_get_label(name, feature))) {
> Index: CHANGES
> =================================> --- CHANGES (revision 5996)
> +++ CHANGES (working copy)
> @@ -2,6 +2,10 @@
> -----------------------
>
> SVN HEAD
> + libsensors: Added support for new sysfs attributes
> + sensors: Added support for new sysfs attributes
> + For power sensors, display both instantaneous and average data
> + if available
Your patch doesn't actually implement that second change. Which is
good, BTW, this unrelated change would really belong to a separate
patch.
> sensors-detect: Stop calling for PIIX5 SMBus testers
> Improve filtering of fake DMI data
> Print DMI system/product version if available
--
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] Add support for new attributes to
2011-11-06 22:46 [lm-sensors] [PATCH v2] Add support for new attributes to Guenter Roeck
2012-01-08 21:45 ` Jean Delvare
@ 2012-01-08 22:20 ` Guenter Roeck
2012-01-09 0:14 ` Guenter Roeck
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-01-08 22:20 UTC (permalink / raw)
To: lm-sensors
On Sun, Jan 08, 2012 at 04:45:02PM -0500, Jean Delvare wrote:
> Hi Guenter,
>
> Sorry for the late review.
>
> On Sun, 6 Nov 2011 14:46:22 -0800, Guenter Roeck wrote:
> > Add support for new sysfs attributes to libsensors and to the sensors command.
> >
> > v2: Use defines for array sizes. Some of the resulting arrays are larger than necessary,
> > but that is better than missing an array size update if the number of attributes
> > changes.
>
> I don't get the logic. I understand the point of computing the sizes
> automatically to avoid an overflow [1], but then why don't you compute
> the right values? It's only a matter of adding a "- 1" after every call
> to ARRAY_SIZE(), this seems fairly easy and cheap.
>
> Or maybe you were referring to the fact that some attributes are
> mutually exclusive per Documentation/hwmon/sysfs-interface (in
> particular, general alarm flag vs. limit-specific alarm flags.) It was
> probably a bad idea to rely on this in the first place anyway, so I
> have no objection in changing this.
>
Yes, correct, though I had mostly the power sensors in mind if I recall correctly.
> [1] Note that the overflows couldn't really happen, BTW, as we have
> code to catch them in get_sensor_limit_data(). Maybe these checks can
> go away if the array sizes are now guaranteed to be sufficient by
> construction?
>
> This should really be a separate patch, BTW, as this is not related to
> the original purpose of the patch.
>
Sure, no problem.
> > Index: doc/libsensors-API.txt
> > =================================> > --- doc/libsensors-API.txt (revision 5996)
> > +++ doc/libsensors-API.txt (working copy)
> > @@ -7,6 +7,16 @@
> > given new feature.
> >
> > 0x431 lm-sensors 3.3.0 to 3.3.1
> > +* Added support for new sysfs attributes
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_AVERAGE
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_LOWEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_HIGHEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_AVERAGE
>
> I don't see this one in Documentation/hwmon/sysfs-interface.
>
Me not either, nor am I aware of any sensors implementing it.
No idea where I got that idea from.
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_LOWEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_HIGHEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_AVERAGE
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_LOWEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_HIGHEST
> > * Added support for intrusion detection
> > enum sensors_feature_type SENSORS_FEATURE_INTRUSION
> > enum sensors_subfeature_type SENSORS_SUBFEATURE_INTRUSION_ALARM
> > Index: lib/sensors.h
> > =================================> > --- lib/sensors.h (revision 5996)
> > +++ lib/sensors.h (working copy)
> > @@ -157,6 +157,9 @@
> > SENSORS_SUBFEATURE_IN_MAX,
> > SENSORS_SUBFEATURE_IN_LCRIT,
> > SENSORS_SUBFEATURE_IN_CRIT,
> > + SENSORS_SUBFEATURE_IN_AVERAGE,
> > + SENSORS_SUBFEATURE_IN_LOWEST,
> > + SENSORS_SUBFEATURE_IN_HIGHEST,
> > SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
> > SENSORS_SUBFEATURE_IN_MIN_ALARM,
> > SENSORS_SUBFEATURE_IN_MAX_ALARM,
> > @@ -181,6 +184,9 @@
> > SENSORS_SUBFEATURE_TEMP_LCRIT,
> > SENSORS_SUBFEATURE_TEMP_EMERGENCY,
> > SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST,
> > + SENSORS_SUBFEATURE_TEMP_AVERAGE,
> > + SENSORS_SUBFEATURE_TEMP_LOWEST,
> > + SENSORS_SUBFEATURE_TEMP_HIGHEST,
> > SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
> > SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
> > SENSORS_SUBFEATURE_TEMP_MIN_ALARM,
> > @@ -215,6 +221,9 @@
> > SENSORS_SUBFEATURE_CURR_MAX,
> > SENSORS_SUBFEATURE_CURR_LCRIT,
> > SENSORS_SUBFEATURE_CURR_CRIT,
> > + SENSORS_SUBFEATURE_CURR_AVERAGE,
> > + SENSORS_SUBFEATURE_CURR_LOWEST,
> > + SENSORS_SUBFEATURE_CURR_HIGHEST,
> > SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
> > SENSORS_SUBFEATURE_CURR_MIN_ALARM,
> > SENSORS_SUBFEATURE_CURR_MAX_ALARM,
> > Index: lib/sysfs.c
> > =================================> > --- lib/sysfs.c (revision 5996)
> > +++ lib/sysfs.c (working copy)
> > @@ -233,6 +233,9 @@
> > { "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
> > { "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
> > { "emergency_hyst", SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST },
> > + { "average", SENSORS_SUBFEATURE_TEMP_AVERAGE },
> > + { "lowest", SENSORS_SUBFEATURE_TEMP_LOWEST },
> > + { "highest", SENSORS_SUBFEATURE_TEMP_HIGHEST },
> > { "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },
> > @@ -252,6 +255,9 @@
> > { "max", SENSORS_SUBFEATURE_IN_MAX },
> > { "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
> > { "crit", SENSORS_SUBFEATURE_IN_CRIT },
> > + { "average", SENSORS_SUBFEATURE_IN_AVERAGE },
> > + { "lowest", SENSORS_SUBFEATURE_IN_LOWEST },
> > + { "highest", SENSORS_SUBFEATURE_IN_HIGHEST },
> > { "alarm", SENSORS_SUBFEATURE_IN_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },
> > @@ -302,6 +308,9 @@
> > { "max", SENSORS_SUBFEATURE_CURR_MAX },
> > { "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
> > { "crit", SENSORS_SUBFEATURE_CURR_CRIT },
> > + { "average", SENSORS_SUBFEATURE_CURR_AVERAGE },
> > + { "lowest", SENSORS_SUBFEATURE_CURR_LOWEST },
> > + { "highest", SENSORS_SUBFEATURE_CURR_HIGHEST },
> > { "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },
> > Index: prog/sensors/chips.c
> > =================================> > --- prog/sensors/chips.c (revision 5996)
> > +++ prog/sensors/chips.c (working copy)
> > @@ -280,15 +280,26 @@
> > { SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, 0, "crit" },
> > { SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, 0,
> > "emerg" },
> > + { SENSORS_SUBFEATURE_TEMP_AVERAGE, NULL, 0, "avg" },
> > + { SENSORS_SUBFEATURE_TEMP_LOWEST, NULL, 0, "lowest" },
> > + { SENSORS_SUBFEATURE_TEMP_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_TEMP_ALARMS 6
> > +#define NUM_TEMP_SENSORS (ARRAY_SIZE(temp_sensors) \
> > + + ARRAY_SIZE(temp_max_sensors) \
> > + + ARRAY_SIZE(temp_crit_sensors) \
> > + + ARRAY_SIZE(temp_emergency_sensors) \
> > + - NUM_TEMP_ALARMS)
> > +
> > +
> > static void print_chip_temp(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > - struct sensor_subfeature_data sensors[8];
> > - struct sensor_subfeature_data alarms[5];
> > + struct sensor_subfeature_data sensors[NUM_TEMP_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_TEMP_ALARMS];
> > int sensor_count, alarm_count;
> > const sensors_subfeature *sf;
> > double val;
> > @@ -365,17 +376,23 @@
> > { SENSORS_SUBFEATURE_IN_MIN, NULL, 0, "min" },
> > { SENSORS_SUBFEATURE_IN_MAX, NULL, 0, "max" },
> > { SENSORS_SUBFEATURE_IN_CRIT, NULL, 0, "crit max" },
> > + { SENSORS_SUBFEATURE_IN_AVERAGE, NULL, 0, "avg" },
> > + { SENSORS_SUBFEATURE_IN_LOWEST, NULL, 0, "lowest" },
> > + { SENSORS_SUBFEATURE_IN_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_IN_ALARMS 5
> > +#define NUM_IN_SENSORS (ARRAY_SIZE(voltage_sensors) - NUM_IN_ALARMS)
> > +
> > static void print_chip_in(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > const sensors_subfeature *sf;
> > char *label;
> > - struct sensor_subfeature_data sensors[4];
> > - struct sensor_subfeature_data alarms[4];
> > + struct sensor_subfeature_data sensors[NUM_IN_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_IN_ALARMS];
> > int sensor_count, alarm_count;
> > double val;
> >
> > @@ -504,6 +521,7 @@
> > };
> >
> > static const struct sensor_subfeature_list power_inst_sensors[] = {
> > + { SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },
>
> I'm confused. This one was missing on purpose. For power sensors we
> consider instantaneous and averaging sensors as different types. The
> code in function print_chip_power is pretty clear about this. So, just
> as all _INPUT subfeatures aren't listed in limit arrays,
> SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.
>
LM25066 reports instantaneous, average, and peak power, and I wanted to be able
to display it all.
> > { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" },
> > { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > @@ -517,14 +535,20 @@
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_POWER_ALARMS 4
> > +#define NUM_POWER_SENSORS (ARRAY_SIZE(power_common_sensors) \
> > + + ARRAY_SIZE(power_inst_sensors) \
> > + + ARRAY_SIZE(power_avg_sensors) \
> > + - NUM_POWER_ALARMS)
>
> Due to the specificity of the power sensors described above, the
> computed value is larger than needed. We'll use power_inst_sensors or
> power_avg_sensors but never both. You could define a MAX() macro to
> select the right one for to compute NUM_POWER_SENSORS.
>
Yes, I know, thus the comment at the very beginning.
> If you want to change the code to display both types, this can be
> discussed, but please keep the definition of NUM_POWER_SENSORS in line
> with the code that uses it, for clarity.
>
> > +
> > static void print_chip_power(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > double val;
> > const sensors_subfeature *sf;
> > - struct sensor_subfeature_data sensors[6];
> > - struct sensor_subfeature_data alarms[3];
> > + struct sensor_subfeature_data sensors[NUM_POWER_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_POWER_ALARMS];
> > int sensor_count, alarm_count;
> > char *label;
> > const char *unit;
> > @@ -653,9 +677,15 @@
> > { SENSORS_SUBFEATURE_CURR_MIN, NULL, 0, "min" },
> > { SENSORS_SUBFEATURE_CURR_MAX, NULL, 0, "max" },
> > { SENSORS_SUBFEATURE_CURR_CRIT, NULL, 0, "crit max" },
> > + { SENSORS_SUBFEATURE_CURR_AVERAGE, NULL, 0, "avg" },
> > + { SENSORS_SUBFEATURE_CURR_LOWEST, NULL, 0, "lowest" },
> > + { SENSORS_SUBFEATURE_CURR_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_CURR_ALARM 5
> > +#define NUM_CURR_SENSORS (ARRAY_SIZE(current_sensors) - NUM_CURR_ALARM)
> > +
> > static void print_chip_curr(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > @@ -663,8 +693,8 @@
> > const sensors_subfeature *sf;
> > double val;
> > char *label;
> > - struct sensor_subfeature_data sensors[4];
> > - struct sensor_subfeature_data alarms[4];
> > + struct sensor_subfeature_data sensors[NUM_CURR_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_CURR_ALARM];
> > int sensor_count, alarm_count;
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > Index: CHANGES
> > =================================> > --- CHANGES (revision 5996)
> > +++ CHANGES (working copy)
> > @@ -2,6 +2,10 @@
> > -----------------------
> >
> > SVN HEAD
> > + libsensors: Added support for new sysfs attributes
> > + sensors: Added support for new sysfs attributes
> > + For power sensors, display both instantaneous and average data
> > + if available
>
> Your patch doesn't actually implement that second change. Which is
> good, BTW, this unrelated change would really belong to a separate
> patch.
>
Hmm - I thought it did. I'll take it out for now and have another look.
Thanks a lot for the review.
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] Add support for new attributes to
2011-11-06 22:46 [lm-sensors] [PATCH v2] Add support for new attributes to Guenter Roeck
2012-01-08 21:45 ` Jean Delvare
2012-01-08 22:20 ` Guenter Roeck
@ 2012-01-09 0:14 ` Guenter Roeck
2012-01-09 16:23 ` Jean Delvare
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-01-09 0:14 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On Sun, Jan 08, 2012 at 04:45:02PM -0500, Jean Delvare wrote:
> Hi Guenter,
>
[ ... ]
> >
> > static const struct sensor_subfeature_list power_inst_sensors[] = {
> > + { SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },
>
> I'm confused. This one was missing on purpose. For power sensors we
> consider instantaneous and averaging sensors as different types. The
> code in function print_chip_power is pretty clear about this. So, just
> as all _INPUT subfeatures aren't listed in limit arrays,
> SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.
>
I looked this up again.
The idea was to support display of both instantaneous power and average power
if both is provided by the chip/driver.
With the above, this is displayed as follows:
power: 55mW (avg: 50mW, highest: 70mW)
In other words, average power is displayed as limit, similar to lowest/highest values,
if instantaneous power is supported as well. If not, it is still displayed as before.
power: 50mW (highest: 70mW)
Do you have a better idea how to handle this case ?
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] Add support for new attributes to
2011-11-06 22:46 [lm-sensors] [PATCH v2] Add support for new attributes to Guenter Roeck
` (2 preceding siblings ...)
2012-01-09 0:14 ` Guenter Roeck
@ 2012-01-09 16:23 ` Jean Delvare
2012-01-09 16:40 ` Guenter Roeck
2012-01-09 17:16 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2012-01-09 16:23 UTC (permalink / raw)
To: lm-sensors
On Sun, 8 Jan 2012 16:14:27 -0800, Guenter Roeck wrote:
> Hi Jean,
>
> On Sun, Jan 08, 2012 at 04:45:02PM -0500, Jean Delvare wrote:
> > Hi Guenter,
> >
> [ ... ]
> > >
> > > static const struct sensor_subfeature_list power_inst_sensors[] = {
> > > + { SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },
> >
> > I'm confused. This one was missing on purpose. For power sensors we
> > consider instantaneous and averaging sensors as different types. The
> > code in function print_chip_power is pretty clear about this. So, just
> > as all _INPUT subfeatures aren't listed in limit arrays,
> > SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.
> >
> I looked this up again.
>
> The idea was to support display of both instantaneous power and average power
> if both is provided by the chip/driver.
>
> With the above, this is displayed as follows:
>
> power: 55mW (avg: 50mW, highest: 70mW)
>
> In other words, average power is displayed as limit, similar to lowest/highest values,
> if instantaneous power is supported as well. If not, it is still displayed as before.
>
> power: 50mW (highest: 70mW)
>
> Do you have a better idea how to handle this case ?
Oh, this is pretty nice. Now this all makes sense, thanks for the
clarification.
However it looks like a specific hack for a specific problem, rather
than a generic solution. We have two sets of highest/lowest power
attributes, one for instant measurements and one for averaged
measurements. While your change makes it possible to display both the
instant and averaged value, it can only display one set of
highest/lowest limit. I supposed that a monitoring device could expose
both, even if yours does not.
So with this change you can't claim that you display all power
attributes. Rather, you special-cased SENSORS_SUBFEATURE_POWER_AVERAGE
to be either an input value or a limit value. This is certainly
acceptable as long as it is properly documented. But this makes me
wonder if it wouldn't be better to assume that all attributes may be
present, and input may be missing in which case we use average as the
measurement.
Well that was a long speech for a short conclusion: as far as I can
see, all we have to do is duplicate power_avg_sensors[] at the end of
power_inst_sensors[] and adjust the labels to avoid ambiguous
duplicates. As far as I can see you don't even need to touch the code
itself, except for stripping out the comment which claims that power
sensor flavors are assumed to be mutually exclusive.
--
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] Add support for new attributes to
2011-11-06 22:46 [lm-sensors] [PATCH v2] Add support for new attributes to Guenter Roeck
` (3 preceding siblings ...)
2012-01-09 16:23 ` Jean Delvare
@ 2012-01-09 16:40 ` Guenter Roeck
2012-01-09 17:16 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-01-09 16:40 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On Mon, 2012-01-09 at 11:23 -0500, Jean Delvare wrote:
> On Sun, 8 Jan 2012 16:14:27 -0800, Guenter Roeck wrote:
> > Hi Jean,
> >
> > On Sun, Jan 08, 2012 at 04:45:02PM -0500, Jean Delvare wrote:
> > > Hi Guenter,
> > >
> > [ ... ]
> > > >
> > > > static const struct sensor_subfeature_list power_inst_sensors[] = {
> > > > + { SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },
> > >
> > > I'm confused. This one was missing on purpose. For power sensors we
> > > consider instantaneous and averaging sensors as different types. The
> > > code in function print_chip_power is pretty clear about this. So, just
> > > as all _INPUT subfeatures aren't listed in limit arrays,
> > > SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.
> > >
> > I looked this up again.
> >
> > The idea was to support display of both instantaneous power and average power
> > if both is provided by the chip/driver.
> >
> > With the above, this is displayed as follows:
> >
> > power: 55mW (avg: 50mW, highest: 70mW)
> >
> > In other words, average power is displayed as limit, similar to lowest/highest values,
> > if instantaneous power is supported as well. If not, it is still displayed as before.
> >
> > power: 50mW (highest: 70mW)
> >
> > Do you have a better idea how to handle this case ?
>
> Oh, this is pretty nice. Now this all makes sense, thanks for the
> clarification.
>
> However it looks like a specific hack for a specific problem, rather
> than a generic solution. We have two sets of highest/lowest power
> attributes, one for instant measurements and one for averaged
> measurements. While your change makes it possible to display both the
> instant and averaged value, it can only display one set of
> highest/lowest limit. I supposed that a monitoring device could expose
> both, even if yours does not.
>
I tend to call that kind of thing "minimal solution" ;).
> So with this change you can't claim that you display all power
> attributes. Rather, you special-cased SENSORS_SUBFEATURE_POWER_AVERAGE
Good point. Maybe "all power attributes known to exist in parallel".
> to be either an input value or a limit value. This is certainly
> acceptable as long as it is properly documented. But this makes me
> wonder if it wouldn't be better to assume that all attributes may be
> present, and input may be missing in which case we use average as the
> measurement.
>
> Well that was a long speech for a short conclusion: as far as I can
> see, all we have to do is duplicate power_avg_sensors[] at the end of
> power_inst_sensors[] and adjust the labels to avoid ambiguous
> duplicates. As far as I can see you don't even need to touch the code
> itself, except for stripping out the comment which claims that power
> sensor flavors are assumed to be mutually exclusive.
>
Makes sense. Something like the following ?
static const struct sensor_subfeature_list power_avg_sensors[] = {
{ SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, 0,
"avg_lowest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, 0,
"avg_highest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, 0,
"interval" },
{ -1, NULL, 0, NULL }
};
static const struct sensor_subfeature_list power_inst_sensors[] = {
{ SENSORS_SUBFEATURE_POWER_AVERAGE, power_avg_sensors, 0,
"avg" },
{ SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" },
{ SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" },
{ -1, NULL, 0, NULL }
};
Any good idea how to name the labels ?
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] Add support for new attributes to
2011-11-06 22:46 [lm-sensors] [PATCH v2] Add support for new attributes to Guenter Roeck
` (4 preceding siblings ...)
2012-01-09 16:40 ` Guenter Roeck
@ 2012-01-09 17:16 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2012-01-09 17:16 UTC (permalink / raw)
To: lm-sensors
On Mon, 9 Jan 2012 08:40:31 -0800, Guenter Roeck wrote:
> On Mon, 2012-01-09 at 11:23 -0500, Jean Delvare wrote:
> > Well that was a long speech for a short conclusion: as far as I can
> > see, all we have to do is duplicate power_avg_sensors[] at the end of
> > power_inst_sensors[] and adjust the labels to avoid ambiguous
> > duplicates. As far as I can see you don't even need to touch the code
> > itself, except for stripping out the comment which claims that power
> > sensor flavors are assumed to be mutually exclusive.
>
> Makes sense. Something like the following ?
>
> static const struct sensor_subfeature_list power_avg_sensors[] = {
> { SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, 0,
> "avg_lowest" },
> { SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, 0,
> "avg_highest" },
> { SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, 0,
> "interval" },
> { -1, NULL, 0, NULL }
> };
>
> static const struct sensor_subfeature_list power_inst_sensors[] = {
> { SENSORS_SUBFEATURE_POWER_AVERAGE, power_avg_sensors, 0,
> "avg" },
> { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" },
> { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" },
> { -1, NULL, 0, NULL }
> };
>
> Any good idea how to name the labels ?
Well, currently we simply use "lowest" and "highest" when a given
sensor only supports one flavor. I think we want to keep things that
way, as this avoids long/confusing labels in the most common cases. I
expect the multi-flavor sensors case with both sets of limits to be
rare.
Of course the drawback is that we can't reuse power_avg_sensors. But
I'd say this is a fairly minor cost. So what I had in mind was more
along the lines of:
static const struct sensor_subfeature_list power_inst_sensors[] = {
{ SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" },
{ SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE, power_avg_sensors, 0, "avg" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, 0, "avg lowest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, 0, "avg highest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, 0,
"interval" },
{ -1, NULL, 0, NULL }
};
static const struct sensor_subfeature_list power_avg_sensors[] = {
{ SENSORS_SUBFEATURE_POWER_AVERAGE_LOWEST, NULL, 0, "lowest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_HIGHEST, NULL, 0, "highest" },
{ SENSORS_SUBFEATURE_POWER_AVERAGE_INTERVAL, NULL, 0,
"interval" },
{ -1, NULL, 0, NULL }
};
I.e. power_avg_sensors is unchanged.
Note that I avoided underscores in labels on purpose and for
consistency, as we don't have any currently.
--
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