[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