[lm-sensors] dynamic chip support in libsensors + generic chip

[lm-sensors] dynamic chip support in libsensors + generic chip

[Hans de Goede]

Hi all,

I've committed everything my students had written + some fixes done by me after 
some initial testing to the 3.0.0 sensors branch.

I claim in no way that this code is perfect, I did things this way to properly 
document / atrribute who wrote what, and to give us a starting point as I think 
the code in general is ok. It needs testing and cleanups, but its not a bad 
starting point perse.

So please test this:
1) checkout the 3.0.0 branch:
    svn checkout http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0
2) comment the entries for your chips int he table at the end of lib/chips.c
    and in the table near the end of progs/sensors/main.c
3) compile and install as usual
4) run sensors and let me/us know how it works

Regards,

Hans


p.s.

I'm planning on doing several cleanups of the code as submitted the coming few 
days, as time permits. Plans so far:
-cleanup the regex used to see if a sysfs entry actual is a sysfs sensor file,
  so that it doesn't match everything (including entries like: uevent, modalias,
  etc), but only matches the actual known / document sysfs sensor files
-cleanup the new sensors_feature_get_type to be much more robust / do much
  exacter matching

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Bob Schlärmann]

On Mon, 09 Apr 2007 17:20:52 +0200
Hans de Goede <j.w.r.degoede@hhs.nl> wrote:

> ...
> So please test this:
> 1) checkout the 3.0.0 branch:
>     svn checkout
> http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0 2)
> comment the entries for your chips int he table at the end of
> lib/chips.c and in the table near the end of progs/sensors/main.c 3)
Note that this isn't needed if you give sensors the -g argument. This
forces sensors to use the generic printing routines.

> compile and install as usual 4) run sensors and let me/us know how it
> works
> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> I'm planning on doing several cleanups of the code as submitted the
> coming few days, as time permits. Plans so far:
> -cleanup the regex used to see if a sysfs entry actual is a sysfs
> sensor file, so that it doesn't match everything (including entries
> like: uevent, modalias, etc), but only matches the actual known /
> document sysfs sensor files -cleanup the new sensors_feature_get_type
> to be much more robust / do much exacter matching
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> 
> !DSPAM:461a569c75901200214150!
> 

--
Bob Schlärmann

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Hans de Goede]

Bob Schlärmann wrote:
> On Mon, 09 Apr 2007 17:20:52 +0200
> Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> 
>> ...
>> So please test this:
>> 1) checkout the 3.0.0 branch:
>>     svn checkout
>> http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0 2)
>> comment the entries for your chips int he table at the end of
>> lib/chips.c and in the table near the end of progs/sensors/main.c 3)
> Note that this isn't needed if you give sensors the -g argument. This
> forces sensors to use the generic printing routines.
> 

Yes, the removal from progs/sensors/main.c isn't really necessary to test the 
generic printing routines, that can be done with -g too. However the removal / 
commenting from lib/chips.c _really_ is necessary to test the dynamic chip support.

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Jean Delvare]

Hi Bob

On Mon, 9 Apr 2007 20:31:11 +0200, Bob Schlärmann wrote:
> On Mon, 09 Apr 2007 17:20:52 +0200
> Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> 
> > So please test this:
> > 1) checkout the 3.0.0 branch:
> >     svn checkout
> > http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0
> > 2) comment the entries for your chips int he table at the end of
> > lib/chips.c and in the table near the end of progs/sensors/main.c
>
> Note that this isn't needed if you give sensors the -g argument. This
> forces sensors to use the generic printing routines.

How does "sensors -g" differ from "sensors -u"? I thought we would be
reusing -u for the generic printing.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Bob Schlärmann]

On Wed, 11 Apr 2007 14:00:20 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> Hi Bob
> 
> On Mon, 9 Apr 2007 20:31:11 +0200, Bob Schlärmann wrote:
> > On Mon, 09 Apr 2007 17:20:52 +0200
> > Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> > 
> > > So please test this:
> > > 1) checkout the 3.0.0 branch:
> > >     svn checkout
> > > http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0
> > > 2) comment the entries for your chips int he table at the end of
> > > lib/chips.c and in the table near the end of progs/sensors/main.c
> >
> > Note that this isn't needed if you give sensors the -g argument.
> > This forces sensors to use the generic printing routines.
> 
> How does "sensors -g" differ from "sensors -u"? I thought we would be
> reusing -u for the generic printing.
> 
The difference is that -u prints every feature, -g only prints feature
types it knows about. So when adding some new special feature type, the
generic printing routines won't show them (yet) but -u will. 

Don't know whether it's worth it to support this extra switch, but I 
wanted to change the behaviour of sensors as little as possible.

--
Bob Schlärmann

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Jean Delvare]

Hi Hans,

Better late than never...

On Mon, 09 Apr 2007 17:20:52 +0200, Hans de Goede wrote:
> I've committed everything my students had written + some fixes done by me after 
> some initial testing to the 3.0.0 sensors branch.
> 
> I claim in no way that this code is perfect, I did things this way to properly 
> document / atrribute who wrote what, and to give us a starting point as I think 
> the code in general is ok. It needs testing and cleanups, but its not a bad 
> starting point perse.
> 
> So please test this:
> 1) checkout the 3.0.0 branch:
>     svn checkout http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0
> 2) comment the entries for your chips int he table at the end of lib/chips.c
>     and in the table near the end of progs/sensors/main.c
> 3) compile and install as usual
> 4) run sensors and let me/us know how it works

I've tried that with my ADM1032 today, and this uncovered two problems.

First problem is the way the new library (and sensors) code handles
temperature hysteresis. It handles it as a "standard" temperature
limit, which it isn't. An hysteresis value is more like a property
of other temperature limit. So having a type named
SENSORS_FEATURE_TEMP_HYST doesn't make sense. We need, instead,
SENSORS_FEATURE_TEMP_MAX_HYST, and SENSORS_FEATURE_TEMP_CRIT_HYST.

Second problem is that the new code doesn't fill the magnitude when
creating the dynamic feature tables. It just happens to work in most
cases thanks to the general symbol name translation code, which
enforces standard magnitudes as a side effect. This doesn't work when
we had a dedicated symbol name translation rule for a given chip though
(which was the case for the hysteresis value of the lm90/adm1032)
because this rule is part of the chip-specific features table which we
drop when switching to the generic code. And the general symbol name
translation is bound to be deleted at some point too, so we shouldn't
rely on it.

Here is the patch I have come up with, which fixes both problems and
makes the generic code work for my ADM1032. Can you please review it and
confirm that it doesn't break anything on your side? Thanks.

Index: lib/sensors.h
=================================--- lib/sensors.h	(révision 4405)
+++ lib/sensors.h	(copie de travail)
@@ -166,14 +166,16 @@
   SENSORS_FEATURE_FAN_DIV,
   
   SENSORS_FEATURE_TEMP = 0x200,
-  SENSORS_FEATURE_TEMP_HYST,
   SENSORS_FEATURE_TEMP_OVER,
   SENSORS_FEATURE_TEMP_MAX,
+  SENSORS_FEATURE_TEMP_MAX_HYST,
   SENSORS_FEATURE_TEMP_MIN,
+  SENSORS_FEATURE_TEMP_MIN_HYST,
   SENSORS_FEATURE_TEMP_HIGH,
   SENSORS_FEATURE_TEMP_LOW,
   SENSORS_FEATURE_TEMP_LIM,
   SENSORS_FEATURE_TEMP_CRIT,
+  SENSORS_FEATURE_TEMP_CRIT_HYST,
   SENSORS_FEATURE_TEMP_ALARM = 0x210,
   SENSORS_FEATURE_TEMP_FAULT,
   SENSORS_FEATURE_TEMP_SENS,
Index: lib/access.c
=================================--- lib/access.c	(révision 4405)
+++ lib/access.c	(copie de travail)
@@ -515,12 +515,14 @@
 };
 
 static struct feature_type_match temp_matches[] = {
-	{ "hyst", SENSORS_FEATURE_TEMP_HYST },
 	{ "over", SENSORS_FEATURE_TEMP_OVER },
 	{ "max", SENSORS_FEATURE_TEMP_MAX },
+	{ "max_hyst", SENSORS_FEATURE_TEMP_MAX_HYST },
 	{ "min", SENSORS_FEATURE_TEMP_MIN },
+	{ "min_hyst", SENSORS_FEATURE_TEMP_MIN_HYST },
 	{ "low", SENSORS_FEATURE_TEMP_LOW },
 	{ "crit", SENSORS_FEATURE_TEMP_CRIT },
+	{ "crit_hyst", SENSORS_FEATURE_TEMP_CRIT_HYST },
 	{ "alarm", SENSORS_FEATURE_TEMP_ALARM },
 	{ "fault", SENSORS_FEATURE_TEMP_FAULT },
 	{ "type", SENSORS_FEATURE_TEMP_SENS },
Index: lib/sysfs.c
=================================--- lib/sysfs.c	(révision 4405)
+++ lib/sysfs.c	(copie de travail)
@@ -37,6 +37,29 @@
 
 #define MAX_SENSORS_PER_TYPE 16
 
+static
+int get_type_scaling(int type)
+{
+	switch (type & 0xFF10) {
+	case SENSORS_FEATURE_IN:
+	case SENSORS_FEATURE_TEMP:
+		return 3;
+	case SENSORS_FEATURE_FAN:
+		return 0;
+	}
+
+	switch (type) {
+	case SENSORS_FEATURE_VID:
+		return 3;
+	case SENSORS_FEATURE_VRM:
+		return 1;
+	default:
+		return 0;
+	}
+
+	return 0;
+}
+
 static 
 sensors_chip_features sensors_read_dynamic_chip(struct sysfs_device *sysdir)
 {
@@ -149,6 +172,8 @@
 			SENSORS_MODE_R : (attr->method & SYSFS_METHOD_STORE) ?
 			SENSORS_MODE_W : SENSORS_MODE_NO_RW;
 
+		feature.scaling = get_type_scaling(type);
+
 		features[i] = feature;
 		fnum++;
 	}
Index: prog/sensors/chips_generic.c
=================================--- prog/sensors/chips_generic.c	(révision 4405)
+++ prog/sensors/chips_generic.c	(copie de travail)
@@ -144,27 +144,16 @@
       } else {
         type = MINMAX;
       }
+    } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX_HYST)) {
+      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX_HYST);
+      type = HYST;
     } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT)) {
       min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_CRIT);
       type = CRIT;
-    } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_HYST)) {
-      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_HYST);
-      type = HYST;
     } else {
       min = 0;
       type = MAXONLY;
     }
-  } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_HYST)) {
-    min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_HYST);
-    
-    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_OVER)) {
-      max = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_OVER);
-      type = HYST;
-    } else {
-      max = min;
-      max = 0;
-      type = HYSTONLY;
-    }
   } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_LOW)) {
     min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_LOW);
     
@@ -214,18 +203,14 @@
   }
   printf("\n");
   
-  if (type = MINMAX && TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT))
+  if (type != CRIT && TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT))
   {
     const sensors_feature_data *subfeature;
     max = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_CRIT);
     
-    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_HYST)) {
-      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_HYST);
+    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT_HYST)) {
+      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_CRIT_HYST);
       type = HYSTONLY;
-    } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX) && 
-        !TEMP_FEATURE(SENSORS_FEATURE_TEMP_MIN)) {
-      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX);
-      type = HYSTONLY;
     } else {
       type = SINGLE;
       min = 0.0;
@@ -239,7 +224,7 @@
       if (valid) {
         print_label(label, label_size);
         free(label);
-        print_temp_info_real(max, min, 0, 0.0, type, 1, 0);
+        print_temp_info_real(max, min, 0, 0.0, type, 1, 1);
         printf("\n");
       }
     }


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Jean Delvare]

On Fri, 25 May 2007 15:47:50 +0200, Jean Delvare wrote:
> Here is the patch I have come up with, which fixes both problems and
> makes the generic code work for my ADM1032. Can you please review it and
> confirm that it doesn't break anything on your side? Thanks.

I spoke a bit too fast. There was actually a third problem, alarms
weren't reported. This is because the ADM1032 has per-limit alarm flags
and the generic code only supported per-channel alarms for temperatures.

So here is a more complete patch which fixes all three problems.

Index: lib/sensors.h
=================================--- lib/sensors.h	(révision 4405)
+++ lib/sensors.h	(copie de travail)
@@ -166,15 +166,20 @@
   SENSORS_FEATURE_FAN_DIV,
   
   SENSORS_FEATURE_TEMP = 0x200,
-  SENSORS_FEATURE_TEMP_HYST,
   SENSORS_FEATURE_TEMP_OVER,
   SENSORS_FEATURE_TEMP_MAX,
+  SENSORS_FEATURE_TEMP_MAX_HYST,
   SENSORS_FEATURE_TEMP_MIN,
+  SENSORS_FEATURE_TEMP_MIN_HYST,
   SENSORS_FEATURE_TEMP_HIGH,
   SENSORS_FEATURE_TEMP_LOW,
   SENSORS_FEATURE_TEMP_LIM,
   SENSORS_FEATURE_TEMP_CRIT,
+  SENSORS_FEATURE_TEMP_CRIT_HYST,
   SENSORS_FEATURE_TEMP_ALARM = 0x210,
+  SENSORS_FEATURE_TEMP_MAX_ALARM,
+  SENSORS_FEATURE_TEMP_MIN_ALARM,
+  SENSORS_FEATURE_TEMP_CRIT_ALARM,
   SENSORS_FEATURE_TEMP_FAULT,
   SENSORS_FEATURE_TEMP_SENS,
   
@@ -185,7 +190,7 @@
   
   /* special the largest number of subfeatures used, iow the 
      highest ## from all the 0x?## above + 1*/
-  SENSORS_FEATURE_MAX_SUB_FEATURES = 19
+  SENSORS_FEATURE_MAX_SUB_FEATURES = 22
 } sensors_feature_type;
 
 sensors_feature_type sensors_feature_get_type
Index: lib/access.c
=================================--- lib/access.c	(révision 4405)
+++ lib/access.c	(copie de travail)
@@ -515,13 +515,18 @@
 };
 
 static struct feature_type_match temp_matches[] = {
-	{ "hyst", SENSORS_FEATURE_TEMP_HYST },
 	{ "over", SENSORS_FEATURE_TEMP_OVER },
 	{ "max", SENSORS_FEATURE_TEMP_MAX },
+	{ "max_hyst", SENSORS_FEATURE_TEMP_MAX_HYST },
 	{ "min", SENSORS_FEATURE_TEMP_MIN },
+	{ "min_hyst", SENSORS_FEATURE_TEMP_MIN_HYST },
 	{ "low", SENSORS_FEATURE_TEMP_LOW },
 	{ "crit", SENSORS_FEATURE_TEMP_CRIT },
+	{ "crit_hyst", SENSORS_FEATURE_TEMP_CRIT_HYST },
 	{ "alarm", SENSORS_FEATURE_TEMP_ALARM },
+	{ "min_alarm", SENSORS_FEATURE_TEMP_MIN_ALARM },
+	{ "max_alarm", SENSORS_FEATURE_TEMP_MAX_ALARM },
+	{ "crit_alarm", SENSORS_FEATURE_TEMP_CRIT_ALARM },
 	{ "fault", SENSORS_FEATURE_TEMP_FAULT },
 	{ "type", SENSORS_FEATURE_TEMP_SENS },
 	{ 0 }
Index: lib/sysfs.c
=================================--- lib/sysfs.c	(révision 4405)
+++ lib/sysfs.c	(copie de travail)
@@ -37,6 +37,29 @@
 
 #define MAX_SENSORS_PER_TYPE 16
 
+static
+int get_type_scaling(int type)
+{
+	switch (type & 0xFF10) {
+	case SENSORS_FEATURE_IN:
+	case SENSORS_FEATURE_TEMP:
+		return 3;
+	case SENSORS_FEATURE_FAN:
+		return 0;
+	}
+
+	switch (type) {
+	case SENSORS_FEATURE_VID:
+		return 3;
+	case SENSORS_FEATURE_VRM:
+		return 1;
+	default:
+		return 0;
+	}
+
+	return 0;
+}
+
 static 
 sensors_chip_features sensors_read_dynamic_chip(struct sysfs_device *sysdir)
 {
@@ -149,6 +172,8 @@
 			SENSORS_MODE_R : (attr->method & SYSFS_METHOD_STORE) ?
 			SENSORS_MODE_W : SENSORS_MODE_NO_RW;
 
+		feature.scaling = get_type_scaling(type);
+
 		features[i] = feature;
 		fnum++;
 	}
Index: prog/sensors/chips_generic.c
=================================--- prog/sensors/chips_generic.c	(révision 4405)
+++ prog/sensors/chips_generic.c	(copie de travail)
@@ -144,27 +144,16 @@
       } else {
         type = MINMAX;
       }
