Fix bug in mask bit tests #198

dpfrey · 2018-09-28T19:02:39Z

Previously, the index field of struct iio_channel was being used to test
against the buffer field of struct iio_device and struct iio_buffer.
This is incorrect because the index of the first channel may be non-zero
and the indexes may not be contiguous. For further evidence, look at
iio_channel_enable/disable() which use number rather than index.

pcercuei · 2018-09-28T20:05:44Z

The channels are sorted by indexes so the first one should be zero and they should be contiguous, unless I'm missing something. What's your setup?

Previously, the index field of struct iio_channel was being used to test against the buffer field of struct iio_device and struct iio_buffer. This is incorrect because the index of the first channel may be non-zero and the indexes may not be contiguous. For further evidence, look at iio_channel_enable/disable() which use number rather than index. Signed-off-by: David Frey <dfrey@sierrawireless.com>

dpfrey · 2018-09-28T20:21:27Z

If you look at drivers/iio/imu/bmi160/bmi160_core.c, you will see that it declares enum bmi160_scan_axis and values 0 through 3 are occupied by definitions which correspond to a bmm150 magnetometer downstream of the bmi160. The driver doesn't currently support the bmm150. So when the driver declares an array of type struct iio_chan_spec, the first element in the array has scan_index == BMI160_SCAN_ACCEL_X. BMI160_SCAN_ACCEL_X has a value of 4. As for why the indicies would be discontiguous... I don't know. I just can't find any documentation in the kernel that indicates that they must be contiguous.

This is the only documentation I can find:

Here scan_index defines the order in which the enabled channels are placed inside the buffer. Channels with a lower scan_index will be placed before channels with a higher index. Each channel needs to have a unique scan_index.

Setting scan_index to -1 can be used to indicate that the specific channel does not support buffered capture. In this case no entries will be created for the channel in the scan_elements directory

lclausen-adi · 2018-09-29T11:10:08Z

@pcercuei 4792fa3

pcercuei · 2018-09-30T13:20:51Z

Sorry, I forgot about that. I guess it makes sense then.

lclausen-adi · 2018-10-01T11:16:37Z

Do we need the same fix in iiod/ops.c when server side demuxing is used?

dpfrey · 2018-10-01T14:53:36Z

@lclausen-adi: Are you referring to the send_sample() and receive_sample() functions? They definitely look suspicious. I didn't do an exhaustive search through the codebase for similar defects. I made the change in iio_buffer_foreach_sample() because I was blocked from reading a sensor properly by that issue. I made the change in iio_buffer_first() because I happened to notice it while making the first change.

lclausen-adi · 2018-10-01T15:54:30Z

@dpfrey Yes. I think those also need to be fixed. But I'd like @pcercuei to confirm this.

Either way your patch looks good. Thanks for fixing this issue.

pcercuei · 2018-10-03T09:20:20Z

@lclausen-adi: The channel masks must match on both ends. I think this PR is dangerous, we should be extra-careful. It will definitely break ABI in the case where several channels share one index (think M2K's logic analyzer device).

pcercuei · 2018-10-03T09:29:21Z

I think to make it work properly, the channel mask within the iio_buffer should be a bitmask of the indices, and the mask within the iio_device should be a bitmask of the channel numbers.

lclausen-adi · 2018-10-03T09:43:55Z

I don't see why the code in iio_buffer_foreach_sample() should match the code in iio_device_get_sample_size_mask(). Both in the end compute the same thing, whereas iio_buffer_foreach_sample() just does a few intermediary stops.

lclausen-adi · 2018-10-03T09:45:29Z

Which reveals another bug in iio_buffer_foreach_sample(). It does not take the repeat property of the channel into account.

We should factor that code into a common helper function.

Edit: Forget it, it does. But we should still factor this out.

pcercuei · 2018-10-03T09:54:07Z

Speaking about iio_device_get_sample_size_mask, it's the only blocker that prevents us to compile IIOD with the public libiio API. It would be nice to decouple them.

pcercuei · 2018-10-13T07:02:33Z

@dpfrey: Could you fix it also in iio_buffer_foreach_sample, and (in a separate commit) in IIOD's send_sample/receive_sample? Thanks!

From: #198 Previously, the index field of struct iio_channel was being used to test against the buffer field of struct iio_device and struct iio_buffer. This is incorrect because the index of the first channel may be non-zero and the indexes may not be contiguous. For further evidence, look at iio_channel_enable/disable() which use number rather than index. Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

mhennerich · 2018-10-30T11:36:30Z

please see #211 - this is untested

From: #198 Previously, the index field of struct iio_channel was being used to test against the buffer field of struct iio_device and struct iio_buffer. This is incorrect because the index of the first channel may be non-zero and the indexes may not be contiguous. For further evidence, look at iio_channel_enable/disable() which use number rather than index. Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

commodo · 2018-10-31T08:49:40Z

Will merge this later today if no objections together with #211

commodo · 2018-10-31T08:50:26Z

pcercuei · 2018-10-31T11:39:13Z

There is an objection :)
Please add the same fix to iio_buffer_foreach_sample.

From: #198 Previously, the index field of struct iio_channel was being used to test against the buffer field of struct iio_device and struct iio_buffer. This is incorrect because the index of the first channel may be non-zero and the indexes may not be contiguous. For further evidence, look at iio_channel_enable/disable() which use number rather than index. Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

mhennerich · 2018-10-31T11:55:25Z

There is an objection :)
Please add the same fix to iio_buffer_foreach_sample.

I added a fix for this to this PR:
#211

From: #198 Previously, the index field of struct iio_channel was being used to test against the buffer field of struct iio_device and struct iio_buffer. This is incorrect because the index of the first channel may be non-zero and the indexes may not be contiguous. For further evidence, look at iio_channel_enable/disable() which use number rather than index. Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

Changelog: 20c9079 CI/travis/inside_bionic_docker.sh: re-use the make_linux logic cd1be7d build,travis: parametrize library name 2b4503d README.md : Update with links to doc cf87e0a travis-ci: install graphviz 2f3632f doxygen: check if building on a case senstive file system. a18ff6e doxygen: If dot exists, use it. cb28721 appveyor: install graphviz and fix doxygen version 550f4b8 README: Update with latest build changes 540c96b deployment : export the LDIST var which is needed in other scripts de8356b fix snprintf warnings from gcc 8 24c7f00 .travis.yml: Add new Xcode versions to the Travis CI builds. d65cabb CI/travis/before_install_darwin: Handle brew failures when installing packages. 29c582e .travis.yml: Drop the Travis CI builds for OSX Image xcode 6.4. 6799876 .travis.yml: Drop the Travis CI builds for Ubuntu 12.04 LTS (Precise Pangolin). f18ed59 .travis.yml: Add support for Ubuntu 18.04 (Bionic) builds. 983657a cmake/LinuxPackaging.cmake: Add libserialport to the list of dependencies that are being handled when creating Linux packages. 25c20d6 channel: Fix #219 get_modifier and get_type seems to work incorrectly ed6709e sorting: ensure sorting happens after global attributes are added f05434e travis-ci: don't hard code distributions anymore 9b75895 Revert "sort: Move channel attribute sorting to context creation" fix #215 cf39834 cmake: set the CPACK_DEBIAN_PACKAGE_ARCHITECTURE for old versions of Cmake 76d4ff7 buffer: Fix bug in mask bit tests (continued) b7407af IIOD: Fix bug in mask bit tests (continued) 37ecd2e Update README.md with newest centos packages 5dd1ff9 add note about triggered buffers in dox source 98d85f6 local: pass errors up the stack 879abfe usb: Increase ctrl pipe timeout da13ffc usb: add libusb version to context attributes 876db45 usb: be more verbose when unable to claim an interface 686ced9 travis : Add the LDIST for centos db47744 Ensure iio_info can find a locally installed libiio.so c5973cf fix whitespace damage from previous commits 67a994c sort: when sorting iio_channels, if the index is the same, use ID b9008a7 sort: Move channel attribute sorting to context creation 8405704 sort: change function names to be more descriptive/accurate afd6d69 Update FIR enable function in ML bindings to not force sample rate ahead of filter write. 53bfb03 local: Sort devices, channels and attributes when adding them. 256a80a appveyor.yml: Downgrade curl from 7.61.1-3 to 7.61.1-2 15ddcd6 Fix FIR load function in ML bindings to actually enable the FIR once loaded. 4c9a050 CI/travis/inside_centos_docker.sh: hack/patch CPackRPM.cmake for CentOS 7 f60f957 cmake/LinuxPackaging.cmake: use non-dev packages to .deb dep list ed6d860 CI/travis/make_linux: install deb package as final test cfe093a build,CI/travis: setup CentOS testing 4a39cb6 CI/travis: enable errexit & xtrace behavior in scripts 77a1154 CI/travis/before_deploy: move `grep` expression in `find` a05d607 .gitignore: add vim swap files 0b23cbe spelling fonction->function ee936e8 Fix bug in mask bit tests c80412c cmake: Suppress errors when looking up the git repository path 2a76c2e .travis.yml: add host-key algo ssh-dss for xenial deploy b853fdb appveyor.yml: change versioning to '{branch}.{build}' format 0950037 CI/travis/deploy: extend cleanup to all debian packages 2b1c4b8 README.md: add Xenial artifact links ea80423 .travis.yml: add Xenial distro to job run Alexandra Trifan (1): appveyor.yml: Downgrade curl from 7.61.1-3 to 7.61.1-2 . Alexandra.Trifan (6): cmake/LinuxPackaging.cmake: Add libserialport to the list of dependencies that are being handled when creating Linux packages. .travis.yml: Add support for Ubuntu 18.04 (Bionic) builds. .travis.yml: Drop the Travis CI builds for Ubuntu 12.04 LTS (Precise Pangolin). .travis.yml: Drop the Travis CI builds for OSX Image xcode 6.4. CI/travis/before_install_darwin: Handle brew failures when installing packages. .travis.yml: Add new Xcode versions to the Travis CI builds. Alexandru Ardelean (14): .travis.yml: add Xenial distro to job run README.md: add Xenial artifact links CI/travis/deploy: extend cleanup to all debian packages appveyor.yml: change versioning to '{branch}.{build}' format .travis.yml: add host-key algo ssh-dss for xenial deploy .gitignore: add vim swap files CI/travis/before_deploy: move `grep` expression in `find` CI/travis: enable errexit & xtrace behavior in scripts build,CI/travis: setup CentOS testing CI/travis/make_linux: install deb package as final test cmake/LinuxPackaging.cmake: use non-dev packages to .deb dep list CI/travis/inside_centos_docker.sh: hack/patch CPackRPM.cmake for CentOS 7 build,travis: parametrize library name CI/travis/inside_bionic_docker.sh: re-use the make_linux logic David Frey (2): Fix bug in mask bit tests spelling fonction->function Lars-Peter Clausen (2): cmake: Suppress errors when looking up the git repository path usb: Increase ctrl pipe timeout Michael Hennerich (15): Merge pull request #175 from analogdevicesinc/sort Merge pull request #203 from analogdevicesinc/sort1 Merge pull request #206 from analogdevicesinc/rgetz-patch-1 Merge pull request #201 from analogdevicesinc/fix-ml-bindings Merge pull request #207 from analogdevicesinc/rgetz-patch-2 IIOD: Fix bug in mask bit tests (continued) buffer: Fix bug in mask bit tests (continued) Merge pull request #198 from mangOH/mask_fix_bug Merge pull request #211 from analogdevicesinc/iiod-mask-fix-bug Merge pull request #212 from analogdevicesinc/rgetz-patch-2 Merge pull request #213 from analogdevicesinc/rgetz-patch-3 Merge pull request #214 from analogdevicesinc/rgetz-patch-4 Revert "sort: Move channel attribute sorting to context creation" fix #215 channel: Fix #219 get_modifier and get_type seems to work incorrectly Merge pull request #224 from analogdevicesinc/rft-issue-219 Robin Getz (23): local: Sort devices, channels and attributes when adding them. sort: change function names to be more descriptive/accurate sort: Move channel attribute sorting to context creation sort: when sorting iio_channels, if the index is the same, use ID fix whitespace damage from previous commits Ensure iio_info can find a locally installed libiio.so travis : Add the LDIST for centos usb: be more verbose when unable to claim an interface usb: add libusb version to context attributes local: pass errors up the stack add note about triggered buffers in dox source Update README.md with newest centos packages cmake: set the CPACK_DEBIAN_PACKAGE_ARCHITECTURE for old versions of Cmake travis-ci: don't hard code distributions anymore sorting: ensure sorting happens after global attributes are added fix snprintf warnings from gcc 8 deployment : export the LDIST var which is needed in other scripts README: Update with latest build changes appveyor: install graphviz and fix doxygen version doxygen: If dot exists, use it. doxygen: check if building on a case senstive file system. travis-ci: install graphviz README.md : Update with links to doc Travis Collins (2): Fix FIR load function in ML bindings to actually enable the FIR once loaded. Update FIR enable function in ML bindings to not force sample rate ahead of filter write. Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

dpfrey force-pushed the mask_fix_bug branch from c988383 to ee936e8 Compare September 28, 2018 20:06

lclausen-adi approved these changes Oct 1, 2018

View reviewed changes

mhennerich mentioned this pull request Oct 30, 2018

IIOD: Fix bug in mask bit tests (continued) #211

Merged

commodo approved these changes Oct 31, 2018

View reviewed changes

mhennerich merged commit a69f252 into analogdevicesinc:master Oct 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in mask bit tests #198

Fix bug in mask bit tests #198

dpfrey commented Sep 28, 2018

pcercuei commented Sep 28, 2018

dpfrey commented Sep 28, 2018

lclausen-adi commented Sep 29, 2018

pcercuei commented Sep 30, 2018

lclausen-adi commented Oct 1, 2018

dpfrey commented Oct 1, 2018

lclausen-adi commented Oct 1, 2018

pcercuei commented Oct 3, 2018

pcercuei commented Oct 3, 2018

lclausen-adi commented Oct 3, 2018

lclausen-adi commented Oct 3, 2018 •

edited

Loading

pcercuei commented Oct 3, 2018

pcercuei commented Oct 13, 2018

mhennerich commented Oct 30, 2018

commodo commented Oct 31, 2018

commodo commented Oct 31, 2018

pcercuei commented Oct 31, 2018

mhennerich commented Oct 31, 2018

Fix bug in mask bit tests #198

Fix bug in mask bit tests #198

Conversation

dpfrey commented Sep 28, 2018

pcercuei commented Sep 28, 2018

dpfrey commented Sep 28, 2018

lclausen-adi commented Sep 29, 2018

pcercuei commented Sep 30, 2018

lclausen-adi commented Oct 1, 2018

dpfrey commented Oct 1, 2018

lclausen-adi commented Oct 1, 2018

pcercuei commented Oct 3, 2018

pcercuei commented Oct 3, 2018

lclausen-adi commented Oct 3, 2018

lclausen-adi commented Oct 3, 2018 • edited Loading

pcercuei commented Oct 3, 2018

pcercuei commented Oct 13, 2018

mhennerich commented Oct 30, 2018

commodo commented Oct 31, 2018

commodo commented Oct 31, 2018

pcercuei commented Oct 31, 2018

mhennerich commented Oct 31, 2018

lclausen-adi commented Oct 3, 2018 •

edited

Loading