-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add missing display examples (MAX32672FTHR & MAX32665EvKIT) #378
Conversation
1e40078
to
b3c27bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source and header files in the Examples are missing copyright notices. I don't know if the copyright notice in Libraries/LVGL/lvgl/LICENCE.txt also extends toward the new Display Examples' source and header files.
But your changes looks good.
/* | ||
* test_screen.h | ||
* | ||
* Created on: Dec 7, 2022 | ||
* Author: AKara | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a copyright notice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
@@ -0,0 +1,42 @@ | |||
#include "test_screen.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a copyright notice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
@@ -0,0 +1,35 @@ | |||
#if defined(LV_LVGL_H_INCLUDE_SIMPLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a copyright notice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
#if defined(LV_LVGL_H_INCLUDE_SIMPLE) | ||
#include "lvgl.h" | ||
#else | ||
#include "lvgl/lvgl.h" | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright notice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, we've needed this!
See review notes below. I think there is a simple change to the build system we can make to simplify things. There is also a more long-term question about consolidating to the common display.h
approach for our other drivers
@@ -67,6 +81,73 @@ const mxc_gpio_cfg_t led_pin[] = { | |||
}; | |||
const unsigned int num_leds = (sizeof(led_pin) / sizeof(mxc_gpio_cfg_t)); | |||
|
|||
#ifndef NO_DISPLAY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we should reverse the logic here and change this option to
#ifdef ENABLE_DISPLAY
... or some similar name.
Otherwise all examples that do not use the display will need to manually disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
SBT=0 | ||
|
||
# Enforce to use FTHR board, the example works on FTHR only. | ||
BOARD = FTHR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
override BOARD = FTHR
to force FTHR in VS Code/Eclipse too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
Libraries/LVGL/lvgl.mk
Outdated
############################################################################### | ||
|
||
ifeq "$(LVGL_DIR)" "" | ||
$(error LVGL_DIR must be specified") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this Makefile self-locating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
LIB_LVGL ?= 0 | ||
ifeq ($(LIB_LVGL), 1) | ||
LVGL_DIR ?= $(LIBS_DIR)/LVGL | ||
include $(LVGL_DIR)/lvgl.mk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest exposing an ENABLE_DISPLAY
option here with a default of 1.
# libs.mk
# lvgl (Disabled by default)
# ************************
LIB_LVGL ?= 0
ifeq ($(LIB_LVGL), 1)
LVGL_DIR ?= $(LIBS_DIR)/LVGL
ENABLE_DISPLAY ?= 1 # <-- (expose option here)
include $(LVGL_DIR)/lvgl.mk
endif
# lvgl.mk
# ...
ifeq($(ENABLE_DISPLAY),1)
PROJ_CFLAGS += -DENABLE_DISPLAY # <-- (set compiler definition here)
endif
# ...
Combined with reversing the compiler definition logic above this will have the following effect:
- if
LIB_LVGL
is 1, the display is automatically enabled. - if
LIB_LVGL
is 0, the display is disabled
Therefore LIB_LVGL=1
in project.mk is enough, and we do not need to manually maintain the display enable/disable for each example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
int (*write)(uint8_t *data, uint32_t data_len); | ||
uint8_t *comm_buffer; | ||
uint32_t comm_buffer_len; | ||
} display_comm_api; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-adding a common display header is a good change. I think we should consider this for use with the other displays and think about the common API a little more.
I have implemented the following scheme for the st7789v drivers:
- Declare SPI transport layer functions as "extern" (Init/Read/Write): https://github.com/Analog-Devices-MSDK/msdk/blob/7c709440f43bec33167c26b1491d0017084edee0/Libraries/MiscDrivers/Display/tft_st7789v.h#L148
- Implement them in board.c: https://github.com/Analog-Devices-MSDK/msdk/blob/7c709440f43bec33167c26b1491d0017084edee0/Libraries/Boards/MAX78000/EvKit_V1/Source/board.c#L92
This is very similar to the approach you have taken here. So I wonder if we can consolidate the other drivers to also use this common header, and choose between the "extern" approach or this common struct with function pointers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets apply this change by another dedicated PR.
@@ -18,6 +18,9 @@ MXC_OPTIMIZE_CFLAGS=-Os | |||
# Strip debug symbols | |||
DEBUG=0 | |||
|
|||
# To close display related code in board.c file | |||
PROJ_CFLAGS+=-DNO_DISPLAY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be necessary - see other review notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see 074bb88
- Remove unneeded includes - Run clang format - Update clang format script to pass lvgl lib too - Remove uneeded LVGL_LIB flag - Update examples list - Update library makefile Signed-off-by: Sadik.Ozer <sadik.ozer@analog.com>
Signed-off-by: Sadik.Ozer <sadik.ozer@analog.com>
Signed-off-by: Sadik.Ozer <sadik.ozer@analog.com>
Signed-off-by: Sadik.Ozer <sadik.ozer@analog.com>
beca947
to
074bb88
Compare
- Change #include "lvgl/lvgl.h" to "lvgl.h" so MSDKGen can detect LVGL usage - Regenerate Eclipse and VS Code project files to add LVGL search paths - Regenerate core Makefile (minor tweaks, update links to online UG)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ozersa, looks good!
FYI I updated the project files with MSDKGen in 3b56385 and bc8dc2e to add the LVGL paths to Eclipse/VS Code.
Let me know if it looks good to merge.
For a future PR:
- Enable MiscDrivers for all micros (we still have a lot that are still using
led.c
/pb.c
, etc. in their own boards directory) - Use common display.h API for all Display drivers
@Jake-Carter It is ok, please feel free to merge it. Thanks. |
MAX32665EvKit and MAX32672FTHR have display but there are driver and use case examples.
Added display examples for these devices, examples developed based on LVGL library.
Examples: