Introduce the cfb_shell sample app #11072

diegosueiro · 2018-11-04T09:17:25Z

This PR adds:

cfb_framebuffer_invert function to invert the pixels in the character framebuffer;
cfb_get_numof_fonts function to get the number of available fonts
a character framebuffer shell module;
a sample app to test the character framebuffer shell module.

All tested and validated with the reel_board: cmake -DBOARD=reel_board ../..

codecov-io · 2018-11-04T09:51:44Z

Codecov Report

Merging #11072 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #11072     +/-   ##
=========================================
- Coverage   48.23%   48.13%   -0.1%     
=========================================
  Files         279      279             
  Lines       43291    43278     -13     
  Branches    10359    10358      -1     
=========================================
- Hits        20881    20833     -48     
- Misses      18270    18302     +32     
- Partials     4140     4143      +3

Impacted Files	Coverage Δ
subsys/logging/log_output.c	`62.38% <0%> (-10%)`	⬇️
subsys/logging/log_core.c	`70.85% <0%> (-4.69%)`	⬇️
misc/printk.c	`81.81% <0%> (-3.13%)`	⬇️
include/logging/log_msg.h	`89.09% <0%> (-1.82%)`	⬇️
include/logging/log_core.h	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61ccafa...65aa31b. Read the comment docs.

vanwinkeljan

On top of the inline comments please add a README.rst for the sample.

subsys/fb/Kconfig

subsys/fb/cfb_shell.c

vanwinkeljan · 2018-11-06T19:05:45Z

@diegosueiro Code looks good to me but could you add a README.rst file to the sample then it will be visible from zephyrs documentation?

diegosueiro · 2018-11-11T15:45:43Z

@vanwinkeljan and @jfischer-phytec-iot ,

Any more comments on this PR?

vanwinkeljan · 2018-11-11T16:36:54Z

@diegosueiro No only open comment from my side is the missing README.rst

nashif · 2018-11-11T18:11:30Z

samples/display/cfb_shell/prj.conf

+CONFIG_DISPLAY=y
+
+CONFIG_LOG=y
+CONFIG_DISPLAY_LOG_LEVEL_DBG=y


do not enable debug logging by default.

nashif · 2018-11-11T18:11:37Z

samples/display/cfb_shell/prj.conf

+CONFIG_LOG=y
+CONFIG_DISPLAY_LOG_LEVEL_DBG=y
+
+CONFIG_CFB_LOG_LEVEL_DBG=y


diegosueiro · 2018-11-11T19:01:48Z

@nashif ,

Do you think we should move from samples/display/cfb_shell to samples/subsys/shell/cfb_module/ ?

dbkinder · 2018-11-11T23:23:03Z

samples/display/cfb_shell/README.rst

+
+Overview
+********
+A simple shell module that exercises displays using the Character Framebuffer


Suggested change

A simple shell module that exercises displays using the Character Framebuffer

This is a simple shell module that exercises displays using the Character Framebuffer

dbkinder · 2018-11-11T23:23:41Z

samples/display/cfb_shell/README.rst

+Building and Running
+********************
+
+Build the sample app by choosing the OVERLAY_CONFIG for the target board. E.g.:


Suggested change

Build the sample app by choosing the OVERLAY_CONFIG for the target board. E.g.:

Build the sample app by choosing the OVERLAY_CONFIG for the target board, for example:

dbkinder · 2018-11-11T23:28:02Z

samples/display/cfb_shell/README.rst

+Shell Module Command Help
+=========================
+
+.. code-block:: console


While this shows the command syntax, it doesn't explain what the (sub)commands actually do.

Aren't the command names self-explanatory?

For the uninitiated, such as me, no. I don't know what the commands do. I'll yield to the wisdom of other reviewers.

I'm guessing the framebuffer is sized exactly as the display so the scroll command is not working with a larger buffer: I can't scroll up 5 rows and down 5 rows and the display would be restored. Will the print command wrap text when hitting the edge of the display or scroll at the bottom? Do I have to call init before any other command? Are the <col:pos> arguments "character" size (not pixels), so I can't scroll half a character height?

When you say font I'm assuming you're talking about a statically defined font-family + font-weight + font-size. Are all characters on the display the same font? Can I change the font after some characters are displayed (what happens)? What is ppt?

For get_param, I'm guessing height/width units are pixels (and a fixed value for a given display), and rows/cols depends on the size of the current font that's set?

I agree with @dbkinder here, anything is better than "[none]" and even for those with help, it is not really easy for someone not familiar with the feature to do anything simple.

dbkinder · 2018-11-12T22:54:09Z

samples/display/cfb_shell/README.rst

+Shell Module Command Help
+=========================
+
+.. code-block:: console


For the uninitiated, such as me, no. I don't know what the commands do. I'll yield to the wisdom of other reviewers.

I'm guessing the framebuffer is sized exactly as the display so the scroll command is not working with a larger buffer: I can't scroll up 5 rows and down 5 rows and the display would be restored. Will the print command wrap text when hitting the edge of the display or scroll at the bottom? Do I have to call init before any other command? Are the <col:pos> arguments "character" size (not pixels), so I can't scroll half a character height?

When you say font I'm assuming you're talking about a statically defined font-family + font-weight + font-size. Are all characters on the display the same font? Can I change the font after some characters are displayed (what happens)? What is ppt?

For get_param, I'm guessing height/width units are pixels (and a fixed value for a given display), and rows/cols depends on the size of the current font that's set?

dbkinder · 2018-11-12T23:03:23Z

samples/display/cfb_shell/README.rst

+                 -h, --help  :Show command help.
+         Subcommands:
+                 init        :[none]
+                 get_param   :<all, heigh, width, ppt, rows, cols>


diegosueiro · 2018-11-15T05:56:00Z

@dbkinder and @nashif ,

Any more comments?

dbkinder

LGTM but one typo

dbkinder · 2018-11-15T06:39:31Z

samples/display/cfb_shell/README.rst

+
+**get_device**: prints the display device name.
+
+**get_param**: get the display parameters where heigh, width and ppt


addressed

nashif · 2018-11-21T20:19:30Z

@diegosueiro Can you provide as an example a list of commands that can entered on the shell to allow someone unfamiliar to get going quickly, like print "reel board" in different way, scroll it, invert it, flip it, you name it....

addressed

dbkinder

Sorry to pick on your sample, but there's no documentation elsewhere about the CFB API. Maybe some of this wouldn't be necessary if that were documented.

dbkinder · 2018-11-21T21:42:24Z

samples/display/cfb_shell/README.rst

+
+**print**: pass the initial column and row positions and the text in
+double quotation marks when it contains spaces. If text hits the edge
+the display the remain characters will be displayed in the next line. The


Suggested change

the display the remain characters will be displayed in the next line. The

of the display the remaining characters will be displayed on the next line. The

dbkinder · 2018-11-21T21:43:17Z

samples/display/cfb_shell/README.rst

+(pixel per tile) are in pixels and the number of rows and columns. The row
+position is incremented by a multiple of the ppt.
+
+**get_fonts**: print the index, height and width in pixels of the static


how do you install a new font?

Installing new fonts is not covered by this sample. And, AFAIK, you can only install new fonts at build time.
The PR #11224 is covering this.

dbkinder · 2018-11-21T21:46:27Z

samples/display/cfb_shell/README.rst

+display boundary, last column for horizontal and last row for vertical
+direction.
+
+**clear**: clear the display screen.


It would be great, after these descriptions, to show a nice example of how to display a phrase on the display and manipulate it (and have pictures or describe what gets displayed).

I can put command examples as @nashif suggested, but at the moment there is no API, AFAIK, to draw pictures, or manipulated what is printed like inversion.

@diegosueiro I think what @dbkinder means is to give examples with screenshots in the docs.
But on adding the screenshots in the docs I already raised the question (PR #9149) if it is allowed to add binary files into the git repo. In my opinion this is not a good idea and git-lfs is not setup for the zephyr repo (@nashif any inputs on this?)

Regarding the API for drawing you have PR #6826 if you need something more fancy than CFB. CFB is just a lightweight framework for displaying text where LVGL is a full blown GUI library and I think they complement each other nicely

diegosueiro · 2018-11-22T06:42:22Z

@diegosueiro Can you provide as an example a list of commands that can entered on the shell to allow someone unfamiliar to get going quickly, like print "reel board" in different way, scroll it, invert it, flip it, you name it....

I will add examples.

Do you think we should move from samples/display/cfb_shell/ to samples/subsys/shell/cfb_module/

zephyrbot · 2018-11-22T07:55:53Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

diegosueiro · 2018-11-27T09:46:20Z

@dbkinder ,

Anything more is needed?

dbkinder · 2018-11-27T17:43:56Z

samples/display/cfb_shell/README.rst

+
+         uart:~$ cfb print 60 5 ZEPHYR
+
+**scroll**: pass the scroll direction, vertical or horizontal, the initial


Thanks for the update and examples! I'm still a bit confused about the difference between "print" and "scroll". Is "print" the same as "scroll vertical"?

@dbkinder,
The scroll command will "move" vertically or horizontally the text from the initial position to the display boundaries. And the print will only display the text without any movement.

Ah, so "scroll" inserts the text and moves any existing text on the display, and "print" will overwrite existing text? That's worth clarifying.

Nope. The scroll will move the text that you passed with the scroll command. What is already on the display will be overwritten as well.

dbkinder

Did I get that right?

dbkinder · 2018-11-27T18:05:21Z

samples/display/cfb_shell/README.rst

+**print**: pass the initial column and row positions and the text in
+double quotation marks when it contains spaces. If text hits the edge
+of the display the remaining characters will be displayed on the next line. The
+previous printed text will be erased.


Suggested change

previous printed text will be erased.

previous printed text will be overwritten.

dbkinder · 2018-11-27T18:07:28Z

samples/display/cfb_shell/README.rst

+contains spaces. If text hits the edge the display the remaining characters
+will be displayed in the next line. The text will scroll until it hits the
+display boundary, last column for horizontal and last row for vertical
+direction.


Suggested change

direction.

direction. Existing text on the display will be moved, potentially off the display.

Adds the cfb_framebuffer_invert function to invert Pixels in the Character Framebuffer Signed-off-by: Diego Sueiro <[email protected]>

Adds the cfb_get_numof_fonts function to get the number of available fonts Signed-off-by: Diego Sueiro <[email protected]>

This adds the config option and files of the Character Framebuffer shell module Signed-off-by: Diego Sueiro <[email protected]>

This adds the shell sample app for the Character Framebuffer testing. cfb - Character Framebuffer shell commands Options: -h, --help :Show command help. Subcommands: init :[none] get_device :[none] get_param :<all, height, width, ppt, rows, cols> get_fonts :[none] set_font :<idx> invert :[none] print :<col: pos> <row: pos> <text> scroll :<dir: (vertical|horizontal)> <col: pos> <row: pos> <text> clear :[none] Signed-off-by: Diego Sueiro <[email protected]>

diegosueiro · 2018-12-04T11:43:56Z

@dbkinder,

Any more comments on the documentation?

addressed

diegosueiro requested a review from vanwinkeljan as a code owner November 4, 2018 09:17

vanwinkeljan requested a review from jfischer-no November 4, 2018 10:28

diegosueiro force-pushed the cfb_shell branch from c897331 to 58d02a9 Compare November 4, 2018 10:48

vanwinkeljan requested changes Nov 4, 2018

View reviewed changes

diegosueiro force-pushed the cfb_shell branch from 58d02a9 to 87d41ca Compare November 4, 2018 14:33

vanwinkeljan added the area: Display label Nov 6, 2018

nashif previously requested changes Nov 11, 2018

View reviewed changes

diegosueiro force-pushed the cfb_shell branch from 87d41ca to ec858b9 Compare November 11, 2018 19:26

diegosueiro requested a review from dbkinder as a code owner November 11, 2018 19:26

dbkinder suggested changes Nov 11, 2018

View reviewed changes

diegosueiro force-pushed the cfb_shell branch from ec858b9 to 0ae6c4d Compare November 12, 2018 05:43

dbkinder suggested changes Nov 12, 2018

View reviewed changes

diegosueiro force-pushed the cfb_shell branch 2 times, most recently from 49e3058 to 10d512b Compare November 13, 2018 07:38

vanwinkeljan approved these changes Nov 13, 2018

View reviewed changes

dbkinder previously requested changes Nov 15, 2018

View reviewed changes

diegosueiro force-pushed the cfb_shell branch 2 times, most recently from a656e92 to 3277016 Compare November 15, 2018 08:42

dbkinder suggested changes Nov 21, 2018

View reviewed changes

diegosueiro force-pushed the cfb_shell branch from 3277016 to 5d4d8bf Compare November 22, 2018 07:06

diegosueiro force-pushed the cfb_shell branch from 5d4d8bf to a43a864 Compare November 22, 2018 17:28

dbkinder suggested changes Nov 27, 2018

View reviewed changes

dbkinder previously requested changes Nov 27, 2018

View reviewed changes

diegosueiro added 4 commits November 27, 2018 18:19

subsys/fb/cfb: Introduce cfb_framebuffer_invert function

03ed899

Adds the cfb_framebuffer_invert function to invert Pixels in the Character Framebuffer Signed-off-by: Diego Sueiro <[email protected]>

subsys/fb/cfb: Introduce cfb_get_numof_fonts function

2b20fda

Adds the cfb_get_numof_fonts function to get the number of available fonts Signed-off-by: Diego Sueiro <[email protected]>

shell: Add initial code for Character Framebuffer shell module

6592428

This adds the config option and files of the Character Framebuffer shell module Signed-off-by: Diego Sueiro <[email protected]>

diegosueiro force-pushed the cfb_shell branch from a43a864 to 65aa31b Compare November 27, 2018 18:40

nashif approved these changes Dec 6, 2018

View reviewed changes

nashif merged commit 48f4932 into zephyrproject-rtos:master Dec 6, 2018

diegosueiro mentioned this pull request Dec 7, 2018

[HOTFIX #11072] Introduce the cfb_shell sample app #11923

Merged

	A simple shell module that exercises displays using the Character Framebuffer
	This is a simple shell module that exercises displays using the Character Framebuffer

	Build the sample app by choosing the OVERLAY_CONFIG for the target board. E.g.:
	Build the sample app by choosing the OVERLAY_CONFIG for the target board, for example:


		get_device: prints the display device name.

		get_param: get the display parameters where heigh, width and ppt

	the display the remain characters will be displayed in the next line. The
	of the display the remaining characters will be displayed on the next line. The


		uart:~$ cfb print 60 5 ZEPHYR

		scroll: pass the scroll direction, vertical or horizontal, the initial

	previous printed text will be erased.
	previous printed text will be overwritten.

	direction.
	direction. Existing text on the display will be moved, potentially off the display.

Introduce the cfb_shell sample app #11072

Introduce the cfb_shell sample app #11072

Conversation

diegosueiro commented Nov 4, 2018 • edited Loading

codecov-io commented Nov 4, 2018 • edited Loading

Codecov Report

vanwinkeljan left a comment

Choose a reason for hiding this comment

vanwinkeljan commented Nov 6, 2018

diegosueiro commented Nov 11, 2018

vanwinkeljan commented Nov 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro commented Nov 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro commented Nov 15, 2018

dbkinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashif commented Nov 21, 2018

dbkinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro commented Nov 22, 2018

zephyrbot commented Nov 22, 2018 • edited Loading

diegosueiro commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro commented Dec 4, 2018

diegosueiro commented Nov 4, 2018 •

edited

Loading

codecov-io commented Nov 4, 2018 •

edited

Loading

diegosueiro Nov 22, 2018 •

edited

Loading

zephyrbot commented Nov 22, 2018 •

edited

Loading