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
