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
