-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Makefile.include: fix override headers from application #20744
base: master
Are you sure you want to change the base?
Makefile.include: fix override headers from application #20744
Conversation
Hmm it's failing on a test that tries to override the |
I included a commit that seems to fix this (at least this failing case) locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Thanks for fixing this! ❤️
@@ -399,6 +399,7 @@ ifneq (,$(IOTLAB_NODE)) | |||
endif | |||
|
|||
# Add standard include directories | |||
INCLUDES += -I$(APPDIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! I have falling into this pitfall numerous times when I needed to quickly test something, but never really traced it down but instead did a quick-and-dirty hack of the system header instead.
I'm glad that next time I need to quickly test something, I won't fall into that pitfall again ❤️
I'm almost certain that header was never included before, so it can be safely removed. I think the idea was to have the signatures of the IRQ functions available, so that setting up the mockup wrapper functions is possible. Since the mock wrapper functions replace the actual calls, having a working IRQ header included was not needed. They just would need to match the API. (For the mock ups it would not matter if they are But as the test worked just fine with wrapping the IRQ functions using the real signatures (including |
After some fiddling around, it turns out it's not so straight forward. Removing the header seems to uncover other issues:
IIUC the "official" interface is to always go through The test would then define |
Ah, OK. So Now, the header is overriding the systems Maybe just copy-paste the contents of irq.h in the test source file and drop the header then? |
42212cd
to
78413fe
Compare
Now this is implemented on 78413fe. It feels a bit off, but the alternative (I believe) would be a whole restructuring of how |
To avoid name clash, rename this application header file to test_common.h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks! Do you know what was failing in the last Murdock build? Any chance we can get this in before hard-feature freeze next week?
@leandrolanzieri what do you need to move this forward? |
Contribution description
As our drivers guide states:
For some reason this doesn't seem to be working. I'm not sure whether there was some uncaught regression at some point. Currently, when one places a
<driver>_params.h
this doesn't override the one define by RIOT, and is simply ignored.This PR fixes the issue by including the
APPDIR
in the considered include paths, so that it has priority.Testing procedure
<driver>_params.h
file in your app directory.Issues/PRs references
None