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

drivers: adc: fix warning when using ADC_DT_SPEC_GET macro on C++ #69419

Conversation

ArchieAtkinson
Copy link

@ArchieAtkinson ArchieAtkinson commented Feb 24, 2024

Modifies the ADC_CHANNEL_CFG_DT macro to always initialise the differential property to prevent the -Wmissing-field-initializers warning when using ADC_DT_SPEC_GET macros in C++ when not using a differential ADC.

@zephyrbot zephyrbot added the area: ADC Analog-to-Digital Converter (ADC) label Feb 24, 2024
@zephyrbot zephyrbot requested a review from anangl February 24, 2024 18:35
Copy link

Hello @ArchieAtkinson, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

A similar, recent change broke one of our toolchains:

Please hold this back until the corresponding policy has been discussed and approved and this flag added in CI.

This policy should be either enforced everywhere or nowhere.

See also:

@ArchieAtkinson ArchieAtkinson force-pushed the adc_channel_cfg_dt_cpp_fix branch 3 times, most recently from 8b1a9b3 to aff8bd0 Compare February 26, 2024 21:24
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 2, 2024

Enforcing this policy could be as simple as making sure CI compiles every .h file with g++ -std=c++20 at least once.

https://github.com/zephyrproject-rtos/zephyr/pull/69486/files explains why.

This would of course catch ALL OTHER C++ compatibility issues, not just this initializer warning.

@ArchieAtkinson
Copy link
Author

Enforcing this policy could be as simple as making sure CI compiles every .h file with g++ -std=c++20 at least once.

https://github.com/zephyrproject-rtos/zephyr/pull/69486/files explains why.

This would of course catch ALL OTHER C++ compatibility issues, not just this initializer warning.

As someone who uses Zephyr with C++20 in production code, I would love to see these compatibility issues solved and be part of the effort to do so. However, I think there would have to be an appetite for it within the project as macros like CONTAINER_OF() that are widely used trigger C++ warnings.

@ArchieAtkinson ArchieAtkinson force-pushed the adc_channel_cfg_dt_cpp_fix branch from aff8bd0 to 54092f3 Compare March 2, 2024 16:20
Modifies the `ADC_CHANNEL_CFG_DT` macro to always initialise the
`differential` property to prevent the `-Wmissing-field-initializers`
warning when using `ADC_DT_SPEC_GET` macros in C++ when not using a
`differential` ADC.

Signed-off-by: Archie Atkinson <archieatkinson97@gmail.com>
@ArchieAtkinson ArchieAtkinson force-pushed the adc_channel_cfg_dt_cpp_fix branch from 54092f3 to f2a6c20 Compare March 2, 2024 16:22
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 10, 2024

Enforcing this policy could be as simple as making sure CI compiles every .h file with g++ -std=c++20 at least once.

This could be as simple as compiling a file with a lot of #include in tests/lib/cpp. You should give it a try.

EDIT: except... macros. Unused C/C++ macros are parsed but obviously not "compiled" unless you use them :-(

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 20, 2024

This could be as simple as compiling a file with a lot of #include in tests/lib/cpp. You should give it a try.

It's a bit more complicated because unused macros are parsed but not compiled.

It can be done though, see how I'm adding C++ compilation of device.h in #70403 with just one additional line in tests/lib/cpp/cxx/src/main.cpp

See if you can actually compile ADC_CHANNEL_CFG_DT() in C++ using some tests/ change too.

EDIT: I thoroughly documented this type of compatibility issue between C and C++ in:

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 20, 2024
@github-actions github-actions bot closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ADC Analog-to-Digital Converter (ADC) area: C++ Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants