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

boards/sipeed-longan-nano: revert default to variant with TFT #19814

Merged

Conversation

gschorcht
Copy link
Contributor

Contribution description

This PR reverts commit 69fb00b to fix CI compilation.

Testing procedure

Green CI with full build.

Issues/PRs references

@github-actions github-actions bot added Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration labels Jul 10, 2023
@gschorcht gschorcht requested a review from maribu July 10, 2023 12:58
@gschorcht gschorcht added 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 CI: full build disable CI build filter labels Jul 10, 2023
@maribu
Copy link
Member

maribu commented Jul 10, 2023

This was intended to fix https://ci.riot-os.org/results/87ababc017c34548931760b535fd1635/output/builds/tests/drivers/disp_dev/sipeed-longan-nano:gnu.txt where the modules used and the firmware sizes differ.

And indeed, now the modules used and the firmware sizes for this app/board combi matches, only the hashes differ: https://ci.riot-os.org/results/c75caeb32cba4b0486da5a0617302de7/output/builds/tests/drivers/disp_dev/sipeed-longan-nano:gnu.txt

So this does improve things for this test. Looking at the Makefile handles things, I think

diff --git a/boards/sipeed-longan-nano/Kconfig b/boards/sipeed-longan-nano/Kconfig
index 0899330d76..7f65aa90a6 100644
--- a/boards/sipeed-longan-nano/Kconfig
+++ b/boards/sipeed-longan-nano/Kconfig
@@ -38,7 +38,7 @@ menu "Sipeed Longan Nano Board Configuration"
 
     config SIPEED_LONGAN_NANO_WITH_TFT
         bool "Board with TFT display"
-        default y
+        default y if MODULE_DISP_DEV
         select HAVE_ST7735
         help
             Indicates that a Sipeed Longan Nano board with TFT display is used.

might be the way to go if we want to stick with behavior of the non-KConfig resolution.

I can reproduce the mis-matching hashes, though.

@maribu
Copy link
Member

maribu commented Jul 10, 2023

diff --git a/boards/sipeed-longan-nano/Kconfig b/boards/sipeed-longan-nano/Kconfig
index 0899330d76..7f65aa90a6 100644
--- a/boards/sipeed-longan-nano/Kconfig
+++ b/boards/sipeed-longan-nano/Kconfig
@@ -38,7 +38,7 @@ menu "Sipeed Longan Nano Board Configuration"
 
     config SIPEED_LONGAN_NANO_WITH_TFT
         bool "Board with TFT display"
-        default y
+        default y if MODULE_DISP_DEV
         select HAVE_ST7735
         help
             Indicates that a Sipeed Longan Nano board with TFT display is used.
diff --git a/boards/sipeed-longan-nano/Makefile.include b/boards/sipeed-longan-nano/Makefile.include
index 1ac941091b..da31713759 100644
--- a/boards/sipeed-longan-nano/Makefile.include
+++ b/boards/sipeed-longan-nano/Makefile.include
@@ -1,3 +1,7 @@
 PORT_LINUX ?= /dev/ttyACM0
 PROGRAMMER ?= dfu-util
+
+ifneq (,$(filter st7735,$(USEMODULE)))
+  CFLAGS += '-DCONFIG_SIPEED_LONGAN_NANO_WITH_TFT=1'
+endif
 include $(RIOTBOARD)/common/gd32v/Makefile.include

would fix the mismatch, as without KConfig the TFT display driver is pulled in by disp_dev no matter what, but the board config part of the TFT driver needs to be enabled by hand. The above diff would make sure that the board config is pulled in when the driver is used.

@riot-ci
Copy link

riot-ci commented Jul 10, 2023

Murdock results

✔️ PASSED

ce653b7 boards/sipeed-longan-nano: default to TFT version if st7735 is used

Success Failures Total Runtime
512 0 512 05m:29s

Artifacts

@gschorcht gschorcht removed the CI: full build disable CI build filter label Jul 10, 2023
@gschorcht
Copy link
Contributor Author

would fix the mismatch, as without KConfig the TFT display driver is pulled in by disp_dev no matter what, but the board config part of the TFT driver needs to be enabled by hand. The above diff would make sure that the board config is pulled in when the driver is used.

Ok let's try this change.

@gschorcht
Copy link
Contributor Author

@maribu It's still an open issue. Should I squash so that we can try the next round?

@benpicco
Copy link
Contributor

Please squash, this is such a simple change now

@maribu
Copy link
Member

maribu commented Jul 12, 2023

It will not fix the issue, though:

  • With KConfig,the correct driver and params are pulled in by disp_dev
  • Without KConfig, the correct driver is pulled in without the params

The diff in the params will result in mismatching firmwares. See #19814 (comment) for a patch

@gschorcht gschorcht force-pushed the boards/sipeed-longan-nano/revert_kconfig_fix branch from 0c24c6c to ce653b7 Compare July 12, 2023 13:56
@gschorcht
Copy link
Contributor Author

The diff in the params will result in mismatching firmwares. See #19814 (comment) for a patch

Sorry I missed the diff for Makefile.include. I pushed now this change also.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Jul 12, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 7dd7d1e into RIOT-OS:master Jul 12, 2023
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
bors bot added a commit that referenced this pull request Aug 7, 2023
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



Co-authored-by: Gunar Schorcht <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
bors bot added a commit that referenced this pull request Aug 7, 2023
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 

Co-authored-by: Gunar Schorcht <[email protected]>
bors bot added a commit that referenced this pull request Aug 7, 2023
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]>
@gschorcht gschorcht deleted the boards/sipeed-longan-nano/revert_kconfig_fix branch September 3, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants