Please wait with this PR until #7882 is merged.

@gebart makes sense, so squash your #7882, and I'm happy to merge it right away (if Murdock is happy, though). Afterwards I can adapt mine here.

rebased after #7882 merged, ready to go from my side.

@cgundogan as @gebart pointed out, won't work need pragma here - besides we do use pragma in RIOT where ever necessary.

@cgundogan nice idea, wasn't aware of that plus seems to work!

@cgundogan -isystem is for system headers such as libc and compiler provided headers. Besides, if you wanted to use that you would make the whole CPU include dir a system include, because there are no -I for the vendor directories themselves.

@gebart, I replaced the #include "vendor/*.h with #include "*.h" and added an extra include directive for subdirectory include/vendor.

@cgundogan -isystem is for system headers such as libc and compiler provided headers.

Hm from the gcc docs:

You can use -I to override a system header file, substituting your own version, since these directories are searched before the standard system header file directories. However, you should not use this option to add directories that contain vendor-supplied system header files; use -isystem for that.

In interpret this as a good fit of -isystem for our vendor files. To put it this way: we could've also provided a RIOT installer that puts those headers into an appropriate system location. They are third-party and we usually do not modify them.

Okay, I see your point with the isystem, but still we are using the path prefix vendor/x.h to see clearly show in the source where we are using vendor supplied headers, I don't think we should change that.

Ideally there shouldn't be any comment here. But just in case: this is a warning for that ;-).

Worked :-)

@gebart by using -isystem we can use <vendor.h> instead of "vendor.h" to show the difference to RIOT internal headers. Prefixing it with directory vendor/header.h would be possible, too, by using -isystem for $(CPU)/include but this would also prevent e.g. cpu_conf.h from being checked, which isn't desirable, right?

If we agree to use -isystem for vendor headers I would adapt this PR for all vendor headers and put the include into a upper level Makefile, would do you think?

I like the idea of the -isystem and the "#include <vendor_header.h>".

If it is made global to RIOT, I think it would be good to add a new variable for system include path, and populate CFLAGS from the main Makefile.include.
This way it would document at least in one place why its needed instead of thinking everyone knows about '-isystem'.

@cladmi I take this as a +1 for the proposed idea above.

Will adapt this PR in a fixup commit here for review.

@kaspar030 and @cgundogan adapted to proposed solution, thanks for the hint!

@cgundogan happy, can I squash?

rebased and squashed

@haukepetersen you are the one who introduced the vendor include prefix, I think it makes sense if you add your comments to this PR

@gebart and/or @haukepetersen can you elaborate, I'm out of context right now?

Sure: (big) +1 for adding the vendor headers to the system includes. But I think this PR is immature and does not go all the way. I have mainly two concerns:

• Adding the vendor headers to the system include path in the main Makefile.include using $(RIOTCPU)/$(CPU)/include/vendor is a bad idea: this forces a certain structure on the internals of a CPU implementation. E.g. the headers for the sam0 family are all stored in the common CPU folder. I strictly think that the definition of the location of the vendor headers should be something a CPU implementation has to do explicitly!

• When adding (and using) the vendor header files from the system include path, they should be removed from the application include path (or however the 'normal' include path -Ixx.h is called). This only fills up the include path and still allows to use the #include "vendor/xx.h" include scheme, inviting developers for style errors...

So if we go with this change, let's do it right!

@haukepetersen:

for your first point: I had it that way first (i.e., in <cpu>/Makefile.include), but others suggested to put it in a more central Makefile.

For the second: I tend to agree as well, but was hesitant to introduce another subdirectory for vendor headers, which are still includes in a way. So you would suggest to have directory structure like:

cpu/
    <cpu_abc>/
           include/    <- RIOT headers, like cpu.h
           periph/     <- low level periph drivers
           vendor/     <- vendor headers
    <cpu_xyz>/
    ...

@kYc0o sorry for the short message. I was referring to an earlier PR (#6700) by @haukepetersen where we moved all vendor headers to the vendor dir to avoid name clashes between vendor files and our own.

@haukepetersen

• Adding the vendor headers to the system include path in the main Makefile.include using $(RIOTCPU)/$(CPU)/include/vendor is a bad idea: this forces a certain structure on the internals of a CPU implementation. E.g. the headers for the sam0 family are all stored in the common CPU folder. I strictly think that the definition of the location of the vendor headers should be something a CPU implementation has to do explicitly!

I don't agree, I think it makes it a lot easier on the port developers to have the central makefile assume at least one CPU specific include directory. Any port which needs more than the one default include path can set additional paths in its Makefile.include, but it makes future refactoring and maintenance easier to have the default set at only one location.

I propose that the includes still have the vendor/ prefix, even with -isystem, to make them stand out a little extra against the other system libc/compiler headers such as math.h, stdio.h, inttypes.h etc. (#include <vendor/mycpumodelregmap.h>)
On the other hand, doing it like this would make the file location a bit convoluted: cpu/$CPU/vendor/include/vendor/mycpumodelregmap.h

I still disagree on the default inclusion, but I won't block this. So if you guys agree that this is best, leave it...

Regarding the include path: I agree with @gebart, that keeping the vendor/ prefix is a good idea. Alternatively we could go with prefixing the vendor headers with the company name?! Just thinking out loud now: what do you think about putting vendor headers in a central location, something like:

/cpu/system_include/stm/...
/cpu/system_include/atmel/...
// or maybe
/cpu/vendor_include/stm/...
...
e.g. #include <stm/stm32f4xx.h> or #include <atmel/samd21/include/samd21j18a.h>

This would lead to a clear separation of vendor and RIOT specific headers, would simplify the exclusion of these headers (e.g. in static tests etc), and it would also simplify/solve the issue of the default include path as discussed above :-)

One more general question that just came to my mind: does 'moving' the vendor headers to the system includes have any implications at the #includes called from within the vendor headers?

keeping the vendor/ prefix is a good idea.

yes, but it also likely means we need to have an unnecessarily complicated nested directory structure just to achieve that. IMHO if we stick with a certain order for #includes like

// system include
#include <stdio.h>
...
// vendor includes
#include <cc2538.h>
...
// RIOT includes
#include "board.h"
...
// app includes
#include "mystuff.h"

document and impose that rule/order when reviewing, would be fine too. I know there might be side-cases where headers must be included in a certain order - but no rule without exception, I guess.

what do you think about putting vendor headers in a central location,

big NO, please don't - this basically means to duplicate (or even extend) the current directory structure for nearly all CPUs (or vendors) we have in another directory. I really favour the current approach of heaving vendor headers with their respective CPU, that is also as close as possible to it directory-wise, like for stm32f1 directly within that directory, or for samd21 in sam0_common and all ARM based also share cortexm_common.

would simplify the exclusion of these headers (e.g. in static tests etc)

but at the costs describe above, btw. its already dead simple: just exclude any file within some (sub) directory of */vendor/*, which is also in place for all (most?) static tests.

also note: typically vendor headers are included by cpu.h or something like it very close to the actual cpu stuff in RIOT. Contrary to my example for order of includes, most developers will never use vendor includes but use higher layer RIOT headers, which wrap that away. And I would also say that someone who is porting a CPU to RIOT knows to distinguish vendor from system headers and hence doesn't need the directory suffix vendor/ that badly.

I agree with @smlng, don't move the headers to a single dir, that would make it harder to find the files and breaks the containment for CPU specific files. However, I do like the idea of changing the vendor prefix to the actual vendor name, it makes it even more obvious what each header inclusion is for.

big NO, please don't - this basically means to duplicate (or even extend) the current directory structure for nearly all CPUs (or vendors) we have in another directory. I really favour the current approach of heaving vendor headers with their respective CPU, that is also as close as possible to it directory-wise, like for stm32f1 directly within that directory, or for samd21 in sam0_common and all ARM based also share cortexm_common.

I completely disagree. The more I think about it, the more I even start to like this idea.

• I don't see us duplicating the current directory structure at all, handling this on a (IMHO very logically) vendor level suffices, so not really a duplication of the CPU directory structure
• it is not like we currently have the 'one fits all' solution anyway. We are being inconsistent (e.g. stms vs. sam0's vs. kinetis).
• what is easier to read for beginners: exclude */vendors/* or exclude cpu/vendor_include/*? (Though this is minor and indeed not relevant for this discussion :-) ).

but at the costs describe above

I don't see the costs. If it helps the discussion, I could open a showcase branch so we get a practical comparison going.

Contrary to my example for order of includes, most developers will never use vendor includes but use higher layer RIOT headers

Completely agreed. But all directions in the current discussions are equal in this regard...

document and impose that rule/order when reviewing, would be fine too.

This I have my trouble with: if we can have ways to enforce something that are equally simple to implement than the rule based variant, we should always choose enforcing variant... We all know how bad reviews can be on things that are not enforced...

I do like the idea of changing the vendor prefix to the actual vendor name,

but then we could go for my suggested directory structure above just adding:

cpu/
    <cpu_abc>/
           include/    <- RIOT headers, like cpu.h
           periph/     <- low level periph drivers
           vendor/<vendor-name>/     <- vendor headers
    <cpu_xyz>/
    ...

and then use #include <vendor-name/header.h>.

A good compromise, at least to me.

Which vendor headers actually cause problems? To me it seems they're not that many (I only found the mips headers). We could just pragma their include sites.

Please don't rearrange the directory structure just for "-pedantic". And please don't put "vendor/stmd32foobar.h" on the same level as <stdio.h> by including it as <stmd32foobar.h>, just for "-pedantic" sake. The gain (compared to -Wextra) is almost completely academic.

Forget everything I stated above. After talking about the more general motivation of this PR offline with people, I come to the same conclusion as @kaspar030 above and I say that we don't add the vendor headers to the system headers in the first place. Excluding the (as it seems few) vendor headers that are actually causing problems with pedantic using pragmas seems to be the better move to me, too.

Looking at the current code base on master, we already use a lot of pragmas for similar purposes. I wasn't aware of that and AFAIR, our intentions were always to reduce the use of pragmas whereever possible. If that's not our philosophy, then I do not understand why we're not using pragma once, but do header guards instead. Nevertheless, @smlng your initial proposal is the least intrusive solution and is in line with @kaspar030's and @haukepetersen's opinion. Please revert to that (:

If that's not our philosophy, then I do not understand why we're not using pragma once, but do header guards instead.

AFAIK (and I'm well aware that I could be totally wrong there) pragma once is not portable. As far as I can see pragmas in our code-base are only used either in places where portability isn't required to be assured (CPU-code) or are #ifdef'd properly

AFAIK (and I'm well aware that I could be totally wrong there) pragma once is not portable. As far as I can see pragmas in our code-base are only used either in places where portability isn't required to be assured (CPU-code) or are #ifdef'd properly

Why is CPU code not required to be portable? Still multiple compilers can be used to compile for a single cpu.

I do not understand why we're not using pragma once, but do header guards instead.

Simple: pragma once uses a file's full path to identify if it is already included, the #ifdef-guards use a custom string. This means that pragme once can not be used to have header files that are 'overridden' by others depending on the order of the include paths (like we do e.g. for the xx_params.h files).

AFAIK (and I'm well aware that I could be totally wrong there) pragma once is not portable

This is not (always) true: The ‘#pragma’ directive is the method specified by the C standard [quote gcc documentation]. If I understand the doc correctly, a subset of pragma directives is defined by the C Standard, and should well be portable. But compilers can also add their own pragmas, which are not portable I guess...

how do we want to proceed here, I still like the idea of including the vendor headers with -isystem in general. Regardless of -Wpedantic errors, which might also be solved by using #pragmas.

I still like the idea of including the vendor headers with -isystem in general.
You'd find as many subjective reasons to not like this as you'd find to like it.

I see a style change with no objective benefits and at least potential for confusion / trouble.
Sometimes it is reasonable to be conservative, and IMO, here it is.

No consensus here, thus I'll close. Feel free to re-open.