Regression with iio_channel_attr_read_all() after recent channel sorting enhancements

[mhennerich]

commit b9008a7870fe3b47cdd1d206ce20eea423fed8a3 sort: Move channel attribute sorting to context creation

Introduced a regression in case local and remote libiio version differs (has this commit or not) Then iio_channel_attr_read_all() would read the wrong value for attribute.

michael@mhenneri-D06:~/devel/git/iio-oscilloscope$ git diff
diff --git a/iio_widget.c b/iio_widget.c
index f2b2021..16ee7a4 100644
--- a/iio_widget.c
+++ b/iio_widget.c
@@ -425,6 +425,7 @@ static int __cb_chn_update(struct iio_channel *chn, const char *attr,
                struct iio_widget *widget = &params->widgets[i];
                if (widget->update_value && widget->chn == chn &&
                                !strcmp(widget->attr_name, attr)) {
+                       printf("--- found %s %s %s value %s\n", __func__, widget->attr_name, attr, value);
                        widget->update_value(widget, value, len);
                        return 0;
                }

--- found cb_chn_update external external value [325000000 1 3800000000] --- found cb_chn_update frequency frequency value 2400999998 --- found cb_chn_update external external value 0 --- found cb_chn_update frequency frequency value 128 248,248,248,248,248,248,248,248,248,248,248,248,248,248,248,248 --- found cb_chn_update hardwaregain hardwaregain value -10.000000 dB **--- found cb_chn_update rf_bandwidth rf_bandwidth value [0 250 89750]** --- found cb_chn_update rf_port_select rf_port_select value A B --- found cb_chn_update sampling_frequency sampling_frequency value [200000 1 40000000] --- found cb_chn_update bb_dc_offset_tracking_en bb_dc_offset_tracking_en value [-3 1 71] --- found cb_chn_update gain_control_mode gain_control_mode value 53.00 dB --- found cb_chn_update hardwaregain hardwaregain value manual --- found cb_chn_update quadrature_tracking_en quadrature_tracking_en value 200000 --- found cb_chn_update rf_bandwidth rf_bandwidth value 1 --- found cb_chn_update rf_dc_offset_tracking_en rf_dc_offset_tracking_en value 1 --- found cb_chn_update rf_port_select rf_port_select value 10999998 --- found cb_chn_update sampling_frequency sampling_frequency value [200000 1 56000000] --- found cb_chn_update sampling_frequency sampling_frequency value 10999998 1374999 --- found cb_chn_update sampling_frequency sampling_frequency value 10999998 1374999

IMHO we need to make sure this sorting stuff doesn't break compatibility!

[pcercuei]

I think we should revert that one commit then. The network/USB backends will then list their attributes according to how they appear in the XML, sorted or not, depending on whether the qsort commits are in the remote libiio or not.

[rgetz]

I think this is a bug in osc, not in libiio; and the sort should go back in.

We said people shouldn't blindly be writing channels/attributes via indexes without checking names/ID, and that deserves to break. As far as I can tell - that is what osc is doing.

A change in the kernel driver, which effects the xml will cause osc to break just the same.

-Robin

[mhennerich]

I think this is a bug in osc, not in libiio; and the sort should go back in.

We said people shouldn't blindly be writing channels/attributes via indexes without checking names/ID, and that deserves to break. As far as I can tell - that is what osc is doing.

A change in the kernel driver, which effects the xml will cause osc to break just the same.

No it's not. There is no hard-coded indexes, etc. That's existing libiio API. Take a look how iio_channel_attr_read_all() and friends interact across the backends. Sorting must be the same, or it fails. The only way is to don't break compatibility and sort only on the local backend.

-Michael

[rgetz]

Maybe I'm missing something - but I couldn't recreate a problem when I had a blank ini file.

How exactly does the problem manifest itself?

I have : host IIO : 0.15-9b75895 (tip of master) - which has the sort reverted remote IIO : 0.15-ea597c4 (just one reverted) - with the sort.

and osc works fine for me.

-Robin

[mhennerich]

osc ini file a nothing todo with this. Take a release PlutoSDR which is libiio tag v0.15 Use a remote system - with this patch applied - (The one which I reverted today). Things fail, not because OSC does something wrong - only because remote and local do things differently. And that's what we want to avoid!

-Michael

[mhennerich]

how it's manifest? - take a look at the original bug report. libiio reads wrong values for matching channels and attributes. Because the iio_channel_attr_read_all() and friends were invented to cut down turnaround times by reading attributes not one by one, but instead transferring them in bulk from the local to the remote backend. This causes the compatibility issue, since things need to be sorted the same way...

The whole thing is only a problem with these bulk transactions ...

[rgetz]

this bug report?

or #203 where you describe things as "some attribute widgets are just empty."

I don't see that with my configuration. host IIO : 0.15-9b75895 (tip of master) - which has the sort reverted remote IIO : 0.15-ea597c4 (just one reverted) - with the sort.

Now you told me what to test - if I move the host osc to 0.15-ea597c4 (head), and set something to a super old pluto (0.10-v0.10) I can reproduce the problem you are seeing. It's only an issue if you have a newer libiio (sorting in context.c); talking to an old (0.15 or pre) libiio (not sorting at all).

After playing with things for awhile - You are correct (sorry for doubting you). I will follow up with a few more patches to libiio, and osc though.

[mhennerich]

Honestly - patches can't fix this...and it's not worth the effort to spend time on it.

There is one thing we want to maintain - which is compatibility. Fixing bugs is differently - but introducing a new feature should not break things.

Right now - if the local libiio is new it will sort things which is a feature/enhancement. Everything works expectedly, but if not a older remote will still work, that's compatibility.

[rgetz]

patches can fix the sorting - (it's not sorting in the right place in local.c), that is one of the reasons I moved things. patches can report the remote iio version in osc (that's not done now). Those were the patches I was talking about.

I agree with the compatibility comments - I never said compatibility wasn't important.