[lm-sensors] [PATCH 1/4] Simplify sensors_get_all_features()

[lm-sensors] [PATCH 1/4] Simplify sensors_get_all_features()

[Jean Delvare]

The way we build the feature lists guarantees that subfeatures always
immediately follow their main feature. This makes it possible to simplify
sensors_get_all_features() quite a bit. We no longer need to maintain
separate pointers for the last main feature and the last subfeature,
we can simply walk the list linearly.

Note that I am still not entirely happy with this API. It was obviously
designed for debugging purposes (sensors -u) and without performance
concernes nor interface cleanliness in mind. I believe that we want to
tag main features and subfeatures as such, and let the application ask
specifically for the list of main features, and for each feature, for
its list of subfeatures (i.e. two functions instead of one.)

---
 lib/access.c                 |   21 ++++-----------------
 lib/libsensors.3             |   12 +++++++++---
 lib/sensors.h                |    6 +++---
 prog/sensors/chips.c         |    6 +++---
 prog/sensors/chips_generic.c |   34 +++++++++++++++++-----------------
 5 files changed, 36 insertions(+), 43 deletions(-)

--- lm-sensors-3.orig/lib/access.c	2007-07-22 14:52:57.000000000 +0200
+++ lm-sensors-3/lib/access.c	2007-07-22 15:28:33.000000000 +0200
@@ -333,9 +333,9 @@ const char *sensors_get_adapter_name(int
 	return NULL;
 }
 
-/* nr1-1 is the last main feature found; nr2-1 is the last subfeature found */
+/* nr-1 is the last feature returned */
 const sensors_feature_data *sensors_get_all_features(sensors_chip_name name,
-						     int *nr1, int *nr2)
+						     int *nr)
 {
 	sensors_chip_feature *feature_list;
 	int i;
@@ -343,22 +343,9 @@ const sensors_feature_data *sensors_get_
 	for (i = 0; i < sensors_proc_chips_count; i++)
 		if (sensors_match_chip(sensors_proc_chips[i].chip, name)) {
 			feature_list = sensors_proc_chips[i].feature;
-			if (!*nr1 && !*nr2) {	/* Return the first entry */
-				*nr1 = *nr2 = 1;
-				return &feature_list->data;
-			}
-			for ((*nr2)++; feature_list[*nr2 - 1].data.name; (*nr2)++)
-				if (feature_list[*nr2 - 1].data.mapping =
-				    feature_list[*nr1 - 1].data.number)
-					return &((feature_list + *nr2 - 1)->data);
-			for ((*nr1)++;
-			     feature_list[*nr1 - 1].data.name
-			     && (feature_list[*nr1 - 1].data.mapping !-				 SENSORS_NO_MAPPING); (*nr1)++) ;
-			*nr2 = *nr1;
-			if (!feature_list[*nr1 - 1].data.name)
+			if (!feature_list[*nr].data.name)
 				return NULL;
-			return &((feature_list + *nr1 - 1)->data);
+			return &feature_list[(*nr)++].data;
 		}
 	return NULL;
 }
--- lm-sensors-3.orig/lib/sensors.h	2007-07-22 14:52:57.000000000 +0200
+++ lm-sensors-3/lib/sensors.h	2007-07-22 15:28:33.000000000 +0200
@@ -179,13 +179,13 @@ typedef struct sensors_feature_data {
 /* This returns all features of a specific chip. They are returned in
    bunches: everything with the same mapping is returned just after each
    other, with the master feature in front (that feature does not map to
-   itself, but has SENSORS_NO_MAPPING as mapping field). nr1 and nr2 are
-   two internally used variables. Set both to zero to start again at the
+   itself, but has SENSORS_NO_MAPPING as mapping field). nr is
+   an internally used variable. Set it to zero to start again at the
    begin of the list. If no more features are found NULL is returned.
    Do not try to change the returned structure; you will corrupt internal
    data structures. */
 extern const sensors_feature_data *sensors_get_all_features
-             (sensors_chip_name name, int *nr1,int *nr2);
+             (sensors_chip_name name, int *nr);
 
 #ifdef __cplusplus
 }
--- lm-sensors-3.orig/prog/sensors/chips.c	2007-07-22 14:52:57.000000000 +0200
+++ lm-sensors-3/prog/sensors/chips.c	2007-07-22 15:28:33.000000000 +0200
@@ -118,13 +118,13 @@ void print_vid_info(const sensors_chip_n
 
 void print_unknown_chip(const sensors_chip_name *name)
 {
-  int a,b,valid;
+  int a, valid;
   const sensors_feature_data *data;
   char *label;
   double val;
  
-  a=b=0;
-  while((data=sensors_get_all_features(*name,&a,&b))) {
+  a = 0;
+  while((data=sensors_get_all_features(*name, &a))) {
     if (sensors_get_label_and_valid(*name,data->number,&label,&valid)) {
       printf("ERROR: Can't get feature `%s' data!\n",data->name);
       continue;
--- lm-sensors-3.orig/prog/sensors/chips_generic.c	2007-07-22 14:52:57.000000000 +0200
+++ lm-sensors-3/prog/sensors/chips_generic.c	2007-07-22 15:28:33.000000000 +0200
@@ -35,7 +35,7 @@ static int get_feature_value(const senso
 
 static void sensors_get_available_features(const sensors_chip_name *name, 
                                            const sensors_feature_data *feature, 
-                                           int i, int j, 
+                                           int i,
                                            short *has_features, 
                                            double *feature_vals, 
                                            int size, 
@@ -43,7 +43,7 @@ static void sensors_get_available_featur
 {
   const sensors_feature_data *iter;
   
-  while((iter = sensors_get_all_features(*name, &i, &j)) && 
+  while((iter = sensors_get_all_features(*name, &i)) &&
       iter->mapping = feature->number) {
     int indx;
     
@@ -62,13 +62,13 @@ static void sensors_get_available_featur
 
 static int sensors_get_label_size(const sensors_chip_name *name)
 {
-  int i, j, valid;
+  int i, valid;
   const sensors_feature_data *iter;
   char *label;
   unsigned int max_size = 11; /* Initialised to 11 as minumum label-width */
 
-  i = j = 0;
-  while((iter = sensors_get_all_features(*name, &i, &j))) {
+  i = 0;
+  while((iter = sensors_get_all_features(*name, &i))) {
     if (!sensors_get_label_and_valid(*name, iter->number, &label, &valid) &&
         valid && strlen(label) > max_size)
       max_size = strlen(label);
@@ -89,7 +89,7 @@ static inline float deg_ctof(float cel)
 #define TEMP_FEATURE_VAL(x) feature_vals[x - SENSORS_FEATURE_TEMP - 1]
 static void print_generic_chip_temp(const sensors_chip_name *name, 
                                     const sensors_feature_data *feature,
-                                    int i, int j, int label_size)
+                                    int i, int label_size)
 {
   double val, max, min;
   char *label;
@@ -113,7 +113,7 @@ static void print_generic_chip_temp(cons
     return;
   }
   
-  sensors_get_available_features(name, feature, i, j, has_features, 
+  sensors_get_available_features(name, feature, i, has_features,
       feature_vals, size, SENSORS_FEATURE_TEMP);
   
   if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX)) {
@@ -207,7 +207,7 @@ static void print_generic_chip_temp(cons
 #define IN_FEATURE_VAL(x) feature_vals[x - SENSORS_FEATURE_IN - 1]
 static void print_generic_chip_in(const sensors_chip_name *name, 
                                   const sensors_feature_data *feature,
-                                  int i, int j, int label_size)
+                                  int i, int label_size)
 {
   const int size = SENSORS_FEATURE_IN_MAX_ALARM - SENSORS_FEATURE_IN;
   int valid;
@@ -231,7 +231,7 @@ static void print_generic_chip_in(const 
     return;
   }
   
-  sensors_get_available_features(name, feature, i, j, has_features, feature_vals,
+  sensors_get_available_features(name, feature, i, has_features, feature_vals,
       size, SENSORS_FEATURE_IN);
   
   print_label(label, label_size);
@@ -274,7 +274,7 @@ static void print_generic_chip_in(const 
 #define FAN_FEATURE_VAL(x) feature_vals[x - SENSORS_FEATURE_FAN - 1]
 static void print_generic_chip_fan(const sensors_chip_name *name, 
                                    const sensors_feature_data *feature,
-                                   int i, int j, int label_size)
+                                   int i, int label_size)
 {
   char *label;
   int valid;
@@ -308,7 +308,7 @@ static void print_generic_chip_fan(const
   else
     printf("%4.0f RPM", val);
   
-  sensors_get_available_features(name, feature, i, j, has_features, feature_vals,
+  sensors_get_available_features(name, feature, i, has_features, feature_vals,
       size, SENSORS_FEATURE_FAN);
   
   if (FAN_FEATURE(SENSORS_FEATURE_FAN_MIN) &&
@@ -332,22 +332,22 @@ static void print_generic_chip_fan(const
 void print_generic_chip(const sensors_chip_name *name)
 {
   const sensors_feature_data *feature;
-  int i,j, label_size;
+  int i, label_size;
   
   label_size = sensors_get_label_size(name);
   
-  i = j = 0;
-  while((feature = sensors_get_all_features(*name, &i, &j))) {
+  i = 0;
+  while((feature = sensors_get_all_features(*name, &i))) {
     if (feature->mapping != SENSORS_NO_MAPPING)
       continue;
     
     switch (feature->type) {
       case SENSORS_FEATURE_TEMP:
-        print_generic_chip_temp(name, feature, i, j, label_size); break;
+        print_generic_chip_temp(name, feature, i, label_size); break;
       case SENSORS_FEATURE_IN:
-        print_generic_chip_in(name, feature, i, j, label_size); break;
+        print_generic_chip_in(name, feature, i, label_size); break;
       case SENSORS_FEATURE_FAN:
-        print_generic_chip_fan(name, feature, i, j, label_size); break;
+        print_generic_chip_fan(name, feature, i, label_size); break;
       case SENSORS_FEATURE_VID:
         print_vid_info(name, feature->number, label_size); break;
       default: continue;
--- lm-sensors-3.orig/lib/libsensors.3	2007-07-22 14:52:57.000000000 +0200
+++ lm-sensors-3/lib/libsensors.3	2007-07-22 14:53:40.000000000 +0200
@@ -49,7 +49,7 @@ libsensors \- publicly accessible functi
 .B extern int sensors_do_all_sets(void);
 .B const sensors_chip_name *sensors_get_detected_chips(int *nr);
 .B const sensors_feature_data *sensors_get_all_features 
-             \fB(sensors_chip_name name, int *nr1,int *nr2);\fP
+             \fB(sensors_chip_name name, int *nr);\fP
 .B const char *libsensors_version;
 .fi
 .SH DESCRIPTION
@@ -133,9 +133,15 @@ The mode field can be one of:
 SENSORS_MODE_NO_RW, SENSORS_MODE_R, SENSORS_MODE_W or SENSORS_MODE_RW.
 
 \fBconst sensors_feature_data *sensors_get_all_features
-      (sensors_chip_name name, int *nr1,int *nr2);\fP
+      (sensors_chip_name name, int *nr);\fP
 .br
-This returns all features of a specific chip. They are returned in bunches: everything with the same mapping is returned just after each other, with the master feature in front (that feature does not map to itself, but has SENSORS_NO_MAPPING as mapping field). nr1 and nr2 are two internally used variables. Set both to zero to start again at the begin of the list. If no more features are found NULL is returned. Do not try to change the returned structure; you will corrupt internal data structures.
+This returns all features of a specific chip. They are returned in bunches:
+everything with the same mapping is returned just after each other, with
+the master feature in front (that feature does not map to itself, but
+has SENSORS_NO_MAPPING as mapping field). nr is an internally used variable.
+Set it to zero to start again at the begin of the list. If no more features
+are found NULL is returned. Do not try to change the returned structure; you
+will corrupt internal data structures.
 
 \fBconst char *libsensors_version;\fP
 .br


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 1/4] Simplify sensors_get_all_features()

[Hans de Goede]

Jean Delvare wrote:
> The way we build the feature lists guarantees that subfeatures always
> immediately follow their main feature. This makes it possible to simplify
> sensors_get_all_features() quite a bit. We no longer need to maintain
> separate pointers for the last main feature and the last subfeature,
> we can simply walk the list linearly.
> 

Sounds & looks good. I'm not all to familiar with the internals of
sensors_get_all_features() though, so I might have missed something, but the 
changes look ok to me.


> Note that I am still not entirely happy with this API. It was obviously
> designed for debugging purposes (sensors -u) and without performance
> concernes nor interface cleanliness in mind. I believe that we want to
> tag main features and subfeatures as such, and let the application ask
> specifically for the list of main features, and for each feature, for
> its list of subfeatures (i.e. two functions instead of one.)

I am thinking very much along the same lines. Maybe however, it would be better 
to only have an iteration function for mainfeatures, and have seperate structs 
for sub_features and main_features like this:

typedef struct sensors_sub_feature_data {
   const char *name;
   int type;  // SENSORS_FEATURE_UNKNOWN when not present
   int flags; // mode + use main feature compute rule flag, 0 when not present
} sensors_sub_feature_data;


typedef struct sensors_main_feature_data {
   const char *name;
   int type;
   int mode;
   /* SENSORS_FEATURE_MAX_SUB_FEATURES - 1, because
      SENSORS_FEATURE_MAX_SUB_FEATURES includes the main feature,
      maybe we should change this? */
   sensors_sub_feature_data sub_features[SENSORS_FEATURE_MAX_SUB_FEATURES - 1];
} sensors_feature_data;

Then the application itself could easily see which subfeatures are available, a 
potential problem with this approach is that the size of the 
sensors_main_feature_data struct will change if we add new sub-feature types, 
causing  SENSORS_FEATURE_MAX_SUB_FEATURES to grow. If we however always return 
a pointer to sensors_main_feature_data, then the struct growing is not a 
problem. An application will just not use / look for the new sub_features.

We could then also have a matching sensors_get_feature which takes an array of 
doubles, reading all values at once, which gets passed an array size, so that 
an app can say I'm only interested in the main feature + the first X sub features.

Notice that I removed the number from the feature data, instead the name could 
be directly passed to sensors_get_feature, afaik we have no need for the number 
any more. Hmm, maybe we do for the compute_mappings.

If we go this way, the feature data inside libsensors could be stored in the 
same way, maybe we can then even completely remove sensors_get_all_features, 
and just add a pointer to an array of main_features to the chip structs being 
passed around. This array would be terminated with a main_feature entry with a 
name of NULL.

Regards,

Hans

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

Re: [lm-sensors] [PATCH 1/4] Simplify sensors_get_all_features()

[Jean Delvare]

Hi Hans,

On Sun, 22 Jul 2007 17:01:22 +0200, Hans de Goede wrote:
> > Note that I am still not entirely happy with this API. It was obviously
> > designed for debugging purposes (sensors -u) and without performance
> > concernes nor interface cleanliness in mind. I believe that we want to
> > tag main features and subfeatures as such, and let the application ask
> > specifically for the list of main features, and for each feature, for
> > its list of subfeatures (i.e. two functions instead of one.)
> 
> I am thinking very much along the same lines. Maybe however, it would be better 
> to only have an iteration function for mainfeatures, and have seperate structs 
> for sub_features and main_features like this:
> 
> typedef struct sensors_sub_feature_data {
>    const char *name;
>    int type;  // SENSORS_FEATURE_UNKNOWN when not present
>    int flags; // mode + use main feature compute rule flag, 0 when not present
> } sensors_sub_feature_data;

I agree that the "compute mapping" can be a simple boolean flag rather
a feature number. But note that the user application doesn't need to
know whether the subfeature follows its master's compute rule or not,
so we don't have to export this information.

I'm not happy with type = SENSORS_FEATURE_UNKNOWN when not present. I
believe that a missing feature should not be listed at all.

> typedef struct sensors_main_feature_data {
>    const char *name;
>    int type;
>    int mode;
>    /* SENSORS_FEATURE_MAX_SUB_FEATURES - 1, because
>       SENSORS_FEATURE_MAX_SUB_FEATURES includes the main feature,
>       maybe we should change this? */
>    sensors_sub_feature_data sub_features[SENSORS_FEATURE_MAX_SUB_FEATURES - 1];
> } sensors_feature_data;
> 
> Then the application itself could easily see which subfeatures are available,

Indeed, this is a possible alternative to a second function.

> a potential problem with this approach is that the size of the 
> sensors_main_feature_data struct will change if we add new sub-feature types, 
> causing  SENSORS_FEATURE_MAX_SUB_FEATURES to grow. If we however always return 
> a pointer to sensors_main_feature_data, then the struct growing is not a 
> problem. An application will just not use / look for the new sub_features.

First of all, I have already mentioned that I do NOT want
SENSORS_FEATURE_MAX_SUB_FEATURES to be exposed to the public. This is
creating a limitation that can easily be avoided. If user applications
really need to know this value, it should be returned by a library
function. But ideally, I believe that the user application should not
need to care about this value at all.

Secondly, you cannot assume that the application will only read the
structure you are returning a pointer to. It could copy these
structures to an array for later use - then the structure size matters
a lot. So the proposal above is simply not acceptable.

Anyway, this would be very inefficient memory-wise. Allocating memory
for 21 subfeatures for every feature would be a waste of memory.
Typically, fans have 3, voltages have 4 and temperatures have between 4
and 8. So I'd rather use a NULL-terminated list (or alternatively have
a "subfeat_nr" struct member):

typedef struct sensors_main_feature_data {
   const char *name;
   int type;
   int mode;
   /* const? */ sensors_sub_feature_data *sub_features;
} sensors_feature_data;

> We could then also have a matching sensors_get_feature which takes an array of 
> doubles, reading all values at once, which gets passed an array size, so that 
> an app can say I'm only interested in the main feature + the first X sub features.

I don't think this is really interesting. Ideally the application
shouldn't assume anything about which are the "first subfeatures". But
what we can have is a function taking an array of subfeature numbers
and filling a same-sized array with their respective values. So the
application can asks for exactly what it wants, in one call.

> Notice that I removed the number from the feature data, instead the name could 
> be directly passed to sensors_get_feature, afaik we have no need for the number 
> any more. Hmm, maybe we do for the compute_mappings.

I have been thinking about it too. Given that the names are standard
now, and the number isn't (it's generated by libsensors) it would make
some sense to use the names as key identifiers. But OTOH, string
comparison is slower than integer comparisons, so there is a
performance concern.

One reason why I am particularly interested in this is that I want to
add a command line interface to "sensors", where you could do something
like:

$ sensors f71805f-isa-0290/temp1
+45°C
$

Having a sensors_get_value_by_name() function would make it very easy.
OTOH, this just means that libsensors walks the list of features
instead of the user application, so from a performance point of view it
probably doesn't matter that much.

> If we go this way, the feature data inside libsensors could be stored in the 
> same way,

Of course. We really want to store the things internally in the same
form we give to the user application, otherwise performance will suffer
significantly.

>            maybe we can then even completely remove sensors_get_all_features, 
> and just add a pointer to an array of main_features to the chip structs being 
> passed around. This array would be terminated with a main_feature entry with a 
> name of NULL.

Yes, ultimately I would like to get rid of sensors_get_all_feature().

But as you can see, the question is now: how far do we want to go?
Because we also don't want to delay lm-sensors-3.0.0 too much.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 1/4] Simplify sensors_get_all_features()

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Sun, 22 Jul 2007 17:01:22 +0200, Hans de Goede wrote:
>>> Note that I am still not entirely happy with this API. It was obviously
>>> designed for debugging purposes (sensors -u) and without performance
>>> concernes nor interface cleanliness in mind. I believe that we want to
>>> tag main features and subfeatures as such, and let the application ask
>>> specifically for the list of main features, and for each feature, for
>>> its list of subfeatures (i.e. two functions instead of one.)
>> I am thinking very much along the same lines. Maybe however, it would be better 
>> to only have an iteration function for mainfeatures, and have seperate structs 
>> for sub_features and main_features like this:
>>
>> typedef struct sensors_sub_feature_data {
>>    const char *name;
>>    int type;  // SENSORS_FEATURE_UNKNOWN when not present
>>    int flags; // mode + use main feature compute rule flag, 0 when not present
>> } sensors_sub_feature_data;
> 
> I agree that the "compute mapping" can be a simple boolean flag rather
> a feature number. But note that the user application doesn't need to
> know whether the subfeature follows its master's compute rule or not,
> so we don't have to export this information.
> 

Agreed

> I'm not happy with type = SENSORS_FEATURE_UNKNOWN when not present. I
> believe that a missing feature should not be listed at all.
> 

I concidered this too, but I want an application to be able to write
if (main_feature->sub_features[SENSORS_FEATURE_IN_MAX].mode)
   // display max value for this in sensor

This is why I made it a sparse (as in some entries are invalid) array instead 
of a pointer to a dynamicly allocated array.

Alternatively, we would not have subfeatures in the main_feature struct at all, 
but have a:
double sensors_get_sub_feature(sensors_chip_name name, int main_feature,
    int subfeature_type);

Which will return the value of subfeature if present and 
SENSORS_FEATURE_NOT_PRESENT (which will be HUGE_VAL) otherwise.

And change the prototype of sensors_get_feature to match, which I suggest 
because I dislike the use of pass by reference while there really is only one 
thing to return. Alternatively we could use the same prototype as 
sensors_get_feature, with an added int subfeature_type param.

Actually thinking about this and matching this thought with another part of 
your reply:

 > Yes, ultimately I would like to get rid of sensors_get_all_feature().
 >
 > But as you can see, the question is now: how far do we want to go?
 > Because we also don't want to delay lm-sensors-3.0.0 too much.
 >

I think that thas may be the way to go make sensors_get_all_features() only 
return main features, and add a sensors_get_sub_feature() function:

Then we don't need the main_feature / sub_feature struct difference and can 
thus keep everything as is except for the above change, so that we can do 
lm_sensors-3.0.0 in a timely fasion.

There will no longer be an obvious way for an application to get all 
subfeatures then, but is it a problem that an application cannot get 
subfeatures it doesn't know about / doesn't know how to handle them?

<snip since my above train of thoughts have left the track discussed sofar>

>> Notice that I removed the number from the feature data, instead the name could 
>> be directly passed to sensors_get_feature, afaik we have no need for the number 
>> any more. Hmm, maybe we do for the compute_mappings.
> 
> I have been thinking about it too. Given that the names are standard
> now, and the number isn't (it's generated by libsensors) it would make
> some sense to use the names as key identifiers. But OTOH, string
> comparison is slower than integer comparisons, so there is a
> performance concern.
> 

I agree, that in the end keeping the numbers is better for performance reasons, 
so lets keep them.

Regards,

Hans


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

Re: [lm-sensors] [PATCH 1/4] Simplify sensors_get_all_features()

[Jean Delvare]

Hi Hans,

On Mon, 23 Jul 2007 11:50:35 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > I'm not happy with type = SENSORS_FEATURE_UNKNOWN when not present. I
> > believe that a missing feature should not be listed at all.
> 
> I concidered this too, but I want an application to be able to write
> if (main_feature->sub_features[SENSORS_FEATURE_IN_MAX].mode)
>    // display max value for this in sensor
> 
> This is why I made it a sparse (as in some entries are invalid) array instead 
> of a pointer to a dynamicly allocated array.
> 
> Alternatively, we would not have subfeatures in the main_feature struct at all, 
> but have a:
> double sensors_get_sub_feature(sensors_chip_name name, int main_feature,
>     int subfeature_type);

Yes, that's (almost) what I had in mind. I prefer this over a sparse
array.

> Which will return the value of subfeature if present and 
> SENSORS_FEATURE_NOT_PRESENT (which will be HUGE_VAL) otherwise.

I'm not a big fan of using HUGE_VAL for errors.

> And change the prototype of sensors_get_feature to match, which I suggest 
> because I dislike the use of pass by reference while there really is only one 
> thing to return. Alternatively we could use the same prototype as 
> sensors_get_feature, with an added int subfeature_type param.

I'd prefer this, yes. Looks cleaner than using HUGE_VAL for errors.

> Actually thinking about this and matching this thought with another part of 
> your reply:
> 
>  > Yes, ultimately I would like to get rid of sensors_get_all_feature().
>  >
>  > But as you can see, the question is now: how far do we want to go?
>  > Because we also don't want to delay lm-sensors-3.0.0 too much.
> 
> I think that thas may be the way to go make sensors_get_all_features() only 
> return main features, and add a sensors_get_sub_feature() function:

Agreed. When I wrote "get rid of sensors_get_all_feature()", I really
meant to add "as it exists now."

> Then we don't need the main_feature / sub_feature struct difference and can 
> thus keep everything as is except for the above change, so that we can do 
> lm_sensors-3.0.0 in a timely fasion.

Indeed. And we can improve the internal implementation later, for
better performance.

> There will no longer be an obvious way for an application to get all 
> subfeatures then, but is it a problem that an application cannot get 
> subfeatures it doesn't know about / doesn't know how to handle them?

Good question. Looking at the code of xsensors, it really wants
specific subfeatures. "sensors" wants them all though. This is why it
is important to have several applications in mind when designing an API.

Note that nothing prevents us from offering an additional function to
get all subfeatures, working the same way sensors_get_all_features()
does on main features.

One thing I still need to check is how the early revisions of the GL518
are handled (voltage channels with limits but no input value.) This is
weird but it exists so out model needs to handle it properly.

-- 
Jean Delvare

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