Difference between revisions of "Improved configuration enumeration"
Line 139: | Line 139: | ||
There could of course be a similar function for a list of uint64_t: | There could of course be a similar function for a list of uint64_t: | ||
uint64_t *sr_config_values_list_uint64(struct sr_config_values *vals, int *len) | uint64_t *sr_config_values_list_uint64(struct sr_config_values *vals, int *len) | ||
Perhaps the two calls to retrieve a list for this key might be combined into a single function: | |||
int sr_config_list_int32(const struct sr_dev_driver *driver, | |||
const struct sr_dev_inst *sdi, | |||
const struct sr_probe_group *probe_group, | |||
int key, | |||
int32_t *values, | |||
int *len); |
Revision as of 12:46, 1 November 2013
Problem
The initial concept of sr_config_list was that it would be a simple counterpart to sr_config_set and sr_config_get, returning a sequence of possible GVariant values for a setting.
In practice however, the possible values of a setting may be very numerous. This arose first in the case of devices which could accept any sample rate in a (minimum, maximum, step) range. A special case is in place to support this, but this special case complicates the API for clients.
We now have the case of a device where a setting is discrete in one part of the range, but continuous in another, so cannot be represented as either a list or a constant-step range. This will require another special case.
It seems very unlikely this will be the last, or the least complicated use case.
Adding more special case return values to the existing sr_config_list function is not a solution. Every frontend and language binding must reproduce support for every special case. Changes to each special case to handle new scenarios will break existing clients.
The fundamental problem is that the set of possible values for a given configuration setting may be arbitrarily complex to express.
Proposal
Concept
We should hide this complexity from clients. In general, clients want to know specific things such as:
- What is the closest valid value to this number which the user gave me?
- What would be a sensible UI element to display for this setting?
- What is the next value up from this one?
We should focus on answering these questions rather than handing out a growing list of complex data structures and telling them each to figure it out for themselves.
Implementation
- Change the output type of sr_config_list from GVariant * to a new type, sr_config_values.
- Define functions to make various queries of this type as required for known use cases.
- Leave it up to drivers how to implement these behind the scenes.
API
/* A set of possible configuration settings. */ struct sr_config_values { // Enum value - see below. int type; // Private data defining the set. void *priv; };
/* Common types of set that can be identified for various shortcuts. */ enum { /* A discrete list - may be retrieved as such. */ SR_CONFVAL_DISCRETE, /* A range with a fixed start, end and step size. */ SR_CONFVAL_RANGE, /* Anything else. */ SR_CONFVAL_OTHER, };
/* New signature for sr_config_list. */ int sr_config_list(const struct sr_dev_driver *driver, const struct sr_dev_inst *sdi, const struct sr_probe_group *probe_group, int key, struct sr_config_values **values);
/* Query functions (examples - others may be needed too) */ // Returns a list of all values, or NULL if that's not practical. GSList *sr_config_values_list(struct sr_config_values *values); // Returns the next possible value after a given one, or NULL if none. GVariant *sr_config_values_next(struct sr_config_values *values, GVariant *value); // Returns the previous possible value before a given one, or NULL if none. GVariant *sr_config_values_prev(struct sr_config_values *values, GVariant *value); // Returns the closest possible value to a given one. GVariant *sr_config_values_closest(struct sr_config_values *values, GVariant *value); // Returns the maximum possible value. GVariant *sr_config_values_min(struct sr_config_values *values); // Returns the minimum possible value. GVariant *sr_config_values_max(struct sr_config_values *values); // Returns a sensible step size for values in the vicinity of a given one. GVariant *sr_config_values_step(struct sr_config_values *values, GVariant *value);
Usage
Clients would call sr_config_list as they do in the existing API. The difference is that rather than returning a complex GVariant data structure which clients must parse and interpret, the function returns a single object representing the set of possible values, which clients can query easily for properties of interest.
The only field of the object which can be accessed directly is the type field, which identifies whether the set is of a common type such as a discrete list, or a fixed-step range. This is intended to help shortcut to simple code for these common cases.
Advantages
- Allows any possible set to be supported, without changing API - we do not have to guess what strange sets we might encounter.
- Avoids duplicating code for common queries in every client and binding.
- New queries can be supported without changing API for existing ones.
- Efficient: driver must only do what is needed to answer what the client wants to know.
Outstanding issues
Simple lists
Using a plain old list of int32_t, such as that returned for SR_CONF_SCAN_OPTIONS or SR_CONF_DEVICE_OPTIONS:
struct sr_config_values *cvals; GSList *lvals, *l; int32_t scanopt; if (sr_config_list(sdi->driver, sdi, NULL, SR_CONF_SCAN_OPTIONS, &cvals) == SR_OK) { if ((lvals = sr_config_values_list(cvals))) { for (l = lvals; l; l = l->next) { scanopt = l->data; sr_dbg("Scan option %d", scanopt); } } }
This involves casting the int32_t into the void * that is the GSList payload, which is a bit ugly. It also means allocating a new GSList for data that is often static in nature.
Another way to convey a list of int32_t would be as a pointer to a list of values. This would use a helper function:
int32_t *sr_config_values_list_int32(struct sr_config_values *vals, int *len)
The helper function could also check the sr_config_values type matches the type in the function name: sr_config_values_list_int32()
must only be called on a (new) confval type SR_CONFVAL_DISCRETE_INT32
.
A code example:
struct sr_config_values *cvals; int32_t *vals; int len, i; if (sr_config_list(sdi->driver, sdi, NULL, SR_CONF_SCAN_OPTIONS, &cvals) == SR_OK) { if ((vals = sr_config_values_list_int32_t(cvals, &len))) { for (i = 0; i < len; i++) { sr_dbg("Scan option %d", vals[i]); } } }
There could of course be a similar function for a list of uint64_t:
uint64_t *sr_config_values_list_uint64(struct sr_config_values *vals, int *len)
Perhaps the two calls to retrieve a list for this key might be combined into a single function:
int sr_config_list_int32(const struct sr_dev_driver *driver, const struct sr_dev_inst *sdi, const struct sr_probe_group *probe_group, int key, int32_t *values, int *len);