Skip to content
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

iio.h: Include iio/types.h when it exists #763

Closed

Conversation

gwendalcr
Copy link
Contributor

enum iio_chan_type and iio_modifier are already defined in
/usr/include/linux/iio/types.h on linux systems.
If an application needs to include both (to support iio events for
instance), compiler will complains these enums are defined twice.

This patch is not without problem:

  • it works only if compiler supports __has_include (C++17, not 11).
  • linux system must have latest kernel-headers installed:
    IIO_MOD_O2 modifier has been added to linux 5.9 kernel.

Alter CI/travis/check_kernel.sh accordingly, check it still passes.

Fixes #758.

Signed-off-by: Gwendal Grignou [email protected]

Copy link
Contributor

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor comment from side... Alternatively to this we could just think about syncing the current iio uapi within libiio on every stable release . Some other projects (eg: libiouring) apparently do the same... However I'm not sure how practical that is given some potential customs things we have. @pcercuei might have a better idea for this issue

iio.h Outdated
@@ -84,6 +84,12 @@ struct iio_context_info;
struct iio_scan_context;
struct iio_scan_block;

#define IIO_CHAN_TYPE_UNKNOWN INT_MAX

#if __has_include(<linux/iio/types.h>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, according to this, the right way of doing this is to first test for the existence of the macro... I'm not sure this will actually build when the compiler does not support __has_include

@rgetz
Copy link
Contributor

rgetz commented Nov 16, 2021

don't we just want to do a:

# ensure that kernel headers are included before libiio headers
#ifndef iio_chan_type

enum ....

#endif

this would resolve things, but only if you included kernel headers before iio.h

@pcercuei
Copy link
Contributor

@rgetz it's not a macro, though.

@gwendalcr please wrap the __has_include macro as described in here: https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005finclude.html

enum iio_chan_type and iio_modifier are already defined in
/usr/include/linux/iio/types.h on linux systems.
If an application needs to include both (to support iio events for
instance), compiler will complains these enums are defined twice.

This patch is not without problem:
- linux system must have latest kernel-headers installed:
IIO_MOD_O2 modifier has been added to linux 5.9 kernel.

Alter CI/travis/check_kernel.sh accordingly, check it still passes.

Fixes analogdevicesinc#758.

Signed-off-by: Gwendal Grignou <[email protected]>
@gwendalcr
Copy link
Contributor Author

I really don't like this patch, it is an ugly band aid:

  1. wrapping __has_include will help pass some tests, but an application including both kernel include and iio.h will not be portable anymore between C++ 11 and 17.
  2. sync'ing uapi does not work for [embedded] devices running an older kernel. We are not able to use the latest libiio when cros compiling for an older kernel.

In another project, to fill iio_chan_type_name_spec[] and modifier_names[] type arrays, X-Macros have been used, but that would require using the macros in the kernel includes as well.

@pcercuei
Copy link
Contributor

What about we sync to the latest uapi header now, then protect libiio's copy with the kernel header's _IIO_TYPES_H_? That sounds like the simplest and safest solution.

@pcercuei
Copy link
Contributor

@gwendalcr could you try 5fc7550?

@gwendalcr
Copy link
Contributor Author

5fc7550 is a better patch.

@gwendalcr gwendalcr closed this Nov 29, 2021
@gwendalcr gwendalcr deleted the fix_double_include_once branch November 29, 2021 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redefinition errors between libiio and kernel iio include files
4 participants