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.

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

treewide: compile fixes for native #14663

Merged
merged 6 commits into from
Jul 31, 2020
Merged

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jul 30, 2020

Contribution description

char is unsigned on ARM but signed on x86.
This lead to a few compile errors when adding the periph_gpio feature to native, because now code that was previously never build for x86 got hit by this difference.
The host compiler is also usually newer than the embedded one, which usually means it is stricter.

This makes offending chars explicitly unsigned and works around some pedantry.

Testing procedure

No logic changes. In fact, the binaries should stay the same.

Issues/PRs references

split off & needed for #12451
fixes #14667

@benpicco benpicco added 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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Jul 30, 2020
@benpicco benpicco changed the title Native fixes treewide: compile fixes for native Jul 30, 2020
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

See inline comments

drivers/cc110x/include/cc110x_settings.h Outdated Show resolved Hide resolved
drivers/cc110x/include/cc110x_settings.h Outdated Show resolved Hide resolved
drivers/kw2xrf/include/overwrites.h Outdated Show resolved Hide resolved
drivers/atwinc15x0/atwinc15x0_bsp.c Outdated Show resolved Hide resolved
drivers/atwinc15x0/atwinc15x0_netdev.c Outdated Show resolved Hide resolved
drivers/atwinc15x0/atwinc15x0_netdev.c Outdated Show resolved Hide resolved
drivers/atwinc15x0/atwinc15x0_netdev.c Outdated Show resolved Hide resolved
drivers/atwinc15x0/atwinc15x0_netdev.c Outdated Show resolved Hide resolved
pkg/semtech-loramac/contrib/semtech_loramac.c Outdated Show resolved Hide resolved
pkg/semtech-loramac/contrib/semtech_loramac_timer.c Outdated Show resolved Hide resolved
@maribu
Copy link
Member

maribu commented Jul 31, 2020

Feel free to squash

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

See inline. You can directly squash in the style nitpick. Regarding the -Wno-pedantic: Does this only affect code out of our control?

@maribu
Copy link
Member

maribu commented Jul 31, 2020

Is it only me, or are the inline comments gone?

@benpicco
Copy link
Contributor Author

I don't see it

@maribu
Copy link
Member

maribu commented Jul 31, 2020

OK, the comments was a another (void*) to (void *) that I initially overlooked, but you already fixed it. And the -Wno-pedantic indeed does only apply to the package and we cannot fix code outside of RIOTs tree.

So, both comments are addressed already.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. This PR does three things:

  • Use a different type name (uint8_t instead of char), which refers to the same type for all previously supported targets
  • Use hex instead of binary representation (0x01 instead of 0xb00000001) of the same numbers
  • Store a function pointer into an uint32_t instead of an void *

The first two changes do obviously not change anything in the generated binaries. The third change resulted in identical binaries being build of tests/pkg_semtech-loramac for the nucleo-f767zi according to elf_diff. Thus, no regressions are expected.

@maribu maribu merged commit f48147b into RIOT-OS:master Jul 31, 2020
@benpicco benpicco deleted the native-fixes branch July 31, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undefined behavior in pkg/semtech-loramac/contrib/
2 participants