device.h: C++20 compatibility #70403

marc-hb · 2024-03-19T04:42:10Z

2 fixes + 2 corresponding tests/ commits, please check commit messages.

Apply to device.h and pm/device.h the C/C++ compatibility lessons learned for init.h in #69411

gmarull · 2024-03-19T17:35:07Z

include/zephyr/device.h

+ * support for anonymous unions but they require these braces when
+ * combined with C99 designated initializers. These braces are
+ * compatible with any C version but not with C++20.  For more obscure
+ * C/C++ compatibility details see commit c15f029a7108 and PR #69411.


please avoid referring to PRs in documentation

I usually do avoid this but this issue is really complex (even when the corresponding code is not) and that PR discussion is quite important.

PRs content is not part of Zephyr git history. Who knows if Github will stay Zephyr's VCS provider forever.

I'm with Gerard on this, just refer to the commit, one can find back the PR out of band but really the commit should have enough context regardless

If you really want to link a PR maybe you can do it on this comment using the Link: tag and the full URL?

Who knows if Github will stay Zephyr's VCS provider forever.

I'm aware and I always try to avoid references to PRs when the git history is enough. But I think there can be some exceptions. It's not because something may disappear one day that it should not be referenced. If Zephyr migrates away from Github one day, then it should definitely export all past review discussions and migrate or archive them somewhere and then PR numbers would still be useful. I've been performing such archeology in the past and any reference helps.

Of course documentation is even better but (1) it's not mutually exclusive (2) not obvious where in the docs this obscure issue belongs (3) more "overhead" :-)

PRs content is not part of Zephyr git history.

True but the git history does not exist in a complete vacuum either.

If you really want to link a PR maybe you can do it on this comment using the Link: tag and the full URL?

That would work for me.

Or I can just delete the PR number entirely if it's really important for you. I think that would be a loss but it's not that big of a deal for me.

Full link of external references in the commit I think are fine, Linux does it as well, it's more my opinion than a general rule but I think it's a good idea and I occasionally do it myself too.

gmarull · 2024-03-19T17:35:42Z

include/zephyr/device.h

 	}

+/*
+ * Anonymous unions require C11. Some pre-C11 gcc versions have early


this is repeating information, can’t it be put somewhere and reference it?

I tried to summarize it but I agree it's still a bit long and duplicated... any suggestion of a good place? I couldn't think of a really better way to document this sorry.

Maybe Zephyr coding guidelines? It looks like we'll need to apply this in other places if we want C++20 compatibility. An explanation there, plus a code snippet would be a solution, you could then just say "Refer to doc/xxx.rst"

Thanks, I'll take a look in the next couple weeks.

Done in

doc: languages: add new section "headers and C/C++ compatibility" #74047

marc-hb

Thx for the review! Answers below.

marc-hb · 2024-03-19T17:47:15Z

include/zephyr/device.h

 	}

+/*
+ * Anonymous unions require C11. Some pre-C11 gcc versions have early


I tried to summarize it but I agree it's still a bit long and duplicated... any suggestion of a good place? I couldn't think of a really better way to document this sorry.

marc-hb · 2024-03-19T17:47:18Z

include/zephyr/device.h

+ * support for anonymous unions but they require these braces when
+ * combined with C99 designated initializers. These braces are
+ * compatible with any C version but not with C++20.  For more obscure
+ * C/C++ compatibility details see commit c15f029a7108 and PR #69411.


I usually do avoid this but this issue is really complex (even when the corresponding code is not) and that PR discussion is quite important.

include/zephyr/device.h

marc-hb · 2024-06-17T14:45:30Z

Tentative v3.7.0 milestone because this PR is:

fixing C++ compatibility
adding tests.

Absolutely zero feature change.

nashif · 2024-06-27T02:14:11Z

@gmarull please review

keith-packard · 2024-06-27T04:40:01Z

include/zephyr/device.h

+#  define Z_DEVICE_INIT_ENTRY_DEV(node_id, dev_id)   Z_DEV_ENTRY_DEV(node_id, dev_id)
+#endif
+
+#define Z_DEV_ENTRY_DEV(node_id, dev_id)                                                           \


Odd to find this macro defined after it was "used" above. Not strictly required, of course.

This places the macro closer to the two locations where it's used.

It's also a lower-level implementation detail, so I think it it reads better last - that's subjective of course.

EDIT: I wrote too fast, I was referring to Z_DEVICE_INIT_ENTRY_DEV(). Then I wanted to preserve the "top-down" order for the next macro.

I think it's "odd" only if you're tainted by C compilation which requires "forward" declarations. Maybe dating all the way back to when compilation was performed in a single pass? Funny how the pre-processor is in some respect more advanced that C itself... https://stackoverflow.com/questions/3136686/is-the-c99-preprocessor-turing-complete

gmarull · 2024-06-27T08:19:42Z

include/zephyr/device.h

+ * more details see https://docs.zephyrproject.org/latest/develop/languages/cpp/
+ */
+#if defined(__STDC_VERSION__) && (__STDC_VERSION__) < 201100
+#  define Z_DEVICE_INIT_PM_BASE(pm_) ({ .pm_base = (pm_),})


isn't this missing a trailing ,?

Great catch, thank you @gmarull for the fix in #74970. Note this regression is only 3 weeks old and independent of this PR. My bad though for not noticing it when I rebased on top of it.

So I just:

rebased on top of your comma fix device: only define/initialize metadata for DT devices #74970

Added CONFIG_DEVICE_DT_METADATA to the prj.conf, on top of the other additions that were already in this PR. I checked it would have caught this bug.

Conditionally remove braces for designated initializers of anonymous unions. This makes it compatible with C++20 while not breaking pre-C11 gcc. This does for device.h what commit c15f029 ("init.h: restore designated initializers in SYS_INIT_NAMED()") did for init.h See https://docs.zephyrproject.org/latest/develop/languages/cpp/ and long discussion in zephyrproject-rtos#69411 for more obscure C/C++ compatibility details. Signed-off-by: Marc Herbert <[email protected]>

This provides C++ build test coverage for device.h (notably: Z_DEVICE_INIT() fixed in the previous commit) and for some other Device Tree macros. Signed-off-by: Marc Herbert <[email protected]>

Fixes the following error appearing in the test coverage added by the next commit: In file included from zephyr/tests/lib/cpp/cxx/src/main.cpp:18: ``` zephyr/include/zephyr/pm/device.h:275:9: error: designator order for field 'pm_device_base::state' does not match declaration order in 'pm_device_base' 275 | } | ^ zephyr/include/zephyr/pm/device.h:315:17: note: in expansion of macro 'Z_PM_DEVICE_INIT' 315 | Z_PM_DEVICE_INIT(Z_PM_DEVICE_NAME(dev_id), node_id, ``` Note this failure is observed with any g++ -std=c++NN standard value - even before C++20. Signed-off-by: Marc Herbert <[email protected]>

Note CONFIG_PM_* affects device.h too. Even if not compiled, the more code the pre-processor sees the better. Signed-off-by: Marc Herbert <[email protected]>

This option allows you to look up a struct device from any of the node labels that were attached to the devicetree node used to create the device, etc. This is helpful because node labels are a much more human-friendly set of unique identifiers than the node names we are currently relying on for use with device_get_binding(). Adding this infrastructure in the device core allows anyone to make use of it without having to replicate node label storage and search functions in various places in the tree. The main use case, however, is for looking up devices by node label in the shell. Since there is a footprint penalty associated with storing the node label metadata, leave this option disabled by default. Signed-off-by: Martí Bolívar <[email protected]>

marc-hb

Just force-pushed:

Rebased on top of comma fix device: only define/initialize metadata for DT devices #74970 fixing 3 weeks old regression from Allow specifying devices by DT node label in the shell #71912
Added CONFIG_DEVICE_DT_METADATA to the prj.conf, on top of the other CONFIG_ additions that were already in this PR. It would have caught this "foreign" regression.

NO OTHER CHANGE.

Please re-review this very minor update quickly before I have to rebase on top of another regression and then another fix :-)

Sorry for the inconvenience.

marc-hb · 2024-07-03T00:44:30Z

include/zephyr/device.h

+#  define Z_DEVICE_INIT_ENTRY_DEV(node_id, dev_id)   Z_DEV_ENTRY_DEV(node_id, dev_id)
+#endif
+
+#define Z_DEV_ENTRY_DEV(node_id, dev_id)                                                           \


This places the macro closer to the two locations where it's used.

It's also a lower-level implementation detail, so I think it it reads better last - that's subjective of course.

EDIT: I wrote too fast, I was referring to Z_DEVICE_INIT_ENTRY_DEV(). Then I wanted to preserve the "top-down" order for the next macro.

I think it's "odd" only if you're tainted by C compilation which requires "forward" declarations. Maybe dating all the way back to when compilation was performed in a single pass? Funny how the pre-processor is in some respect more advanced that C itself... https://stackoverflow.com/questions/3136686/is-the-c99-preprocessor-turing-complete

marc-hb · 2024-07-03T00:50:27Z

include/zephyr/device.h

+ * more details see https://docs.zephyrproject.org/latest/develop/languages/cpp/
+ */
+#if defined(__STDC_VERSION__) && (__STDC_VERSION__) < 201100
+#  define Z_DEVICE_INIT_PM_BASE(pm_) ({ .pm_base = (pm_),})


Great catch, thank you @gmarull for the fix in #74970. Note this regression is only 3 weeks old and independent of this PR. My bad though for not noticing it when I rebased on top of it.

So I just:

rebased on top of your comma fix device: only define/initialize metadata for DT devices #74970

Added CONFIG_DEVICE_DT_METADATA to the prj.conf, on top of the other additions that were already in this PR. I checked it would have caught this bug.

marc-hb · 2024-07-08T20:46:31Z

[minor changes]
NO OTHER CHANGE.
Please re-review this very minor update quickly before I have to rebase on top of another regression and then another fix :-)
Sorry for the inconvenience.

Could I get just one of the previous approvers to take another, quick look? Thanks in advance!

marc-hb force-pushed the c++device branch from 3ee2efe to 31bc3b4 Compare March 19, 2024 14:26

marc-hb marked this pull request as ready for review March 19, 2024 16:21

zephyrbot added area: Power Management area: Device Model labels Mar 19, 2024

zephyrbot requested review from ceolin, dcpleung, gmarull, JordanYates, nashif, tbursztyka, teburd and tmleman March 19, 2024 16:21

zephyrbot assigned gmarull Mar 19, 2024

marc-hb requested review from fabiobaltieri, keith-packard and cfriedt March 19, 2024 16:24

marc-hb mentioned this pull request Mar 19, 2024

pm: use braces for .pm_base inside anonymous union initializer #69270

Merged

marc-hb requested a review from andyross March 19, 2024 16:32

gmarull reviewed Mar 19, 2024

View reviewed changes

marc-hb commented Mar 19, 2024

View reviewed changes

marc-hb force-pushed the c++device branch from 31bc3b4 to 70579ff Compare March 19, 2024 22:17

zephyrbot added the area: C++ label Mar 19, 2024

zephyrbot requested review from alexanderwachter and stephanosio March 19, 2024 22:18

marc-hb mentioned this pull request Mar 20, 2024

efr32xg24_dk2601b + CONFIG_PM does not compile #70466

Closed

marc-hb force-pushed the c++device branch 2 times, most recently from a19094b to db0326e Compare March 20, 2024 02:37

marc-hb mentioned this pull request Mar 20, 2024

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

Closed

fabiobaltieri requested a review from yperess March 20, 2024 11:32

fabiobaltieri reviewed Mar 20, 2024

View reviewed changes

include/zephyr/device.h Outdated Show resolved Hide resolved

marc-hb added this to the v3.7.0 milestone Jun 17, 2024

fabiobaltieri requested a review from aescolar June 18, 2024 00:45

stephanosio previously approved these changes Jun 18, 2024

View reviewed changes

aescolar added the bug The issue is a bug, or the PR is fixing a bug label Jun 18, 2024

cfriedt previously approved these changes Jun 21, 2024

View reviewed changes

marc-hb removed the Enhancement Changes/Updates/Additions to existing features label Jun 26, 2024

keith-packard reviewed Jun 27, 2024

View reviewed changes

gmarull reviewed Jun 27, 2024

View reviewed changes

marc-hb mentioned this pull request Jul 2, 2024

llext: documentation-related code changes #75181

Merged

marc-hb added 4 commits July 2, 2024 23:34

tests: cpp: add some C++ coverage for device.h / Device Tree

7902ce8

This provides C++ build test coverage for device.h (notably: Z_DEVICE_INIT() fixed in the previous commit) and for some other Device Tree macros. Signed-off-by: Marc Herbert <[email protected]>

tests: cpp: add C++ coverage for pm/device.h

efa14ae

Note CONFIG_PM_* affects device.h too. Even if not compiled, the more code the pre-processor sees the better. Signed-off-by: Marc Herbert <[email protected]>

marc-hb dismissed stale reviews from cfriedt, stephanosio, andyross, and fabiobaltieri via efa14ae July 2, 2024 23:42

marc-hb force-pushed the c++device branch from 57e2676 to efa14ae Compare July 2, 2024 23:42

marc-hb mentioned this pull request Jul 3, 2024

Allow specifying devices by DT node label in the shell #71912

Merged

marc-hb commented Jul 3, 2024

View reviewed changes

gmarull approved these changes Jul 3, 2024

View reviewed changes

nashif approved these changes Jul 8, 2024

View reviewed changes

nashif merged commit 65f6362 into zephyrproject-rtos:main Jul 8, 2024
23 checks passed

aescolar mentioned this pull request Jul 15, 2024

Any app setting CONFIG_PM or CONFIG_POWER_OFF will fail to build during cmake step on esp32c6_devkitc #75878

Closed

marc-hb mentioned this pull request Jul 19, 2024

Compiling usbd.h with C++ compiler fails #76072

Closed

marc-hb deleted the c++device branch October 9, 2024 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

device.h: C++20 compatibility #70403

device.h: C++20 compatibility #70403

marc-hb commented Mar 19, 2024 •

edited

Loading

gmarull Mar 19, 2024

marc-hb Mar 19, 2024 •

edited

Loading

gmarull Mar 20, 2024

fabiobaltieri Mar 20, 2024

marc-hb Mar 20, 2024 •

edited

Loading

fabiobaltieri Mar 20, 2024

gmarull Mar 19, 2024

marc-hb Mar 19, 2024

gmarull Mar 20, 2024

marc-hb Mar 20, 2024

marc-hb Jun 14, 2024 •

edited

Loading

marc-hb left a comment

marc-hb Mar 19, 2024

marc-hb Mar 19, 2024 •

edited

Loading

marc-hb commented Jun 17, 2024 •

edited

Loading

nashif commented Jun 27, 2024

keith-packard Jun 27, 2024

marc-hb Jul 3, 2024 •

edited

Loading

gmarull Jun 27, 2024

gmarull Jul 1, 2024

marc-hb Jul 3, 2024

marc-hb left a comment •

edited

Loading

marc-hb Jul 3, 2024 •

edited

Loading

marc-hb Jul 3, 2024

marc-hb commented Jul 8, 2024

device.h: C++20 compatibility #70403

device.h: C++20 compatibility #70403

Conversation

marc-hb commented Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

marc-hb Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

marc-hb commented Jun 17, 2024 • edited Loading

nashif commented Jun 27, 2024

Choose a reason for hiding this comment

marc-hb Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

marc-hb Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Jul 8, 2024

marc-hb commented Mar 19, 2024 •

edited

Loading

marc-hb Mar 19, 2024 •

edited

Loading

marc-hb Mar 20, 2024 •

edited

Loading

marc-hb Jun 14, 2024 •

edited

Loading

marc-hb Mar 19, 2024 •

edited

Loading

marc-hb commented Jun 17, 2024 •

edited

Loading

marc-hb Jul 3, 2024 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb Jul 3, 2024 •

edited

Loading