-
Notifications
You must be signed in to change notification settings - Fork 320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
local: Sort devices, channels and attributes #175
Conversation
local.c
Outdated
@@ -1183,7 +1183,7 @@ static int read_device_name(struct iio_device *dev) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: comment title is a bit long; it could be trimmed to local: Sort devices, channels and attributes when adding them
git-style comments prefer an empty between title & description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah; and the description shouldn't be indented ;
local.c
Outdated
|
||
for (i = chn->nb_attrs - 1; i > 0; i--) { | ||
if (strcmp(attrs[i].name, attrs[i-1].name) < 0) { | ||
tmp1 = attrs[i].filename; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for this case particularly, I'd use a struct iio_channel_attr tmp;
and memcpy() stuff onto it and back with sizeof(tmp)
it's one variable, and can handle future cases [new members on the struct iio_channel_attr
] ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a continuation here [and while thinking about it a bit], an idea would be to do a search for the index/position where the new element could be added;
then after the realloc
memmove() everything +1 position to the right;
so, something like:
int insert_idx = 0;
for (i = 0; i < chn->nb_attrs - 1; i--) {
if (strcmp(attrs[i].name, attrs[i+1].name) >= 0)
break
}
insert_idx = i; /* or just use `i` directly */
attrs = realloc(chn->attrs, (1 + chn->nb_attrs) * sizeof(struct iio_channel_attr));
memmove(attrs[i+1], attrs[i], chn->nb_attrs - i);
chn->nb_attrs++;
i don't have a strong preference about doing it either way;
@pcercuei - yes - we have actually seen this happen when adding features to drivers, or adding drivers to a platform - things get exposed in a different order, and anything that is hard coded breaks. I don't see this as any different. If you want me to add something to the doc that says explicitly that order is not guaranteed - I can do that. |
How about using qsort? |
I was also thinking about qsort ; I'm curious if qsort is available outside of glibc [GNU context] |
"The qsort() function conforms to SVr4, 4.3BSD, C89, C99." |
Cool :) |
OK - I will update things to use qsort. (I checked, and like Lars indicated it is in glibc, uClibc, and bionic). It's missing from many embedded C libraries, but I don't think that is a problem - people using that should be using libtinyiiod (at https://github.com/analogdevicesinc/libtinyiiod ) Thanks for feedback |
Ok - better late than never - I wanted to have this reviewed before we get together next week, since it helps point out some issues I was looking at/puzzling about.. Updated to use qsort, let me know if there are other issues. |
local.c
Outdated
const char *tmp1 = *(const char **)p1; | ||
const char *tmp2 = *(const char **)p2; | ||
|
||
return strcmp(tmp1, tmp2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a MAX value/definition for these so that strncmp() can be used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so - this is the max path that the kernel knows about. @larsclausen or @pcercuei would know for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These strings will always be NULL-terminated, so strcmp() is fine.
Channel re-ordering is already done here: https://github.com/analogdevicesinc/libiio/blob/master/context.c#L233 |
Note: the current reorder_channels( ) function sorts the channels by their index when applicable, I think that should take precedence over their ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If there are no objections I will merge this later today [or tomorrow].
Given Paul's feedback, I don't think this is ready yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those qsorts should run only once, when all attributes have been added. Not every time a new attribute is added.
local.c
Outdated
@@ -1575,6 +1618,14 @@ static int detect_and_move_global_attrs(struct iio_device *dev) | |||
return 0; | |||
} | |||
|
|||
static int cmp_iio_buffer_attr(const void *p1, const void *p2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to re-use the same function for all which just do strcmp().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if I should do that - in case the structures changed later...
-Robin
local.c
Outdated
@@ -1181,6 +1181,14 @@ static int read_device_name(struct iio_device *dev) | |||
return 0; | |||
} | |||
|
|||
static int cmp_iio_device_attr(const void *p1, const void *p2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most functions in this file are prefixed local_, would be good to continue this pattern.
E.g. local_compare_device_attr().
ah ; sorry for the rush |
I think I have all @larsclausen requests in. @pcercuei : still need to move things to the context.c - wanted to chat about how to sort channels first. when sorting by ID, you get: root@analog:/usr/local/src/libiio-test/build# iio_attr -a -c . isn't that better than random voltagexxx numbers? (which is what happens today, when you sort it by index). ? |
It looks better, maybe. But I'm pretty sure some other part of libiio or iiod expect the channels to be sorted by index, so we have to be very cautious here. |
Ok - I had a brief look, but didn't see anything - but you know the code base better than I - I will change it /update it tomorrow. Any other comments? |
And any objections if I move the qsort callback/comparison functions to iio-private.h? It might be more obvious to the reader what is going on if they were defined/included with the structures, and it was just the calls to qsort that were added to local.c and context.c ? |
Ok - update 3, I think this has Paul's and Lar's requests in. -Robin |
One function that will break if the order of channels changes, is iio_buffer_foreach_sample( ), it exits the loop as soon as (chn->index < 0) is reached. I guess it would be enough to replace the "break" with a "continue" there. |
Current Master version of sort: analog@analog:~$ iio_attr -a -c cf-ad9361-dds-core-lpc new code, based on this branch: analog@analog:~$ iio_attr -a -c cf-ad9361-dds-core-lpc which I think is OK? (buffers first, index matches the element). |
Looks good. Could you move the qsort* functions to a qsort.c file, and their prototypes to qsort.h? |
Ok - done. and refactored the channel sorting to ensure it's done by index when buffer capable. Let me know if there are any other tweaks necessary. |
@pcercuei : sorry for the copy/paste error on your name. I took it out, since like you said - your email isn't valid (which is something else - did you want me to set those to something valid now? - there are 46 instances of the old email). @larsclausen : any other comments/fixes? So, this is Ok to merge now? -Robin |
I'd call the file |
In this way, other iio utilities will output information in an easy to read/compare way. This has no functionality difference, except to add a small negative performance, but it makes the output of iio_attr and iio_info, much easier to track by hand. Signed-off-by: Robin Getz <[email protected]>
updated. |
Looks good to me. |
LGTM let's merge things now. |
1 similar comment
LGTM let's merge things now. |
LGTM let's merge things now |
LGTM let's merge things now |
#ifndef __IIO_QSORT_H__ | ||
#define __IIO_QSORT_H__ | ||
|
||
int qsort_iio_channel(const void *p1, const void *p2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better name would be iio_channel_compare() and so on... That would be a descriptive name that explains what the function does.
@mhennerich : Did you want to revert the commit, and have me change the function names like Lars suggested? |
@rgetz Follow up patch please. But wait a day or two. github is a bit broken today, I was not able to review things in full. |
Ok - will do. I also just noticed an issue I will fix at the same time. (channel attributes aren't always sorted properly for some reason). |
@lclausen-adi : Any other comments before I send the next patch? |
@@ -1761,12 +1767,18 @@ static int create_device(void *d, const char *path) | |||
free_protected_attrs(chn); | |||
if (ret < 0) | |||
goto err_free_scan_elements; | |||
|
|||
qsort(chn->attrs, chn->nb_attrs, sizeof(struct iio_channel_attr), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space here after chan->attrs,
} | ||
|
||
ret = detect_and_move_global_attrs(dev); | ||
if (ret < 0) | ||
goto err_free_device; | ||
|
||
qsort(dev->attrs, dev->nb_attrs, sizeof(char *), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well
if (iio_channel_is_scan_element(tmp1) && iio_channel_is_scan_element(tmp2)){ | ||
if (iio_channel_get_index(tmp1) > iio_channel_get_index(tmp2)) | ||
return 1; | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to fallback to id if the index is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good.
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Robin Getz [email protected]