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

picolibc: model as a feature #15011

Merged
merged 4 commits into from
Oct 14, 2020
Merged

picolibc: model as a feature #15011

merged 4 commits into from
Oct 14, 2020

Conversation

benpicco
Copy link
Contributor

Contribution description

Previously picolibc was selected via the PICOLIBC=1 command line parameter.
This changes that to modelling picolibc as a platform feature (as picolibc is not available yet for all architectures supported by RIOT).

Testing procedure

Add FEATURES_OPTIONAL += picolibc to your Makefile to opportunistically use picolibc.

Issues/PRs references

suggested in #14830 (comment)

@benpicco benpicco changed the title Picolobc feature picolibc: model as a feature Sep 11, 2020
@benpicco benpicco added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Sep 11, 2020
@benpicco benpicco mentioned this pull request Sep 11, 2020
Merged
@fjmolinas fjmolinas mentioned this pull request Sep 11, 2020
Merged
@benpicco benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 11, 2020
@benpicco
picolibc: model as a feature
cff8e86
@benpicco
examples/gnrc_minimal: use picolibc
69dd791 
On `samr21-xpro` this saves 5624 bytes of ROM and 108 bytes of RAM.
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 15, 2020
fjmolinas
fjmolinas approved these changes Sep 15, 2020
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK, dependencies against FEATURES_USED and USEMODULE is the way to go.

@benpicco
Copy link
Contributor Author

Hm something is missing in Riotdocker 🤔

@benpicco
Copy link
Contributor Author

clang: error: argument unused during compilation: '-specs=picolibc.specs'

Looks like we have to blacklist clang.

@fjmolinas
Copy link
Contributor

Looks like we have to blacklist clang.

But why did this not fail before?

@benpicco
Copy link
Contributor Author

We never compiled picolibc on CI before.

@fjmolinas
Copy link
Contributor

We never compiled picolibc on CI before.

Aha!

@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 16, 2020
@benpicco benpicco mentioned this pull request Sep 30, 2020
Merged
@benpicco benpicco added the State: waiting for CI update State: The PR requires an Update to CI to be performed first label Oct 1, 2020
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for CI update State: The PR requires an Update to CI to be performed first labels Oct 14, 2020
@aabadie
Copy link
Contributor

aabadie commented Oct 14, 2020

syscalls.c:236:5: error: implicit declaration of function 'FDEV_SETUP_STREAM' is invalid in C99 [-Werror,-Wimplicit-function-declaration

This is fixed now: the workers have been updated on riotbuild. There are still a couple of issues on the Breeze workers, but I can't reproduce locally and it doesn't seem to be related to this PR. Maybe it also needs an update ?

@aabadie aabadie self-assigned this Oct 14, 2020
@benpicco benpicco force-pushed the picolobc_feature branch 2 times, most recently from a4f4139 to 529e21e Compare October 14, 2020 10:25
aabadie
aabadie reviewed Oct 14, 2020
View reviewed changes
cpu/fe310/Kconfig Outdated
@@ -8,6 +8,7 @@
config CPU_ARCH_RISCV
bool
select HAS_ARCH_RISCV
select HAS_PICOLIBC if !'$(RIOT_CI_BUILD)'
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work. @leandrolanzieri do you know how to check if RIOT_CI_BUILD is set from Kconfig ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think comparing strings would be the way. Like so:

RIOT/Kconfig

Line 44 in ac097bb

default y if '$(TEST_KCONFIG)' = '1'

Maybe select HAS_PICOLIBC if '$(RIOT_CI_BUILD)' != '1'?

@benpicco
cpu/fe310: blacklist picolibc on CI
45270da 
The RISC-V toolchain in riotdocker has issues with picolibc and
will still include newlib headers.

This leads to conflicts like

```
In file included from [01m[Knanostubs.c:22[m[K:
[01m[K/usr/local/picolibc/riscv-none-embed/include/stdio.h:270:23:[m[K [01;31m[Kerror: [m[Kconflicting types for '[01m[K__FILE[m[K'
 typedef struct __file [01;31m[K__FILE[m[K;
                       [01;31m[K^~~~~~[m[K
In file included from [01m[K/opt/gnu-mcu-eclipse/riscv-none-gcc/8.2.0-2.2-20190521-0004/riscv-none-embed/include/reent.h:93[m[K,
                 from [01m[Knanostubs.c:20[m[K:
[01m[K/opt/gnu-mcu-eclipse/riscv-none-gcc/8.2.0-2.2-20190521-0004/riscv-none-embed/include/sys/reent.h:287:26:[m[K [01;36m[Knote: [m[Kprevious declaration of '[01m[K__FILE[m[K' was here
 typedef struct __sFILE   [01;36m[K__FILE[m[K;
                          [01;36m[K^~~~~~[m[K
```

The problem does not occur when installing both the toolchain and picolibc
directly from the Debian / Ubuntu repositories, but CI uses an older Ubuntu
version that does not have those packages yet, so it builds them manually.

Blacklist RISC-V until CI has been updated.
@benpicco benpicco added Area: arduino API Area: Arduino wrapper API and removed Area: arduino API Area: Arduino wrapper API labels Oct 14, 2020
@aabadie aabadie merged commit 8df645c into RIOT-OS:master Oct 14, 2020
@aabadie aabadie mentioned this pull request Oct 14, 2020
Merged
@benpicco benpicco deleted the picolobc_feature branch October 14, 2020 15:29
@nmeum nmeum mentioned this pull request Feb 11, 2021
Merged
@benpicco benpicco mentioned this pull request Nov 3, 2021
Closed
@gschorcht
Copy link
Contributor

@benpicco While developing a board definition for the LilyGO-T-Display-GD32 board, I was wondering why I got a Kconfig feature mismatch for picolib locally but not in CI. I guess the change in commit 45270da is now obsolete.

@gschorcht gschorcht mentioned this pull request Aug 7, 2023
Merged
bors bot added a commit that referenced this pull request Aug 7, 2023
@bors @gschorcht @benpicco
Merge #19824 #19855 #19862
d512ff6 
19824: boards/sipeed_longan_nano: separate board definition for Sipeed Longan Nano TFT r=benpicco a=gschorcht

### Contribution description

This PR provides a minimal separate board definition for the Sipeed Longan Nano version with TFT display which is just an extension of `boards/sipeed-longan-nano` with enabled TFT display module.

From the lessons we had to learn with the Kconfig modelling of optional hardware, the TFT version of the Sipeed Longan Nano board has been split off into its own board definition based on the existing Siepeed Longan Nano board.

Commits ba29a5e, 237819e, 6d8b56d and c5faf34 are small cleanups of peripheral configurations and could be split from this PR as follow-up PR (changes +70 -36).

### Testing procedure

Green CI

```
BOARD=sipeed-longan-nano-tft make -j8 -C tests/drivers/st77xx flash
```
should work

### Issues/PRs references

Follow up to PR #19813 and PR #19814
Prerequisite for PR #19825 and PR #19827 

19855: gnrc_static: fix static PID assignment r=benpicco a=benpicco



19862: cpu/riscv_common: remove picolibc from blacklisting in CI r=benpicco a=gschorcht

### Contribution description

`picolibc` is now supported by the RISC-V toolchain in `riotdocker`. It is not necessary to blacklist it in CI any longer.

Reference: `picolib` was blacklisted by commit 45270da in PR #15011.

### Testing procedure

1. Green CI
2. Check feature list for CI compilation:
   ```
   BUILD_IN_DOCKER=1 RIOT_CI_BUILD=1 FEATURES_OPTIONAL=picolibc \
   BOARD=sipeed-longan-nano make -j8 -C tests/sys/shell info-modules | grep picolib
   picolibc
   picolibc_stdout_buffered
   picolibc_syscalls_default
   ```

### Issues/PRs references



Co-authored-by: Gunar Schorcht <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie left review comments

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@fjmolinas fjmolinas fjmolinas approved these changes

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

@cgundogan cgundogan Awaiting requested review from cgundogan

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

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

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

@thomaseichinger thomaseichinger Awaiting requested review from thomaseichinger

Assignees

@aabadie aabadie

Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@benpicco @fjmolinas @aabadie @keith-packard @gschorcht @leandrolanzieri @cgundogan