+    } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX_HYST)) {
+      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX_HYST);
+      type = HYST;
     } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT)) {
       min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_CRIT);
       type = CRIT;
-    } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_HYST)) {
-      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_HYST);
-      type = HYST;
     } else {
       min = 0;
       type = MAXONLY;
     }
-  } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_HYST)) {
-    min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_HYST);
-    
-    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_OVER)) {
-      max = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_OVER);
-      type = HYST;
-    } else {
-      max = min;
-      max = 0;
-      type = HYSTONLY;
-    }
   } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_LOW)) {
     min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_LOW);
     
@@ -208,24 +197,27 @@
   if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_FAULT) &&
       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_FAULT) > 0.5) {
     printf(" FAULT");
-  } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
-      TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5) {
+  } else
+  if ((TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
+       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5)
+   || (type = MINMAX &&
+       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MIN_ALARM) && 
+       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MIN_ALARM) > 0.5)
+   || (type = MINMAX &&
+       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX_ALARM) && 
+       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX_ALARM) > 0.5)) {
     printf(" ALARM");
   }
   printf("\n");
   
-  if (type = MINMAX && TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT))
+  if (type != CRIT && TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT))
   {
     const sensors_feature_data *subfeature;
     max = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_CRIT);
     
-    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_HYST)) {
-      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_HYST);
+    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT_HYST)) {
+      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_CRIT_HYST);
       type = HYSTONLY;
-    } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX) && 
-        !TEMP_FEATURE(SENSORS_FEATURE_TEMP_MIN)) {
-      min = TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX);
-      type = HYSTONLY;
     } else {
       type = SINGLE;
       min = 0.0;
@@ -239,7 +231,11 @@
       if (valid) {
         print_label(label, label_size);
         free(label);
-        print_temp_info_real(max, min, 0, 0.0, type, 1, 0);
+        print_temp_info_real(max, min, 0, 0.0, type, 1, 1);
+        if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_CRIT_ALARM) &&
+            TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_CRIT_ALARM) > 0.5) {
+          printf(" ALARM");
+        }
         printf("\n");
       }
     }

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Hans de Goede]

Jean Delvare wrote:
> On Fri, 25 May 2007 15:47:50 +0200, Jean Delvare wrote:
>> Here is the patch I have come up with, which fixes both problems and
>> makes the generic code work for my ADM1032. Can you please review it and
>> confirm that it doesn't break anything on your side? Thanks.
> 
> I spoke a bit too fast. There was actually a third problem, alarms
> weren't reported. This is because the ADM1032 has per-limit alarm flags
> and the generic code only supported per-channel alarms for temperatures.
> 
> So here is a more complete patch which fixes all three problems.
> 

First of all many thanks for looking add this and trying it, that is just what 
this code needs, much testing. Or some testing on many different kinds of 
hardware to be even more clear.

I've reviewed your patch here are some notes:


> Index: prog/sensors/chips_generic.c
> =================================> --- prog/sensors/chips_generic.c	(révision 4405)
> +++ prog/sensors/chips_generic.c	(copie de travail)

you add a SENSORS_FEATURE_TEMP_MIN_HYST sensor type, but do not check / do 
anything with it in print_generic_chip_temp()

> @@ -208,24 +197,27 @@
>    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_FAULT) &&
>        TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_FAULT) > 0.5) {
>      printf(" FAULT");
> -  } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
> -      TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5) {
> +  } else
> +  if ((TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5)
> +   || (type = MINMAX &&
> +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MIN_ALARM) && 
> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MIN_ALARM) > 0.5)
> +   || (type = MINMAX &&
> +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX_ALARM) && 
> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX_ALARM) > 0.5)) {
>      printf(" ALARM");
>    }
>    printf("\n");
>    

-Is the type = MINMAX check for printing the alarms necessary, doesn't this 
has the risk of not showing an alarm for some chip with a funky combination of 
temp sysfs entries?

-Shouldn't the code somehow show the difference between a min and a max (and a 
generic) alarm? See how print_generic_chip_in() does this

---

I also tested lm_sensors 3.0.0 with your patch and it still works fine on my 
abit uguru motherboard

Some generic notes not directly related to your patch:

This piece of code in print_generic_chip_temp() can be done much simpler:

   if (type = LIM) {
   } else {
     print_temp_info_real(val, max, min, 0.0, type, 1, 1);
   }

Since lim always is 0.0 (this initalized to this) in the non LIM case, this can 
be written as just:
   print_temp_info_real(val, max, min, lim, type, 1, 1);

Will you change this or shall I?

More in general print_generic_chip_temp(), needs a good review by someone who 
is well aware of all possible tempX sysfs entry combo's (iow not by me).


Also print_generic_chip_in(), has some dangerous and unneeded assumptions, for 
example:
-it assumes that if there is a inX_max that there will also always be a inX_min
-it assumes that if there is no inX_max there will also be no alarms

Shall I fix these or will you?

Somewhat less "unneeded" assumption
-it doesn't check for inX_max_alarm without an inX_min_alarm and visa versa, I
  admit this would be a driver bug, as then the driver should have just a
  inX_alarm, but still we might want to concider this.


Well thats about it. About all the "Shall I fix these or will you?", that is 
not me being lazing, but rather me trying to avoid commit conflicts. Its fine 
if you want me to take care of them, but then I guess I should wait until your 
patch is in svn.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Jean Delvare]

Hi Hans,

On Fri, 25 May 2007 22:10:20 +0200, Hans de Goede wrote:
> I've reviewed your patch here are some notes:
> 
> 
> > Index: prog/sensors/chips_generic.c
> > =================================> > --- prog/sensors/chips_generic.c	(révision 4405)
> > +++ prog/sensors/chips_generic.c	(copie de travail)
> 
> you add a SENSORS_FEATURE_TEMP_MIN_HYST sensor type, but do not check / do 
> anything with it in print_generic_chip_temp()


Good point. I though we had temp[1-*]_min_hyst defined in our standard
sysfs interface, but in fact we don't. So SENSORS_FEATURE_TEMP_MIN_HYST
isn't needed.

> > @@ -208,24 +197,27 @@
> >    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_FAULT) &&
> >        TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_FAULT) > 0.5) {
> >      printf(" FAULT");
> > -  } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
> > -      TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5) {
> > +  } else
> > +  if ((TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
> > +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5)
> > +   || (type = MINMAX &&
> > +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MIN_ALARM) && 
> > +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MIN_ALARM) > 0.5)
> > +   || (type = MINMAX &&
> > +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX_ALARM) && 
> > +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX_ALARM) > 0.5)) {
> >      printf(" ALARM");
> >    }
> >    printf("\n");
> >    
> 
> -Is the type = MINMAX check for printing the alarms necessary, doesn't this 
> has the risk of not showing an alarm for some chip with a funky combination of 
> temp sysfs entries?

This will need to be double-checked. Removing the check altogether has
the risk of printing alarms where they do not belong, which isn't
better. Temperatures are a bit tricky because we may have up to 5 limit
values to display and we only have 2 slots on the first line. So some
of the limits may move to a second line. And I think we want to display
the ALARM flag on the right line in this case. Or maybe you think the
ALARM flag should always be on the first line even if it corresponds to
a limit which is on the second line?

> -Shouldn't the code somehow show the difference between a min and a max (and a 
> generic) alarm? See how print_generic_chip_in() does this

Yes I know the voltage code does this, but the temperature code is much
more complex at the moment. I have no objection if you want to add this
feature, but let's first cleanup the code.

I see we have SENSORS_FEATURE_TEMP_OVER, SENSORS_FEATURE_TEMP_LOW and
SENSORS_FEATURE_TEMP_HIGH defined. What are these? We don't have names
for these in our standard sysfs interface, and they seem completely
redundant with SENSORS_FEATURE_TEMP_MAX and SENSORS_FEATURE_TEMP_MIN.
Did your students get confused by the non-standard names in the 2.4
driver somehow? It looks to me like we could plain delete these three
symbols.

> Some generic notes not directly related to your patch:
> 
> This piece of code in print_generic_chip_temp() can be done much simpler:
> 
>    if (type = LIM) {
>    } else {
>      print_temp_info_real(val, max, min, 0.0, type, 1, 1);
>    }
> 
> Since lim always is 0.0 (this initalized to this) in the non LIM case, this can 
> be written as just:
>    print_temp_info_real(val, max, min, lim, type, 1, 1);
> 
> Will you change this or shall I?

I would first need to be explained what this "LIM" thing is. It
doesn't correspond to anything in our standard sysfs interface as far
as I can see, and no legacy chip code was using it. I would get rid of
it.

> More in general print_generic_chip_temp(), needs a good review by someone who 
> is well aware of all possible tempX sysfs entry combo's (iow not by me).

We agree ;) see my comments above. If we get rid of the 4 unneeded
limits as I am suggesting, I'm certain things will be a lot easier. Of
course, even then, there are some decisions to make because we can have
many different combinations of temperature limits depending on the
chip, and we want to present them in the way that makes the more sense
each time. I will look into this after the cleanups are done.

> Also print_generic_chip_in(), has some dangerous and unneeded assumptions, for 
> example:
> -it assumes that if there is a inX_max that there will also always be a inX_min
> -it assumes that if there is no inX_max there will also be no alarms
> 
> Shall I fix these or will you?

The code shouldn't assume anything like that. Please fix.

> Somewhat less "unneeded" assumption
> -it doesn't check for inX_max_alarm without an inX_min_alarm and visa versa, I
>   admit this would be a driver bug, as then the driver should have just a
>   inX_alarm, but still we might want to concider this.

Your own assumption is wrong. A chip could have a voltage input with a
min limit and a max limit, and an alarm which is only raised by the max
limit. In this case, the driver would indeed have inX_max_alarm and not
inX_min_alarm. Not to say that this particular hardware design would be
smart, but if it existed, that's what how our driver would support it.
And we've seen such weird hardware designs already... So you could fix
this too.

> Well thats about it. About all the "Shall I fix these or will you?", that is 
> not me being lazing, but rather me trying to avoid commit conflicts. Its fine 
> if you want me to take care of them, but then I guess I should wait until your 
> patch is in svn.

I've committed my pending patches now, so if you could now fix the
problems you've seen, this would be welcome.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
>>> @@ -208,24 +197,27 @@
>>>    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_FAULT) &&
>>>        TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_FAULT) > 0.5) {
>>>      printf(" FAULT");
>>> -  } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
>>> -      TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5) {
>>> +  } else
>>> +  if ((TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
>>> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5)
>>> +   || (type = MINMAX &&
>>> +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MIN_ALARM) && 
>>> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MIN_ALARM) > 0.5)
>>> +   || (type = MINMAX &&
>>> +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX_ALARM) && 
>>> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX_ALARM) > 0.5)) {
>>>      printf(" ALARM");
>>>    }
>>>    printf("\n");
>>>    
>> -Is the type = MINMAX check for printing the alarms necessary, doesn't this 
>> has the risk of not showing an alarm for some chip with a funky combination of 
>> temp sysfs entries?
> 
> This will need to be double-checked. Removing the check altogether has
> the risk of printing alarms where they do not belong, which isn't
> better. Temperatures are a bit tricky because we may have up to 5 limit
> values to display and we only have 2 slots on the first line. So some
> of the limits may move to a second line. And I think we want to display
> the ALARM flag on the right line in this case. Or maybe you think the
> ALARM flag should always be on the first line even if it corresponds to
> a limit which is on the second line?
> 

I don't have any real preference either way (ALARM always on the first line, or 
on the line with the matching limit).

>> -Shouldn't the code somehow show the difference between a min and a max (and a 
>> generic) alarm? See how print_generic_chip_in() does this
> 
> Yes I know the voltage code does this, but the temperature code is much
> more complex at the moment. I have no objection if you want to add this
> feature, but let's first cleanup the code.
> 

+1

> I see we have SENSORS_FEATURE_TEMP_OVER, SENSORS_FEATURE_TEMP_LOW and
> SENSORS_FEATURE_TEMP_HIGH defined. What are these? We don't have names
> for these in our standard sysfs interface, and they seem completely
> redundant with SENSORS_FEATURE_TEMP_MAX and SENSORS_FEATURE_TEMP_MIN.
> Did your students get confused by the non-standard names in the 2.4
> driver somehow? It looks to me like we could plain delete these three
> symbols.
> 

I think my students may have gotten confused by the 2.4 names in lib/chips.c 
just like I was, so if you're reasonable sure that these do not exist in 2.6 
drivers, feel free to remove them.

>> Some generic notes not directly related to your patch:
>>
>> This piece of code in print_generic_chip_temp() can be done much simpler:
>>
>>    if (type = LIM) {
>>    } else {
>>      print_temp_info_real(val, max, min, 0.0, type, 1, 1);
>>    }
>>
>> Since lim always is 0.0 (this initalized to this) in the non LIM case, this can 
>> be written as just:
>>    print_temp_info_real(val, max, min, lim, type, 1, 1);
>>
>> Will you change this or shall I?
> 
> I would first need to be explained what this "LIM" thing is. It
> doesn't correspond to anything in our standard sysfs interface as far
> as I can see, and no legacy chip code was using it. I would get rid of
> it.
> 

I think my students added then LIM print_temp_info() minmax type, to support 
chips like the fscscy, see print_fscfsy() in prog/sensors/chips.c

>> More in general print_generic_chip_temp(), needs a good review by someone who 
>> is well aware of all possible tempX sysfs entry combo's (iow not by me).
> 
> We agree ;) see my comments above. If we get rid of the 4 unneeded
> limits as I am suggesting, I'm certain things will be a lot easier. Of
> course, even then, there are some decisions to make because we can have
> many different combinations of temperature limits depending on the
> chip, and we want to present them in the way that makes the more sense
> each time. I will look into this after the cleanups are done.
> 

Ok, well first we need a decision on the LIM feature + print support, then I 
can do some cleanup based on what was discussed sofar, after which it would be 
really nice if you could review print_generic_chip_temp()

>> Also print_generic_chip_in(), has some dangerous and unneeded assumptions, for 
>> example:
>> -it assumes that if there is a inX_max that there will also always be a inX_min
>> -it assumes that if there is no inX_max there will also be no alarms
>>
>> Shall I fix these or will you?
> 
> The code shouldn't assume anything like that. Please fix.
> 

will do.

>> Somewhat less "unneeded" assumption
>> -it doesn't check for inX_max_alarm without an inX_min_alarm and visa versa, I
>>   admit this would be a driver bug, as then the driver should have just a
>>   inX_alarm, but still we might want to concider this.
> 
> Your own assumption is wrong. A chip could have a voltage input with a
> min limit and a max limit, and an alarm which is only raised by the max
> limit. In this case, the driver would indeed have inX_max_alarm and not
> inX_min_alarm. Not to say that this particular hardware design would be
> smart, but if it existed, that's what how our driver would support it.
> And we've seen such weird hardware designs already... So you could fix
> this too.
> 

will do.

> I've committed my pending patches now, so if you could now fix the
> problems you've seen, this would be welcome.

Once I've got an answer on the LIM thing from you I'll fix all off the above.

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Jean Delvare]

Hi Hans,

On Tue, 29 May 2007 10:19:03 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > I see we have SENSORS_FEATURE_TEMP_OVER, SENSORS_FEATURE_TEMP_LOW and
> > SENSORS_FEATURE_TEMP_HIGH defined. What are these? We don't have names
> > for these in our standard sysfs interface, and they seem completely
> > redundant with SENSORS_FEATURE_TEMP_MAX and SENSORS_FEATURE_TEMP_MIN.
> > Did your students get confused by the non-standard names in the 2.4
> > driver somehow? It looks to me like we could plain delete these three
> > symbols.
> 
> I think my students may have gotten confused by the 2.4 names in lib/chips.c 
> just like I was, so if you're reasonable sure that these do not exist in 2.6 
> drivers, feel free to remove them.

Yes, I am reasonably sure. Fix committed.

> > I would first need to be explained what this "LIM" thing is. It
> > doesn't correspond to anything in our standard sysfs interface as far
> > as I can see, and no legacy chip code was using it. I would get rid of
> > it.
> 
> I think my students added then LIM print_temp_info() minmax type, to support 
> chips like the fscscy, see print_fscfsy() in prog/sensors/chips.c

I see. But I have no idea what this corresponds to. I do not have the
FSC Scylla datasheet.

OK, after digging into the old fscscy driver code, it seems that the
"min" and "max" temperature values returned by the driver aren't
limits, but the minimum and maximum measured temperatures over the
driver lifetime. Weird, no other driver does this. So "lim" would
presumably be the high temperature limit which all other drivers call
max or high or over.

This confirms my intuition that we don't need any "LIM" temperature
display type. Fix committed.

> Ok, well first we need a decision on the LIM feature + print support, then I 
> can do some cleanup based on what was discussed sofar, after which it would be 
> really nice if you could review print_generic_chip_temp()

I've committed the temperature cleanups already. I'll let you commit
the fixes you suggested for voltages now.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors