Skip to content

Conversation

@gmarull
Copy link
Member

@gmarull gmarull commented Mar 9, 2022

<zephyr.h> ends up pulling <sys/printk.h> via <kernel_includes.h>, so
simplify the sample.

Note: this change assumes <zephyr.h> is a "catch-all" header for applications.

@gmarull gmarull added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Mar 9, 2022
@gmarull gmarull requested a review from nashif as a code owner March 9, 2022 20:08
@github-actions github-actions bot added the area: Samples Samples label Mar 9, 2022
Copy link
Member

Choose a reason for hiding this comment

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

What motivated this change?

Using zephyr.h as a sort of "go-to" header, rather than identifying and manually specifying the required individual headers, is much simpler and less confusing to the beginners; in fact, that is why this header exists in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ref.

#41543

<zephyr.h> in particular is confusing, as it's just <kernel.h> + __ZEPHYR__. It's like a <Windows.h>. Btw, I think it would be nice to have all public headers in zephyr/ so that we could have <zephyr/kernel.h>.

Copy link
Member Author

@gmarull gmarull Mar 9, 2022

Choose a reason for hiding this comment

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

Btw, look at what we pull with <kernel.h> that we don't need in main.c:

image

🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

Well... someone needs it. Zephyr leans heavily on header inlining (LTO is always just over the horizon, but right now inlined code is our tool), especially at the arch layer, and all those seemingly internal headers are needed to expose implementations to the APIs apps "need".

I think there's an excellent argument to be made that a(nother) cleanup pass on our header tangle is probably due. And there's at least a vaguely justifiable bikeshed discussion to be had[1] about whether a "catchall" header for "the whole Zephyr API" should exist, whether it should be named "zephyr.h" or "kernel.h" (and why they are separate files at all), and whether or not code in samples/ and tests/ should be using it.

But I mean, if we have such a header, clearly we'd want to be using it in hello_world, the basic archetype of "A Zephyr App".

[1] Not by me!

(FWIW: this bug was totally worth it just to see that graph)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well... someone needs it.

Maybe kernel.h itself, but not the main.c in hello_world sample.

I think there's an excellent argument to be made that a(nother) cleanup pass on our header tangle is probably due. And there's at least a vaguely justifiable bikeshed discussion to be had[1] about whether a "catchall" header for "the whole Zephyr API" should exist, whether it should be named "zephyr.h" or "kernel.h" (and why they are separate files at all), and whether or not code in samples/ and tests/ should be using it.

Any inputs on #41543 are welcome :-)

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

Where in our docs we talk about autoconf.h and ask for it to be included?
This is a SAMPLE, this is not intended for anything else but show in the most direct way possible and without going into details how to say hello world.
C'MON

@gmarull
Copy link
Member Author

gmarull commented Mar 10, 2022

Where in our docs we talk about autoconf.h and ask for it to be included?

So, then I assume <zephyr.h> is the way to access all CONFIG_ definitions? Where is this documented? autoconf.h is documented here: https://docs.zephyrproject.org/latest/guides/build/index.html#configuration-system-kconfig

This is a SAMPLE, this is not intended for anything else but show in the most direct way possible and without going into details how to say hello world. C'MON

So, I assume we recommend using <zephyr.h> because "it's Zephyr", not for any particular reason.

Btw, with this change I'm not suggesting that <autoconf.h> is the best way, but <zephyr.h> is probably not the solution as it is either. We definitely need to improve quality in include/, I think this PR illustrates the problem.

@gmarull gmarull requested a review from nashif March 10, 2022 08:57
@stephanosio
Copy link
Member

Windows has windows.h, GLIb has glib.h, Python has Python.h, Zephyr has zephyr.h.

I don't see anything out of ordinary and/or particularly problematic here.

@gmarull
Copy link
Member Author

gmarull commented Mar 10, 2022

Windows has windows.h, GLIb has glib.h, Python has Python.h, Zephyr has zephyr.h.

I don't see anything out of ordinary and/or particularly problematic here.

IMO the scope of <zephyr.h> is not clear. Is it for applications? What does it cover? For example, both programs compile:

diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index 6c5c8a27dc..987728bbf2 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -5,7 +5,6 @@
  */
 
 #include <zephyr.h>
-#include <sys/printk.h>
 
 void main(void)
 {
diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index 6c5c8a27dc..b8cedd3678 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -4,7 +4,6 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-#include <zephyr.h>
 #include <sys/printk.h>
 
 void main(void)

why? should I never use sys/printk.h? or should I just include it? Or both "just in case"?

@carlescufi
Copy link
Member

Windows has windows.h, GLIb has glib.h, Python has Python.h, Zephyr has zephyr.h.
I don't see anything out of ordinary and/or particularly problematic here.

IMO the scope of <zephyr.h> is not clear. Is it for applications? What does it cover? For example, both programs compile:

diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index 6c5c8a27dc..987728bbf2 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -5,7 +5,6 @@
  */
 
 #include <zephyr.h>
-#include <sys/printk.h>
 
 void main(void)
 {
diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index 6c5c8a27dc..b8cedd3678 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -4,7 +4,6 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-#include <zephyr.h>
 #include <sys/printk.h>
 
 void main(void)

why? should I never use sys/printk.h? or should I just include it? Or both "just in case"?

How about we analyze how those platforms mentioned above deal with individual components like printk()? If we are justifying the presence of zephyr.h based on other projects (a good practice in my opinion) then we might as well take it further and observe how those other projects deal with the rest, including Linux as well.

<zephyr.h> ends up pulling <sys/printk.h> via <kernel_includes.h>, so
simplify the sample.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
@gmarull gmarull force-pushed the hello-world-headers branch from 2fe39f6 to f23d77e Compare March 10, 2022 09:33
@gmarull
Copy link
Member Author

gmarull commented Mar 10, 2022

In general, I like the idea of <zephyr.h> for applications, but its scope/usage should be clear and well documented.

@nashif nashif merged commit 6c261f2 into zephyrproject-rtos:main Mar 17, 2022
@gmarull gmarull deleted the hello-world-headers branch April 24, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Samples Samples Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants