Skip to content

lib: dk_buttons_and_leds: Include nrfx.h to make GPIO defines available #180

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 30, 2018
Merged

lib: dk_buttons_and_leds: Include nrfx.h to make GPIO defines available #180

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/dk_buttons_and_leds/dk_buttons_and_leds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <gpio.h>
#include <misc/util.h>
#include <logging/sys_log.h>
#include <nrfx.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

CONFIG_GPIO_P0_DEV_NAME is really unavailable? It should be included from fixups brought by soc.h. I think all boards include this file in their board.h file. Maybe this is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, when compiling for nRF91, CONFIG_GPIO_P0_DEV_NAME is currently not defined. I need to refer to @barsok for an explanation of why this is. I agree that using CONFIG_GPIO_P0_DEV_NAME without the need of any extra header file would be the preferred solution, but as of now I don't think it's possible. CONFIG_GPIO_0_LABEL is defined, but my understanding is that CONFIG_GPIO_P0_DEV_NAME is the preferred way to get the device name.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that somebody forgot to include a header but in such case it would be better to fix the actual problem. Otherwise people will have to make sure they add this additional include.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point was to reduce dts_fixup.h to a minimum. In each compilation autoconfig.h is available (injected in gcc command line). A lot of other includes also include generated_dts_board.h and there are all necessary labels etc. The legacy kconfig names have been moved to nrfx.h. Some discussion may be needed if there is a need to duplicate DTS definitions, ie CONFIG_GPIO_P0_DEV_NAME with CONFIG_GPIO_0_LABEL. If you think so we can discuss what is really needed short-term and long-term

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I have seen in zephyr code base then CONFIG_GPIO_P0_DEV_NAME is very often used.

Copy link
Contributor Author

@jtguggedal jtguggedal Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have strong opinions on this, other than that we need a fix for this as soon as possible so samples using this library can compile.

I'll leave it up to you as the author of the lib to decide which solution to use @jhn-nordic , I will update the PR accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jtguggedal I think you should include nrfx.h and then we can remove it when CONFIG_GPIO_P0_DEV_NAME gets available again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then this PR is merge-ready I think

Copy link
Contributor

Choose a reason for hiding this comment

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

@barsok I have created a ticket in Zephyr to improve the way labels are defined. Maybe there will be some further discussion regarding this.
zephyrproject-rtos/zephyr#10938

#include <dk_buttons_and_leds.h>

static struct k_delayed_work buttons_scan;
Expand Down