Skip to content

Conversation

@TimJTi
Copy link
Contributor

@TimJTi TimJTi commented Mar 18, 2025

Summary

This adds a new "Example App" that allows a framebuffer character device to be used for STDOUT and/or STDERR text rendering.

Impact

It is a standalone app so has no impact on anything else

Testing

On a custom board (SAMA5D27C-D1g with 800x480 TFT

@nuttxpr
Copy link

nuttxpr commented Mar 18, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

Yes, this PR appears to mostly meet the requirements, but is missing some key information.

Here's what needs improvement:

  • Summary: While the "what" is explained, the "why" is missing. Why is this new example app necessary? What use case does it address? Is there a demand for this functionality?
  • Impact: While it probably doesn't impact other components, explicitly stating "NO" for each impact category is preferred for clarity. For example: Impact on user (will user need to adapt to change)? NO. This avoids ambiguity.
  • Testing: The testing section is severely lacking.
    • Build Host(s): Specify the OS, CPU architecture, and compiler version used for building. (e.g., Linux x86_64, GCC 12.2.1)
    • Target(s): Be more specific. Instead of just "custom board", provide the NuttX architecture (e.g., armv7-m) and the specific board configuration name used (e.g., sama5d27c_d1g_som1ek1).
    • Logs: "Testing logs" are crucial. Provide actual output demonstrating the application's functionality both before and after the change (though "before" might be N/A if it's a new app). Show that the app works. Example: output of hello world or similar on the framebuffer. If stdin is involved, show how that works too.

By providing this missing information, the PR will be much more complete and easier to review.

@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 18, 2025

Just spotted there's a left over folder and CMakeLists.txt from when I called it something else. Will remove that with any future commits here, and before I make it ready for review after the first round of checks

@cederom
Copy link
Contributor

cederom commented Mar 19, 2025

Thanks @TimJTi amazing work!! :-)

Please remember to update git commit:

  • examples/fbcon: Add video framebuffer console (or something like that).
  • some description.
  • git commit -s

Some documentation would be nice to have too on how to setup hardware so we could test on other targets :-)

@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 19, 2025

@cederom

Thanks @TimJTi amazing work!! :-)
Thank you :-D

Please remember to update git commit:

  • examples/fbcon: Add video framebuffer console (or something like that).
  • some description.
  • git commit -s

As per usual, finger trouble with git...I was sure there was a commit message lol

Some documentation would be nice to have too on how to setup hardware so we could test on other targets :-)

I was hoping the Kconfig help would explain how to use it but I can add documentation somewhere if wanted - where would you suggest and any example of similar I could refer to?

@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 19, 2025

I am going to add a Kconfig option for whether to intercept/redirect STDIN - it's necessary for NSH but probably is not if the FB is just used for output from the majority of otgher apps/tasks.

It was suggested that I could add a splashscreen to this so NuttX could power up with a NuttX logo...but on reflection I think that would be better in NXBOOT?

@acassis
Copy link
Contributor

acassis commented Mar 19, 2025

I am going to add a Kconfig option for whether to intercept/redirect STDIN - it's necessary for NSH but probably is not if the FB is just used for output from the majority of otgher apps/tasks.

It was suggested that I could add a splashscreen to this so NuttX could power up with a NuttX logo...but on reflection I think that would be better in NXBOOT?

Yes, I think we need to have something similar to boot logo like Linux where the number of CPUs could be displayed as the number of NuttX logos. But I think it should be in the kernel, not in fbcon application

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

Just remove fbterm

@TimJTi TimJTi force-pushed the Add-fbcon-example branch from 6262881 to 480a5ee Compare March 19, 2025 14:49
on to the spawned app.

Relies on and uses NX and NXFONTS

Copy link
Contributor

@simbit18 simbit18 Mar 19, 2025

Choose a reason for hiding this comment

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

line 20 remove tabs

string "Default framebuffer driver"
default "/dev/fb0"
---help---
Default framebuffer drivers. This selection can be overridden from
Copy link
Contributor

Choose a reason for hiding this comment

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

add tab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - had a VS Code moment as it lost all my language specific rules. Doh!
Will fix on next push

@TimJTi TimJTi force-pushed the Add-fbcon-example branch 2 times, most recently from 5bcc0af to b1dd4ce Compare March 19, 2025 18:29
@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 19, 2025

@cederom @acassis

Some documentation would be nice to have too on how to setup hardware so we could test on other targets :-)

I have added a bit more meat to Kconfig - shall I embelish that in an index.rst file location in the relevant NuttX documentation folder? As a separate commit?

@TimJTi TimJTi marked this pull request as ready for review March 19, 2025 22:46
@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 20, 2025

I am going to add a Kconfig option for whether to intercept/redirect STDIN - it's necessary for NSH but probably is not if the FB is just used for output from the majority of otgher apps/tasks.
It was suggested that I could add a splashscreen to this so NuttX could power up with a NuttX logo...but on reflection I think that would be better in NXBOOT?

Yes, I think we need to have something similar to boot logo like Linux where the number of CPUs could be displayed as the number of NuttX logos. But I think it should be in the kernel, not in fbcon application

@acassis Uboot allows for a splashscreen, and Uboot is run before NuttX. Using NXboot, although it is an app, it only persists for a short while, so I am thinking there is perhaps some merit in having a splashscreen option within the NXboot app? But only for Framebuffer displays I think?

What do you think? Shall I start a separate discussion topic?

@TimJTi TimJTi force-pushed the Add-fbcon-example branch from b1dd4ce to 1e527aa Compare March 20, 2025 11:11
@acassis
Copy link
Contributor

acassis commented Mar 20, 2025

I am going to add a Kconfig option for whether to intercept/redirect STDIN - it's necessary for NSH but probably is not if the FB is just used for output from the majority of otgher apps/tasks.
It was suggested that I could add a splashscreen to this so NuttX could power up with a NuttX logo...but on reflection I think that would be better in NXBOOT?

Yes, I think we need to have something similar to boot logo like Linux where the number of CPUs could be displayed as the number of NuttX logos. But I think it should be in the kernel, not in fbcon application

@acassis Uboot allows for a splashscreen, and Uboot is run before NuttX. Using NXboot, although it is an app, it only persists for a short while, so I am thinking there is perhaps some merit in having a splashscreen option within the NXboot app? But only for Framebuffer displays I think?

What do you think? Shall I start a separate discussion topic?

Yes, maybe you can discuss this idea in the mailing list. Remember: NXBoot is a NuttX with a boot application, so if we get a "nxlogo" integrated in the kernel it could be displayed earlier.

@acassis acassis merged commit 3e37dd2 into apache:master Mar 20, 2025
37 checks passed
@TimJTi TimJTi deleted the Add-fbcon-example branch March 20, 2025 16:59

config EXAMPLES_FBCON_CUSTOM_BPP
bool "Choose custom BPP (must not be disabled in NX)"

Copy link
Contributor

Choose a reason for hiding this comment

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

We are missing endchoice here. This causes the build to break with errors like:

mkkconfig in /NuttX/apps
/NuttX/apps/examples/fbcon/Kconfig:62: syntax error
/NuttX/apps/examples/fbcon/Kconfig:51: missing end statement for this entry
/NuttX/apps/examples/fbcon/Kconfig:23: missing end statement for this entry
/NuttX/apps/examples/Kconfig:8: missing end statement for this entry
Kconfig:2785: missing end statement for this entry
/NuttX/apps/examples/fbcon/Kconfig:61: invalid statement
/NuttX/apps/examples/fbcon/Kconfig:62: unexpected option "prompt"
/NuttX/apps/examples/fbcon/Kconfig:63: unexpected option "depends"
/NuttX/apps/examples/fbcon/Kconfig:64: invalid statement
/NuttX/apps/examples/fbcon/Kconfig:65:warning: ignoring unsupported character ':'
/NuttX/apps/examples/fbcon/Kconfig:65: unknown statement "Note"
/NuttX/apps/examples/fbcon/Kconfig:95: unexpected end statement
/NuttX/apps/examples/fbcon/Kconfig:96: unexpected end statement
/NuttX/apps/examples/fbcon/Kconfig:173: syntax error
/NuttX/apps/examples/fbcon/Kconfig:172: unknown option "---help--"
/NuttX/apps/examples/fbcon/Kconfig:173: unknown option "Decode"
/NuttX/apps/examples/fbcon/Kconfig:205: unexpected end statement
/NuttX/apps/examples/Kconfig:211: unexpected end statement
Kconfig:2787: unexpected end statement
make: *** [tools/Unix.mk:726: olddefconfig] Error 1
ERROR: failed to refresh

Unfortunately I didn't catch it until after merged...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm...how come I didn't see this? Any suggestions on how I could have caught it when check patch and CI didn't?

Copy link
Contributor

Choose a reason for hiding this comment

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

@TimJTi I'm not sure, could it be that the line was there during checks but got deleted somehow at the last second?

Copy link
Member

@lupyuen lupyuen Mar 21, 2025

Choose a reason for hiding this comment

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

Some RISC-V Builds are failing, I submitted the patch here:

Screenshot 2025-03-21 at 9 40 36 AM

Copy link
Member

Choose a reason for hiding this comment

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

Any suggestions on how I could have caught it when check patch and CI didn't?

If I remember correctly: Our CI Check uses kconfiglib but our Local Builds use kconfig-frontends. That's why Kconfigs are checked differently:

Hi @simbit18: Am I correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lupyuen the use of two tools can have different behaviors. Then you also need a style control tool for the kconfig file. There is no shortage of fun in NuttXland !!! :)


choice EXAMPLES_FBCON_BPP_SELECTION
prompt "BPP Configuration"
default EXAMPLES_FBCON_BPP_NX_DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

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

depends on?

@cederom
Copy link
Contributor

cederom commented Mar 21, 2025

Hmm was this PR marked DRAFT and merged? We need to be more careful as it broke master build for all other boards/config even those not using fbcon. DRAFT means NO MERGE! :-)

Things to update (in next PR):

  1. Kconfig dependencies are missing (i.e. NX Graphics, fonts, bit depths, fifo pipes). In best case selecting this functionality should also enable required underlying functionalities :-)
  2. Documentation on how to setup and use this functionality :-)
  3. Possible conflicts outline - if I use LVGL demo config would it be possible to use fbcon example too?
% gmake -j8 CROSSDEV=riscv32-esp-elf-
Create version.h
LN: platform/board to /XXX/nuttx-apps.git/platform/dummy
Register: lvgldemo
Register: fbcon
Register: nsh
Register: sh
./fbcon_main.c:51:4: error: #error This application requires NX Graphics
 #  error This application requires NX Graphics
    ^~~~~
./fbcon_main.c:57:4: error: #error FIFO and Named Pipe Drivers should be enabled in the configuration
 #  error FIFO and Named Pipe Drivers should be enabled in the configuration
    ^~~~~
./fbcon_main.c:176:6: error: #error NXFONT_DEFAULT not defined
 #    error NXFONT_DEFAULT not defined
      ^~~~~
CC:  dirent/lib_versionsort.c fbcon_main.c:51:4: error: #error This application requires NX Graphics
 #  error This application requires NX Graphics
    ^~~~~
fbcon_main.c:57:4: error: #error FIFO and Named Pipe Drivers should be enabled in the configuration
 #  error FIFO and Named Pipe Drivers should be enabled in the configuration
    ^~~~~
fbcon_main.c:176:6: error: #error NXFONT_DEFAULT not defined
 #    error NXFONT_DEFAULT not defined
      ^~~~~
CC:  sched/sched_rrgetinterval.c fbcon_main.c: In function 'fbcon_renderglyph':
fbcon_main.c:1062:22: warning: passing argument 1 of 'nxf_convert_1bpp' from incompatible pointer type [-Wincompatible-pointer-types]
       ret = RENDERER((FAR nxgl_mxpixel_t *)glyph->bitmap,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from fbcon_main.c:41:
/XXX/nuttx.git/include/nuttx/nx/nxfonts.h:553:35: note: expected 'uint8_t *' {aka 'unsigned char *'} but argument is of type 'nxgl_mxpixel_t *' {aka 'long unsigned int *'}
 int nxf_convert_1bpp(FAR uint8_t *dest, uint16_t height,
                          ~~~~~~~~~^~~~
fbcon_main.c: In function 'fbcon_initialize':
fbcon_main.c:178:24: error: 'NXFONT_DEFAULT' undeclared (first use in this function); did you mean 'FONTID_DEFAULT'?
 #  define FBCON_FONTID NXFONT_DEFAULT
                        ^~~~~~~~~~~~~~

@lupyuen
Copy link
Member

lupyuen commented Mar 21, 2025

@cederom I think it was marked Ready For Review yesterday?

@cederom
Copy link
Contributor

cederom commented Mar 21, 2025

Cool cool at least we have more focus now :D :D

lupyuen added a commit that referenced this pull request Mar 21, 2025
This PR fixes the `endchoice` and `help` in the Kconfig for fbcon
- #3029 (comment)

This patch will fix these build errors:
```text
apps/examples/fbcon/Kconfig:62: syntax error
apps/examples/fbcon/Kconfig:172: unknown option "---help--"
```
https://github.com/lupyuen/nuttx-riscv64/actions/runs/13981663903/job/39147989438#step:5:158

Signed-off-by: Lup Yuen Lee <luppy@appkaki.com>
@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 21, 2025

@cederom I think it was marked Ready For Review yesterday?

@cederom @lupyuen It was merged very quickly after I marked it ready and - not check yet - I might hve pushed a new commit soon after as I'd not signed it.

  1. I think it was merger MUCH too soon after me marking it ready. It gave no time for others to review as it is qwuite reasonable they waited until it was no longer a draft
  2. Is it possible it was merged before the checks on the last push had finished CI? Is this visible to me or, if not, can someone check just be sure?

@lupyuen
Copy link
Member

lupyuen commented Mar 21, 2025

2. Is it possible it was merged before the checks on the last push had finished CI? Is this visible to me or, if not, can someone check just be sure?

CI Checks didn't catch the Kconfig Errors because it uses kconfiglib, but most of us use kconfig-frontends. So we wouldn't have known about the Kconfig Errors, even if we waited for the CI Checks to complete.

Here's a trick I use on my Bigger PRs: I set my PR to Draft Mode while CI Check is still running. It gives me more time to fix things if the CI Check fails :-)

@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 21, 2025

@cederom

Hmm was this PR marked DRAFT and merged? We need to be more careful as it broke master build for all other boards/config even those not using fbcon. DRAFT means NO MERGE! :-)

See comment below!

Things to update (in next PR):

  1. Kconfig dependencies are missing (i.e. NX Graphics, fonts, bit depths, fifo pipes). In best case selecting this functionality should also enable required underlying functionalities :-)

I recalled - but didn't go back and check tbh - a discussion saying that depends on was preferable to select?

Originally I did select (e.g.) NX but changed it to depends on with #define error

Sorry if I got it wrong...and reinforces my other reply that this was merged too soon IMHO.

  1. Documentation on how to setup and use this functionality :-)
    This was mentioned before and I asked where, and for pointers to similar for reference, and followed up with a suggested as to where? Still happy to do this :-)
  2. Possible conflicts outline - if I use LVGL demo config would it be possible to use fbcon example too?

I see this as a standalone example...but it can be built on of course. For example, I've just found that (perhaps of course, which is my inexperience showing) syslog messages got to /dev/console and that doesn't make its way through to FBCON.

It's almost like FBCON should be an actual driver (/dev/fbcon) not an app? That's why I opened a discussion on the mail list, and left it as a draft for a while to seek feedback ;-)

@TimJTi
Copy link
Contributor Author

TimJTi commented Mar 21, 2025

  1. Is it possible it was merged before the checks on the last push had finished CI? Is this visible to me or, if not, can someone check just be sure?

CI Checks didn't catch the Kconfig Errors because it uses kconfiglib, but most of us use kconfig-frontends. So we wouldn't have known about the Kconfig Errors, even if we waited for the CI Checks to complete.

Here's a trick I use on my Bigger PRs: I set my PR to Draft Mode while CI Check is still running. It gives me more time to fix things if the CI Check fails :-)

It was marked as a draft for many days with several pushes and successful CI runs...which is why I wonder if, after marked as ready for review and/or a last push from me, the last CI run wasn't allowed to, or didn't, run?

I always check CI results and fix broken things, and I do use kconfiglib...and it didn't report any issues but perhaps I forgot some method or setting to ensure it reported errors. Blame old age making me forgetful lol.

@cederom
Copy link
Contributor

cederom commented Mar 22, 2025

All good now :-) @hartmannathan found the issue and @lupyuen quickly fixed it, things like this happen sometimes, we just need to keep draft status until absolutely sure it can be merged and all work is complete :-) Have a good weekend folks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants