analogdevicesinc
add functions to decode _available attributes
Many IIO channel attributes have an _available modifier which is described as:
/sys/bus/iio/devices/iio:deviceX/sampling_frequency_available
- a small discrete set of values like "0 2 4 6 8"
- a range with minimum, step and maximum frequencies like
"[min step max]"
/sys/bus/iio/devices/iio:deviceX/in_activity_calibgender_available
Lists all available gender values (e.g.: male, female).
/sys/bus/iio/devices/iio:deviceX/in_intensity_hardwaregain_available
Lists all available hardware applied gain factors. Shared across all
channels.
/sys/.../iio:deviceX/out_voltageY_powerdown_mode_available
Lists all available output power down modes.
If Y is not present the mode is shared across all outputs.
/sys/bus/iio/devices/iio:deviceX/calibration_forced_value_available
Available range for the forced calibration value, expressed as:
- a range specified as "[min step max]"
/sys/.../events/in_accel_gesture_tap_value_available
Lists all available threshold values which can be used to
modify the sensitivity of the tap detection.
I would suggest we need a few fuctions for this:
-
ssize_t iio_channel_get_min(const struct iio_channel *chn)(returns SSIZE_MIN if doesn't exist) -
ssize_t iio_channel_get_step(const struct iio_channel *chn)(returns 0 if doesn't exist) -
ssize_t iio_channel_get_max(const struct iio_channel *chn)(returns SSIZE_MAX if doesn't exist) -
ssize_t iio_channel_get_available(const struct iio_channel *chn, char * buf, size_t len)(returns zero if get_min/get_max should be used, or if doesn't exist, and positive number of elements with all elements in buf.).
It would make things much easier for people, and get rid of all the string parsing in applications for users.
The functions should target an attribute instead of a channel, but I understand your idea. The functions could then be like this:
ssize_t iio_attr_range_get_min(const struct iio_attr *attr)
ssize_t iio_attr_range_get_step(const struct iio_attr *attr)
ssize_t iio_attr_range_get_max(const struct iio_attr *attr)
Alternatively, we could not use the 'range' and the functions will be:
ssize_t iio_attr_get_min(const struct iio_attr *attr)
ssize_t iio_attr_get_step(const struct iio_attr *attr)
ssize_t iio_attr_get_max(const struct iio_attr *attr)
The function for retrieving available values would simplify things further for users by eliminating the need to parse raw data from the buffer, split the values, and allocate memory for each one. I propose the following:
ssize_t iio_attr_get_available_count(const struct iio_attr *attr)
const char * iio_attr_get_available(const struct iio_attr *attr, unsigned int index)
Alternatively, we could add a suffix:
iio_attr_get_available_at
Or
iio_attr_get_available_item
Below, a code snipped of how they would be used:
struct iio_attr *attr;
ssize_t count, i;
const char *available_value;
...
count = iio_attr_get_available_count(attr);
for (i = 0; i < count; i++) {
available_value = iio_attr_get_available(i);
}
The functions should target an attribute instead of a channel, but I understand your idea. The functions could then be like this:
One thing to remember is that _available can also exist for device attributes...
I think Dan's proposal improves things but to me it still feels a lot of new user visible APIs being added and I'm not 100% convinced if the added value of this really pays off for doing this in the lib. 99% of the _available attributes are just a list of string separated by newlines (or spaces). Parsing that is not really that big of a deal. IMHO, I would only care for this if we get enough users asking for it....
Having said the above... Here it goes my 2 cents. I would keep it as simple as possible
// just return 0 in case we have no available or if it's not a list
size_t iio_attr_available_get_list_count(const struct iio_attr *attr);
// just get n_elemts. typically one will get them all
int iio_attr_available_get_list(const struct iio_attr *attr, const char *list, size_t n_elems);
// we could very well have some helper structs but not sure if it helps that much. The below will never change on the kernel // side as it would be an ABI breakage.
int iio_attr_available_get_range(const struct iio_attr *attr, int *min, int *step, int *max);
Another alternative would be to combine the first two APIs int
const char *iio_attr_available_get_list(const struct iio_attr *attr, const char *list, size_t n_elems)
So just duplicate the full list and give it to the user... But this could get hard for apps because one thing to have in mind is that available lists can change at runtime (while rare it happens).
TBH, not sure if we should provide a read_all_list or the iio_attr_get_available_at() than Dan suggests. But my feeling is that for starters we should provide the more generic API we can and I guess reading all the available attrs in the list is the common case right? I mean, that's what we do in osc for example (I think). We should only grow the API if we really need too and after understanding how users really want to use this. Trying to get that right from the start is not really doable... So yeah, keep it simple and generic for now.
Again, not sure if this is that simple to implement and when compared with the added value it brings, I really have my doubts on this...
simple is better.
int iio_attr_available_get_range(const struct iio_attr *attr, int *min, int *step, int *max);
returns error code (negative) if fails (including ENXIO if the _available attribute does not exist), and 0 if it is not in the form [max step min], and 1 if it works.
I agree that complicating/copying strings/etc for the _available when it's a list of strings - is well - duplicative - it just all comes down to if you are going to do the int iio_attr_available_get_range(), and then hide the _available attribute (like we do with _label) - how do you expose the strings?
I'm OK with just a specific iio_attr_available_get_list() - which simply just reads the _available attribute.
Thoughts?
Essentially, instead of reading the available list like this:
iio_attr_read_raw()
It would be read like:
iio_attr_available_get_list()
But users still need to guess a size where the list should fit and allocate memory for the list and then split it.
The main difference is that users would no longer have to deal with the attribute ending in "_available". And then, there's also iio_attr_available_get_range() which would help parsing strings of this format: "[min step max]"
I agree it's a simpler approach but it's not the full feature. @rgetz in the original post you said:
It would make things much easier for people, and get rid of all the string parsing in applications for users.
The API I proposed would allow users to:
- not worry about estimating size of memory where to copy the list
- not manage the memory (alloc & free)
- not have to split the list
In both cases (simpler approach or API suggested by me) hiding the _available attributes means that client code using libiio will need to be modified accordingly. This would be one more thing to be done when migrating from libiio v0 to libiio v1. Alternately, we could simply add a helper that would parse "[min step max]" and leave everything as it is.
I believe hiding the _available attributes could be an improvement, as they are read-only and don’t require much interaction and we end up with less attributes.
Does anyone foresee any potential issues by hiding the mentioned attributes? @rgetz @nunojsa @mhennerich @adisuciu @cristina-suteu
Now is the time to break compatibility - there is no such expectation on the 1.x branch.
maybe something like:
iio_attr_available_get_list(int *argc, const char ***argv)
would make sense?
I still like:
ssize_t iio_channel_get_min(const struct iio_channel *chn) (returns SSIZE_MIN if doesn't exist)
ssize_t iio_channel_get_step(const struct iio_channel *chn) (returns 0 if doesn't exist)
ssize_t iio_channel_get_max(const struct iio_channel *chn) (returns SSIZE_MAX if doesn't exist)
to handle numerics.