libiio icon indicating copy to clipboard operation
libiio copied to clipboard

Enable re-reading the channel data format

Open dNechita opened this issue 2 months ago • 7 comments

PR Description

The problem:

The scan-element format (e.g. le:S24/32>>0 vs le:S20/32>>0) can change at runtime when users modify oversampling / resolution attributes. Libiio caches the original type string at context creation time with no means of changing it afterwards. This could affect downstream size calculations and conversion logic.

The fix:

  • Add public API: iio_channel_refresh_data_format() - explicit manual refresh
  • A refresh on all relevant channels is done right before buffer enable. This guarantees format correctness at the moment streaming starts.

Note: this implementation targets only the local backend. The rest of the backends need to be updated as well.

PR Type

  • [ ] Bug fix (a change that fixes an issue)
  • [x] New feature (a change that adds new functionality)
  • [ ] Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • [x] I have conducted a self-review of my own code changes
  • [x] I have commented new code, particularly complex or unclear areas
  • [x] I have checked that I did not introduce new warnings or errors (CI output)
  • [ ] I have checked that components that use libiio did not get broken
  • [ ] I have updated the documentation accordingly (GitHub Pages, READMEs, etc)

dNechita avatar Nov 14 '25 15:11 dNechita

rather than forcing this logic into all the applications - (refreshing things) - why not just do this inside libiio? (ie. - for this one special one - don't cache it).

rgetz avatar Nov 15 '25 16:11 rgetz

When I was looking into it I was trying to make scan_type an attribute, so it would be read like the others (e.g., _raw) And then I tripped into another design choices. For example, take this device:

$ tree
.
├── buffer
│   ├── data_available
│   ├── direction
│   ├── enable
│   ├── length
│   └── watermark
├── buffer0
│   ├── data_available
│   ├── direction
│   ├── enable
│   ├── in_voltage0_en
│   ├── in_voltage0_index
│   ├── in_voltage0_type
│   ├── length
│   └── watermark
├── events
│   ├── sampling_frequency
│   ├── sampling_frequency_available
│   ├── thresh_either_en
│   ├── thresh_falling_hysteresis
│   ├── thresh_falling_value
│   ├── thresh_rising_hysteresis
│   └── thresh_rising_value
├── in_voltage_calibscale
├── in_voltage_oversampling_ratio
├── in_voltage_oversampling_ratio_available
├── in_voltage_raw
├── in_voltage_scale
├── name
├── sampling_frequency
├── sampling_frequency_available
├── scan_elements
│   ├── in_voltage0_en
│   ├── in_voltage0_index
│   └── in_voltage0_type
├── trigger
│   └── current_trigger
├── uevent
└── waiting_for_supplier

Per ABI, it contains a general sampling_frequency and a events/sampling_frequency only for the sample rate used in events, as well as all events attributes are under events. But when I do

iio_info

        iio:device0: ad4062 (buffer capable)
                1 channels found:
                        voltage0:  (input, index: 0, format: be:s16/32>>0)
                        5 channel-specific attributes found:
                                attr  0: calibscale (in_voltage_calibscale) value: 1.000000000
                                attr  1: oversampling_ratio (in_voltage_oversampling_ratio) value: 1
                                attr  2: oversampling_ratio_available (in_voltage_oversampling_ratio_available) options: 1 2 4 8 16 32 64 128 256 512 1024 2048 4096
                                attr  3: raw (in_voltage_raw) value: -1
                                attr  4: scale (in_voltage_scale) value: 0.201416015
                9 device-specific attributes found:
                        attr  0: sampling_frequency value: 2000000
                        attr  1: sampling_frequency value: 2000000
                        attr  2: sampling_frequency_available options: 2000000 1000000 300000 100000 33300 10000 3000 500 333 250 200 166 140 124 111
                        attr  3: sampling_frequency_available options: 2000000 1000000 300000 100000 33300 10000 3000 500 333 250 200 166 140 124 111
                        attr  4: thresh_either_en value: ERROR: No such file or directory (-2)
                        attr  5: thresh_falling_hysteresis value: ERROR: No such file or directory (-2)
                        attr  6: thresh_falling_value value: ERROR: No such file or directory (-2)
                        attr  7: thresh_rising_hysteresis value: ERROR: No such file or directory (-2)
                        attr  8: thresh_rising_value value: ERROR: No such file or directory (-2)
                2 buffer attributes found:
                        attr  0: data_available options: 128
                        attr  1: direction value: in
                1 debug attributes found:
                        attr  0: direct_reg_access
                Current trigger: trigger0(ad4062-dev0)

The paths (e.g., events) are flushed away, resulting in "duplicated" sampling_frequency, as well as, currently (84aea8dd0d7c29f7d82871592d0c5a1dc03787c0) all events attributes appear to be broken, I guess trying to read from /sys/bus/iio/devices/iio\:device0/thresh_either_en instead of/sys/bus/iio/devices/iio\:device0/events/thresh_either_en. Kernel version: 6.12.0+

I see in the source code hardcoded "events/", scan_elements/, dir_is_scan_elements ... Wouldn't be easier to have the attributes names the full relpath events/thresh_either_en, and recursively add all that are not symbolic links (e.g. excludes subsystem, supplier*, of_node)?

I believe this way would be easier to set rules of which should or should not be cached.

$ iio_genxml

<?xml version="1.0" encoding="utf-8"?><!DOCTYPE context [<!ELEMENT context (device | context-attribute)*><!ELEMENT context-attribute EMPTY><!ELEMENT device (channel | attribute | debug-attribute | buffer-attribute)*><!ELEMENT channel (scan-element?, attribute*)><!ELEMENT attribute EMPTY><!ELEMENT scan-element EMPTY><!ELEMENT debug-attribute EMPTY><!ELEMENT buffer-attribute EMPTY><!ATTLIST context name CDATA #REQUIRED version-major CDATA #REQUIRED version-minor CDATA #REQUIRED version-git CDATA #REQUIRED description CDATA #IMPLIED><!ATTLIST context-attribute name CDATA #REQUIRED value CDATA #REQUIRED><!ATTLIST device id CDATA #REQUIRED name CDATA #IMPLIED label CDATA #IMPLIED><!ATTLIST channel id CDATA #REQUIRED type (input|output) #REQUIRED name CDATA #IMPLIED label CDATA #IMPLIED><!ATTLIST scan-element index CDATA #REQUIRED format CDATA #REQUIRED scale CDATA #IMPLIED><!ATTLIST attribute name CDATA #REQUIRED filename CDATA #IMPLIED><!ATTLIST debug-attribute name CDATA #REQUIRED><!ATTLIST buffer-attribute name CDATA #REQUIRED>]><context name="local" version-major="1" version-minor="0" version-git="84aea8dd" description="Linux analog 6.12.0+ #6 SMP PREEMPT Wed Nov 12 22:20:44 CET 2025 armv7l" ><context-attribute name="local,kernel" value="6.12.0+" /><context-attribute name="uri" value="local:" /><device id="iio:device0" name="ad4062" ><channel id="voltage0" type="input" ><scan-element index="0" format="be:s16/32&gt;&gt;0" scale="0.201416" /><attribute name="calibscale" filename="in_voltage_calibscale" /><attribute name="oversampling_ratio" filename="in_voltage_oversampling_ratio" /><attribute name="oversampling_ratio_available" filename="in_voltage_oversampling_ratio_available" /><attribute name="raw" filename="in_voltage_raw" /><attribute name="scale" filename="in_voltage_scale" /></channel><attribute name="sampling_frequency" /><attribute name="sampling_frequency" /><attribute name="sampling_frequency_available" /><attribute name="sampling_frequency_available" /><attribute name="thresh_either_en" /><attribute name="thresh_falling_hysteresis" /><attribute name="thresh_falling_value" /><attribute name="thresh_rising_hysteresis" /><attribute name="thresh_rising_value" /><debug-attribute name="direct_reg_access" /><buffer-attribute name="data_available" /><buffer-attribute name="direction" /></device><device id="trigger0" name="ad4062-dev0" ></device></context>

gastmaier avatar Nov 16 '25 11:11 gastmaier

rather than forcing this logic into all the applications - (refreshing things) - why not just do this inside libiio? (ie. - for this one special one - don't cache it).

Agree. I guess we could read it (cache it) before enabling buffering. A driver changing oversampling while buffering is most likely a bug in driver code itself. Or we just read it from the device "on demand".

The paths (e.g., events) are flushed away, resulting in "duplicated" sampling_frequency

Yes, that's not good...

scan_elements/, dir_is_scan_elements

Also note that the above directory together with buffer/ is legacy (I guess we still need to be aware of them for older kernels) but if buffer0/ exists, that should be the one we look at.

nunojsa avatar Nov 17 '25 08:11 nunojsa

rather than forcing this logic into all the applications - (refreshing things) - why not just do this inside libiio? (ie. - for this one special one - don't cache it).

Agree. I guess we could read it (cache it) before enabling buffering. A driver changing oversampling while buffering is most likely a bug in driver code itself. Or we just read it from the device "on demand".

Yes, that is exactly what is happening with the current changes, the format type is refreshed exactly before enabling the buffer. This step would be enough for most use cases. However, I wanted to provide users the ability to choose the moment when the caching is done as there might be scenarios where users modify the oversampling and know format might change and can call iio_channel_refresh_data_format() to get the newly format right away, possibly to just display it on their application or do something else with it like initializing some algorithm that uses the new format.

I thought about making iio_channel_get_data_format() always perform a fresh read instead of caching, but this would create significant performance overhead for already existing applications that call this function on every buffer element.

dNechita avatar Nov 20 '25 10:11 dNechita

Is this just a break in the social contract between the kernel and libiio then? kernel if it wants to have things be dynamic - should be suffixing with _raw?

Maybe @pcercuei or @larsclausen or @mhennerich knows the vision better...

rgetz avatar Nov 21 '25 22:11 rgetz

When I was looking into it I was trying to make scan_type an attribute, so it would be read like the others (e.g., _raw) And then I tripped into another design choices. For example, take this device:

$ tree
.
├── events
│   ├── sampling_frequency
│   ├── sampling_frequency_available
├── name
├── sampling_frequency
├── sampling_frequency_available

Per ABI, it contains a general `sampling_frequency` and a `events/sampling_frequency`

Is this a valid configuration? Can events/ dir contain an event that is device specific and not channel specific? Looking here https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-iio I can only see events specific to channels. If it's valid, then multiple fixes can be done. I have look at this from multiple angles. What would make sense is to add a new attribute category. Besides Device and Debug attributes there will be Event attributes and new API functions need to be added like: iio_device_get_event_attrs_count() iio_device_get_event_attr() iio_device_find_debug_attr() On the other hand, this scenario would be quite rare, right? If yes, then not sure it's worth adding this API.

dNechita avatar Dec 11 '25 13:12 dNechita

Is this a valid configuration? Can events/ dir contain an event that is device specific and not channel specific?

I think the thing is, we should likely not consider an event attribute as a device attribute but as an event attribute which is (often) itself associated to a specific channel.

When I was looking into it I was trying to make scan_type an attribute, so it would be read like the others

Not sure that is a good idea. We now have multi buffer support in the kernel where in theory (there is nothing preventing it) you can have all channels (which are scan elements) in all buffers. Therefore you would replicate all of those scan_elements as attributes. The scan_element is a property of the channel so I think that having it as we have now is not that bad. Currently you already read what matters in the format format: be:s16/32>>0

Having said the above, multi buffer support is by no means supported in libiio and it will take some effort. Something I plan to do in the beginning of the next year

nunojsa avatar Dec 11 '25 18:12 nunojsa