Skip to content
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

Jump to bottom

sys/net/gnrc/netif: Fix compilation on waspmote-pro #15042

Merged
merged 2 commits into from
Sep 23, 2020
Merged

sys/net/gnrc/netif: Fix compilation on waspmote-pro #15042

merged 2 commits into from
Sep 23, 2020

Conversation

maribu
Copy link
Member

@maribu maribu commented Sep 18, 2020

Contribution description

xtimer.h must not be included, when the xtimer module is not use. Otherwise compilation on the waspmote-pro with #14799 will not longer work. gnrc_netif_pktq includes xtimer.h and uses xtimer, but gnrc_netif includes gnrc_netif_pktq.h regardless of whether gnrc_netif_pktq is used. This makes sure that gnrc_netif_pktq.h is only included when actually used.

Testing procedure

No changes in generated binaries

Issues/PRs references

Needed for #14799

@maribu maribu added Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 18, 2020
miri64
miri64 previously requested changes Sep 18, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Sad, that this doesn't work generally :(

sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved
@maribu maribu force-pushed the gnrc_dont_include_xtimer_when_not_used branch from 10ba1fe to afbb619 Compare September 19, 2020 06:16
@fjmolinas
Copy link
Contributor

@miri64 is this one OK now?

miri64
miri64 reviewed Sep 22, 2020
View reviewed changes
sys/net/gnrc/netif/gnrc_netif.c Outdated
@@ -27,7 +27,9 @@
#include "net/gnrc/ipv6/nib.h"
#include "net/gnrc/ipv6.h"
#endif /* MODULE_GNRC_IPV6_NIB */
#ifdef MODULE_GNRC_NETIF_PKTQ
Copy link
Member

Choose a reason for hiding this comment

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

Here too

Suggested change
#ifdef MODULE_GNRC_NETIF_PKTQ
#if IS_USED(MODULE_GNRC_NETIF_PKTQ)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is consistent with the other includes. Should I update the other as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe as own commit upfront, and the current commit is added afterwards? That way it would always be consistent and one commit per logical change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@miri64: Did as described above.

@miri64
Copy link
Member

miri64 commented Sep 22, 2020

One minor nitpick remaining

@maribu maribu force-pushed the gnrc_dont_include_xtimer_when_not_used branch from afbb619 to 2a4eeef Compare September 22, 2020 08:27
@fjmolinas
Copy link
Contributor

@maribu murdock is not happy :(

@miri64 miri64 dismissed their stale review September 22, 2020 08:41

Looks good now, don't have the capability to test though. @jia200x can you have look?

@maribu
Copy link
Member Author

maribu commented Sep 22, 2020

@maribu murdock is not happy :(

I ran in those issues a few times when using

#if IS_USED(MODULE_FOO)
#include "foo.h"
#endif

Let me revert the changes to the previous state and use #ifdef MODULE_FOO for the #includes for now. I would like to get the bugfix for the waspmote in swiftly and not delay this until a solution for using IS_USE() in that instances is found.

@miri64
Copy link
Member

miri64 commented Sep 22, 2020

Most likely the problem comes because kernel_defines.h was included after the #if

@maribu
Copy link
Member Author

maribu commented Sep 22, 2020

Most likely the problem comes because kernel_defines.h was included after the #if

No, kernel_defines.h is included first. But good idea.

@maribu
sys/net/gnrc/netif: use IS_USED macro
1d1b446
@maribu
sys/net/gnrc/netif: Fix compilation on waspmote-pro
8a5d301 
xtimer.h must not be included, when the xtimer module is not use. Otherwise
compilation on the waspmote-pro with RIOT-OS#14799
will not longer work. gnrc_netif_pktq includes xtimer.h and uses xtimer, but
gnrc_netif includes gnrc_netif_pktq.h regardless of whether gnrc_netif_pktq
is used. This makes sure that gnrc_netif_pktq.h is only included when actually
used.
@maribu maribu force-pushed the gnrc_dont_include_xtimer_when_not_used branch from 2a4eeef to 8a5d301 Compare September 22, 2020 09:51
@maribu
Copy link
Member Author

maribu commented Sep 22, 2020

Most likely the problem comes because kernel_defines.h was included after the #if

No, kernel_defines.h is included first. But good idea.

OK, the issue was just me being stupid :-)

(#ifdef IS_USED(...) instead of #if IS_USED(...))

@maribu
Copy link
Member Author

maribu commented Sep 22, 2020

Generated binaries of tests/unittests for the Nucleo-F767ZI without and with this PR don't differ, according to elf_diff: https://www.mari-bu.de/pr15042-nucleo-f767zi-tests-unittests.html

(Compiled with make RIOT_CI_BUILD=1 BOARD=nucleo-f767zi -C tests/unittests/ for this PR and acf14f8)

@miri64
Copy link
Member

miri64 commented Sep 22, 2020

Generated binaries of tests/unittests for the Nucleo-F767ZI without and with this PR don't differ, according to elf_diff: https://www.mari-bu.de/pr15042-nucleo-f767zi-tests-unittests.html

(Compiled with make RIOT_CI_BUILD=1 BOARD=nucleo-f767zi -C tests/unittests/ for this PR and acf14f8)

No surprise, because AFAIK the code you touch is not covered by those tests 😅. Can you check with USEMODULE=gnrc_netif_pktq added to examples/gnrc_networking, please?

@maribu
Copy link
Member Author

maribu commented Sep 22, 2020

Compiled with USEMODULE=gnrc_netif_pktq make RIOT_CI_BUILD=1 BOARD=nucleo-f767zi -C examples/gnrc_networking: https://www.mari-bu.de/pr15042-nucleo-f767zi-examples-gnrc-networking-with-gnrc_netif_pktq.html

In the HTML it seems that _send_queued_pkt has changed. But the size is identical and the disassembled instruction also look identical to me. Verifying this by

$ cd path/to/pr
$ USEMODULE=gnrc_netif_pktq make RIOT_CI_BUILD=1 BOARD=nucleo-f767zi -C examples/gnrc_networking
$ arm-none-eabi-objdump -d examples/gnrc_networking/bin/nucleo-f767zi/gnrc_netif/gnrc_netif.o > ~/pr
$ cd path/to/master
$ USEMODULE=gnrc_netif_pktq make RIOT_CI_BUILD=1 BOARD=nucleo-f767zi -C examples/gnrc_networking
$ arm-none-eabi-objdump -d examples/gnrc_networking/bin/nucleo-f767zi/gnrc_netif/gnrc_netif.o > ~/master
$ diff ~/master ~/pr
$ #<no output from diff>

So no machine code changed. I wonder why elf_diff still displays the symbols as different.

@benpicco benpicco added State: waiting for CI update State: The PR requires an Update to CI to be performed first and removed State: waiting for CI update State: The PR requires an Update to CI to be performed first labels Sep 22, 2020
miri64
miri64 approved these changes Sep 23, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

OK, if there is literally no change, I think this can be merged. example/gnrc_networking also still works withoout gnrc_netif_pktq.

@miri64 miri64 merged commit 711ea27 into RIOT-OS:master Sep 23, 2020
@maribu maribu deleted the gnrc_dont_include_xtimer_when_not_used branch November 3, 2020 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @fjmolinas @miri64 @benpicco