refactor(API): don't expose private symbols in lvgl.h. phase-out "_lv*" names #6068
Conversation
|
Progress report 🚧️ WIP 🚧️ The progress so far has been directed by getting the number of lines output by this command near zero. Derived from the WIP API JSON generator PR #5677. In particular the lv_conf.h is lv_conf_template.h with this applied and the It dumps all symbols visible in the lvgl.h header.
90% of the _lv symbols in lvgl.h have been hidden so far. The remaining are tricky cases that need to be reasoned about. i.e. Some APIs such as TODO:
|
src/widgets/roller/lv_roller.h
Outdated
| LV_ROLLER_MODE_NORMAL, /**< Normal mode (roller ends at the end of the options).*/ | ||
| LV_ROLLER_MODE_INFINITE, /**< Infinite mode (roller can be scrolled forever).*/ | ||
| }; | ||
|
|
||
| #ifdef DOXYGEN | ||
| typedef _lv_roller_mode_t lv_roller_mode_t; | ||
| typedef enum lv_roller_mode_t lv_roller_mode_t; | ||
| #else | ||
| typedef uint8_t lv_roller_mode_t; |
There was a problem hiding this comment.
We typedef some enums to uint8_t to allow defining them as a bitfields (e.g. lv_roller_mode_t mode : 1;)
However this practice is used inconsistently in LVGL now and I'm not sure if it's worth the loss in readability.
Could you update these like this?
typedef enum {
LV_ROLLER_MODE_NORMAL, /**< Normal mode (roller ends at the end of the options).*/
LV_ROLLER_MODE_INFINITE, /**< Infinite mode (roller can be scrolled forever).*/
}lv_roller_mode_t;
...
//In lv_roller_t
uint32_t mode : 1;
liamHowatt
force-pushed
the
refactor/api
branch
from
d6a41ea
to
c95bdff
Compare
| @@ -1,3 +1,4 @@ | |||
| #include "../../../src/others/observer/lv_observer_private.h" | |||
There was a problem hiding this comment.
The private header files shouldn't be required to "just use" a features. In this file on the basic observer features are used, so lv_observer_private.h shouldn't be needed.
|
|
||
| /**< Monkey execution period*/ | ||
| struct { | ||
| //! @cond Doxygen_Suppress |
There was a problem hiding this comment.
Why do we need this Doxygen comment?
There was a problem hiding this comment.
It got copy-pasted from the public header by my refactoring automation. I can remove it.
examples/anim/lv_example_anim_2.c
Outdated
| @@ -1,3 +1,4 @@ | |||
| #include "../../src/misc/lv_anim_private.h" | |||
There was a problem hiding this comment.
Same here and probably on a lot of other places too.
|
Hey @kisvegabor, it looks like losing APIs isn't going to be a problem. After I push a cleaned up commit, what are the things we want to do to get this merged? Here are all the structs with exposed members. Most of them are in the global singleton, which is why they have to be in this list. Should we add any more? i.e. anim?
Intentionally hidden APIs. It's just the linked list. I need to add LRU and cache, right?
Protos that are now public
Kept inline
Finally, I attached a file where each row gives a file name and then a private header that the file includes. |
|
I'm not sure why CI isn't running. FYI here are the JSONs generated by the API JSON generator.
|
|
Thank you Liam! Can we hide Is it possible? |
demos/widgets/lv_demo_widgets.c
Outdated
| #include "../../src/draw/lv_draw_triangle_private.h" | ||
| #include "../../src/draw/lv_draw_rect_private.h" | ||
| #include "../../src/draw/lv_draw_private.h" | ||
| #include "../../src/core/lv_obj_private.h" |
There was a problem hiding this comment.
The goal would be to not have any private.h includes in demos and examples. It would show that everything is public to use LVGL as a normal user.
| #define _LV_FLEX_COLUMN (1 << 0) | ||
| #define _LV_FLEX_WRAP (1 << 2) | ||
| #define _LV_FLEX_REVERSE (1 << 3) | ||
| #define LV_FLEX_COLUMN (1 << 0) |
There was a problem hiding this comment.
If remove the underscrore let's #undef these after lv_flex_flow_t.
There was a problem hiding this comment.
Hmm, I agree but the .c file depends on these defines. And I can't put them in a private header because this public header needs them to define the enum.
|
@kdschlosser The failing MP CI probably can be fixed by including |
|
the failing CI is more than likely because of /*Garbage Collector settings
*Used if lvgl is bound to higher level language and the memory is managed by that language*/
extern void mp_lv_init_gc();
#define LV_GC_INIT() mp_lv_init_gc()
#define LV_ENABLE_GLOBAL_CUSTOM 1
#if LV_ENABLE_GLOBAL_CUSTOM
extern void *mp_lv_roots;
#define LV_GLOBAL_CUSTOM() ((lv_global_t*)mp_lv_roots)
#endifThat stuff needs to remain public and not private. If we include the private headers into the build the size of the firmware is going to become a lot larger. This is due to the number of additional structures that are going to get added resulting in a large number of qstrings being added. |
|
the other thing you have to keep a really close eye out for is when you make structures private and they hold callback functions if the type for the function pointer is not defined as having the user_data passed as the last parameter then it tests to see if the first parameter is a structure and if it is then it looks for a user_data field in that structure. If the structure has been made private it is not going to be able to locate the user_data inside the structure. You will need to modify the callbacks being made to add the user_data as a parameter. The user_data is how the "real" callback is stored and passed in order to get called from the C code. So that MUST be visible as either a parameter or as a field in the structure. There is no way around this. |
|
another thing you also have to keep an eye out for is functions not being available to set callbacks in a structure. Not all structures have functions that will set those callbacks and those will need to be added. In a lot of those cases there has been no type declared for the callbacks outside of the structure. That will need to be done to the parameter in the function can be typed the same as the field in the structure. Remember to keep with the correct naming for this.. You would have to do this... public header file struct _some_struct_t;
typedef struct _some_struct_t some_struct_t;
typedef void (* some_struct_some_callback_cb_t)(some_struct_t *param1, void *user_data);
void set_callback(some_struct_t *param1, some_struct_some_callback_cb_t callback, void *user_data)
{
param1->some_callback_cb = callback;
param1->user_data = user_data;
}private header file #include "public_header_file.h"
struct _some_struct_t {
some_struct_some_callback_cb_t some_callback_cb;
void *user_data;
} some_struct_t; |
|
Here is an example as I am sure that this structure has possibly been made private. struct _lv_fs_drv_t;
typedef struct _lv_fs_drv_t lv_fs_drv_t;
struct _lv_fs_drv_t {
char letter;
uint32_t cache_size;
bool (*ready_cb)(lv_fs_drv_t * drv); <========== NO GOOD
void * (*open_cb)(lv_fs_drv_t * drv, const char * path, lv_fs_mode_t mode); <========== NO GOOD
lv_fs_res_t (*close_cb)(lv_fs_drv_t * drv, void * file_p); <========== NO GOOD
lv_fs_res_t (*read_cb)(lv_fs_drv_t * drv, void * file_p, void * buf, uint32_t btr, uint32_t * br); <========= NO GOOD
lv_fs_res_t (*write_cb)(lv_fs_drv_t * drv, void * file_p, const void * buf, uint32_t btw, uint32_t * bw); <==== NO GOOD
lv_fs_res_t (*seek_cb)(lv_fs_drv_t * drv, void * file_p, uint32_t pos, lv_fs_whence_t whence); <========= NO GOOD
lv_fs_res_t (*tell_cb)(lv_fs_drv_t * drv, void * file_p, uint32_t * pos_p); <========== NO GOOD
void * (*dir_open_cb)(lv_fs_drv_t * drv, const char * path); <========== NO GOOD
lv_fs_res_t (*dir_read_cb)(lv_fs_drv_t * drv, void * rddir_p, char * fn, uint32_t fn_len); <========== NO GOOD
lv_fs_res_t (*dir_close_cb)(lv_fs_drv_t * drv, void * rddir_p); <========== NO GOOD
void * user_data; /**< Custom file user data*/
};There are no functions to set any of those callbacks. The user data is also not passed to any of the callback functions either. |
|
Here are the errors that are occurring in the CI Error: ../../lib/lv_bindings/../../lib/lv_bindings/lvgl/src/libs/rlottie/lv_rlottie.h:37:16: error: field ‘img_ext’ has incomplete type
37 | lv_image_t img_ext;
| ^~~~~~~
In file included from ../../lib/lv_bindings/../../lib/lv_bindings/lvgl/lvgl.h:103,
from build-standard/lvgl/lv_mpy.c:33:
Error: ../../lib/lv_bindings/../../lib/lv_bindings/lvgl/src/libs/ffmpeg/lv_ffmpeg.h:31:16: error: field ‘img’ has incomplete type
31 | lv_image_t img;
| ^~~
build-standard/lvgl/lv_mpy.c: In function ‘mp_lv_delete_cb’:
Error: build-standard/lvgl/lv_mpy.c:231:25: error: invalid use of incomplete typedef ‘lv_event_t’
231 | LV_OBJ_T *lv_obj = e->current_target;
| ^~
Error: build-standard/lvgl/lv_mpy.c:233:35: error: invalid use of incomplete typedef ‘lv_obj_t’
233 | mp_lv_obj_t *self = lv_obj->user_data;
| ^~
build-standard/lvgl/lv_mpy.c: In function ‘lv_to_mp’:
Error: build-standard/lvgl/lv_mpy.c:243:45: error: invalid use of incomplete typedef ‘lv_obj_t’
243 | mp_lv_obj_t *self = (mp_lv_obj_t*)lv_obj->user_data;
| ^~
Error: build-standard/lvgl/lv_mpy.c:266:15: error: invalid use of incomplete typedef ‘lv_obj_t’
266 | lv_obj->user_data = self;
| ^~
In file included from ../../py/obj.h:32,
from build-standard/lvgl/lv_mpy.c:20:
build-standard/lvgl/lv_mpy.c: In function ‘mp_lv_init_gc’:
Error: build-standard/lvgl/lv_mpy.c:357:57: error: ‘lv_global_t’ undeclared (first use in this function); did you mean ‘lv_label_t’?
357 | mp_lv_roots = MP_STATE_VM(mp_lv_roots) = m_new0(lv_global_t, 1);
| ^~~~~~~~~~~
../../py/misc.h:71:29: note: in definition of macro ‘m_new0’
71 | #define m_new0(type, num) ((type *)(m_malloc0(sizeof(type) * (num))))
| ^~~~
build-standard/lvgl/lv_mpy.c:357:57: note: each undeclared identifier is reported only once for each function it appears in
357 | mp_lv_roots = MP_STATE_VM(mp_lv_roots) = m_new0(lv_global_t, 1);
| ^~~~~~~~~~~
../../py/misc.h:71:29: note: in definition of macro ‘m_new0’
71 | #define m_new0(type, num) ((type *)(m_malloc0(sizeof(type) * (num))))
| ^~~~
Error: ../../py/misc.h:71:35: error: expected expression before ‘)’ token
71 | #define m_new0(type, num) ((type *)(m_malloc0(sizeof(type) * (num))))
| ^
build-standard/lvgl/lv_mpy.c:357:50: note: in expansion of macro ‘m_new0’
357 | mp_lv_roots = MP_STATE_VM(mp_lv_roots) = m_new0(lv_global_t, 1);
| ^~~~~~
build-standard/lvgl/lv_mpy.c: At top level:
Error: build-standard/lvgl/lv_mpy.c:20815:22: error: ‘lv_qrcode_class’ undeclared here (not in a function); did you mean ‘lv_barcode_class’?
20815 | .lv_obj_class = &lv_qrcode_class,
| ^~~~~~~~~~~~~~~
| lv_barcode_class
|
| #endif | ||
|
|
||
| #include "../tick/lv_tick.h" | ||
| #include "../layouts/lv_layout.h" | ||
|
|
||
| #include "../misc/lv_types.h" | ||
|
|
||
| #include "../misc/lv_timer_private.h" |
There was a problem hiding this comment.
having all of the private headers included into a public header defeats the purpose of making private headers.
src/core/lv_group.h
Outdated
| @@ -22,7 +22,7 @@ extern "C" { | |||
| * DEFINES | |||
| *********************/ | |||
| /*Predefined keys to control the focused object via lv_group_send(group, c)*/ | |||
| enum _lv_key_t { | |||
| enum { | |||
There was a problem hiding this comment.
removing the names and making enumerations anonymous is going to break the documentation.
There was a problem hiding this comment.
I agree that it should be typedef enum{} lv_key_t;
There was a problem hiding this comment.
I agree that it should be typedef enum{} lv_key_t;
| lv_ll_t obj_ll; /**< Linked list to store the objects in the group*/ | ||
| lv_obj_t ** obj_focus; /**< The object in focus*/ | ||
|
|
||
| lv_group_focus_cb_t focus_cb; /**< A function to call when a new object is focused (optional)*/ |
There was a problem hiding this comment.
you need to make sure that these function pointer types have a user_data parameter to them and that the user_data in this structure is getting passed to the callback. You also need to make sure that there are functions to set the callbacks because if there isn't there is not going to be a way to do it if they are private.
There was a problem hiding this comment.
There is a setter for this, but this is unrelated to the changes in this PR.
There was a problem hiding this comment.
I was just quickly skimming over and noting things that should be checked to make sure everything is actually complete.
| @@ -43,7 +43,7 @@ extern "C" { | |||
| * Possible states of a widget. | |||
| * OR-ed values are possible | |||
| */ | |||
| enum _lv_state_t { | |||
| enum { | |||
There was a problem hiding this comment.
gonna mess up the documentation by removing the name
| @@ -68,7 +68,7 @@ enum _lv_state_t { | |||
| * Not all parts are used by every widget | |||
| */ | |||
|
|
|||
| enum _lv_part_t { | |||
| enum { | |||
There was a problem hiding this comment.
gonna mess up the documentation by removing the name
| #include "lv_draw_vector.h" | ||
|
|
||
| #if LV_USE_VECTOR_GRAPHIC | ||
|
|
There was a problem hiding this comment.
All of the structures in this file need to have functions to set each and every single field otherwise they will not be usable to a binding.
| @@ -29,7 +29,7 @@ LV_EXPORT_CONST_INT(LV_IMAGE_HEADER_MAGIC); | |||
| * TYPEDEFS | |||
| **********************/ | |||
|
|
|||
| typedef enum _lv_image_flags_t { | |||
| typedef enum lv_image_flags_t { | |||
There was a problem hiding this comment.
don't need the enum name here, only the typedef name
|
It might be better to tackle this animal one header file at a time. do one header and submit a PR for it and make sure that everything is good to go with it then go to the next header. I know that's going to be a real huge ball buster to do but it is the only way I think that it will get done properly.. If you would like we can make an addon script that will monkey patch the gen_json script to collect the header file names that the different pieces of LVGL go into and we can have it sort the output based on the filenames. Sorting it like that we would be able to quickly see what is happening for each file and we will be able to see what is happening with things like the structures and if proper functions are in place in order to move structures into a private header file. it would defiantly make it easier to see the things grouped together like that to be able to determine the work that needs to be done. we can also parse the JSON output and check the structures and if a structure is not currently being used as a type for a parameter in a function then we can flag it for being private. |
What about
The goal of this update to avoid it. We need to include
If there is no API to set the fields the struct must remain public. |
|
I thought you were the maintainer of the micropython binding for some reason haha. Ah so the JSON generator is supposed to be a step in the right direction for the future of docs generation and helping the micropython binding generation and future bindings.
It's not actually as complex as I thought, but there's a lot of coupling and it will be a big deal to update as @kdschlosser said, now that I'm looking through it. When it goes to get struct information to generate something, it also emits the binding for said struct. @amirgon, can you grasp what we're trying to do and offer a solution? Temporary solutions are welcome. No pressure. Thanks in advance. Otherwise I'll keep chipping away and see if I can manage it. |
|
We need some feedback on this pull request. Now we mark this as "stale" because there was no activity here for 14 days. Remove the "stale" label or comment else this will be closed in 7 days. |
|
I did want to make sure that you name all public enums. This is NOT naming the enum typedef enum {
...
} some_enum;That is creating a typedef to an anonymous enum. This is a named enum typedef enum _some_enum {
...
} some_enum;This is the one use case where having the name prefixed with an underscore should be allowed. naming both the enum and the typedef the same makes it tricky to deal with when using pycparser. It is better to have different names. You can even forward declare it if needed. Best practices with enums is to do something along these lines. typedef enum _lv_enum_example {
LV_ENUM_EXAMPLE_ITEM1,
LV_ENUM_EXAMPLE_ITEM2
LV_ENUM_EXAMPLE_ITEM3
} lv_enum_example;The\ names for the enum items should be prefixed with the typedef or enum name. For the most part this is done, But there are some deviations from it that should be corrected. |
|
As these are not introduced in this PR, let's handle these in a follow up PR. I think now we should focus on fixing the MP build issue to add more cleanup in smaller incremental steps. |
|
OK so I want to bring this up here since you are doing work that is going to end up clashing with some of the work I am doing. I am in the process of fixing the errors in the documentation build and one of the things I am running into is how things are done with lv_os and with draw... In order to compile the documentation all of the header files need to be processed to their fullest. so with the draw header files there is the use of macros that are defined that make function calls but when you enable everything so all of the documentation gets built we end up with name collisions because the same macro is defined in more than one header file. The same thing holds true with functions as well. macros shouldn't be used for this kind of a thing. there should only be a single function declaration and the functions should be defined in the source files and not in header files. the document build system only reads the header files and not the source files. so I should be able to "turn everything on" and not end up with naming collisions because the macros should control what gets compiled in the source files and not in the header files. the draw and os stuff needs to be rewritten so it all has a common API. structures can be extended by doing something like the following. in lv_os_none.h you have this code.. typedef int lv_mutex_t;
typedef int lv_thread_t;
typedef int lv_thread_sync_t;and in lv_os_pthread.h you have this code. typedef struct {
pthread_t thread;
void (*callback)(void *);
void * user_data;
} lv_thread_t;
typedef pthread_mutex_t lv_mutex_t;
typedef struct {
pthread_mutex_t mutex;
pthread_cond_t cond;
bool v;
} lv_thread_sync_t;This causes a naming collision when building the docs. If we do the following.. lv_os.h typedef struct _lv_mutex_t {
uint8_t id; // doesn't matter what we do here just need something to declare a non empty structure.
} lv_mutex_t;
typedef struct _lv_thread_t {
void (*callback)(void *);
void * user_data;
} lv_thread_t;
typedef struct _lv_thread_sync_t {
uint8_t id;
} lv_thread_sync_t;and in lv_os_pthread.c we extend the structure like so. typedef struct {
uint8_t id;
pthread_mutex_t mutex;
} lv_mutex_pthread_t;
typedef struct {
void (*callback)(void *);
void * user_data;
pthread_t thread;
} lv_thread_pthread_t;
typedef struct {
uint8_t id;
pthread_mutex_t mutex;
pthread_cond_t cond;
bool v;
} lv_thread_sync_pthread_t;functions would be de3clared in lv_os.h like so. lv_result_t lv_thread_init(lv_thread_t * thread, lv_thread_prio_t prio, void (*callback)(void *), size_t stack_size, void * user_data);
lv_result_t lv_thread_delete(lv_thread_t * thread);and this is what you would do in that function definition in lv_os_pthread.c lv_result_t lv_thread_init(lv_thread_t * thread, lv_thread_prio_t prio, void (*callback)(void *), size_t stack_size, void * user_data)
{
LV_UNUSED(prio);
lv_thread_pthread_t *ret = (lv_thread_pthread_t *)calloc(1, sizeof(lv_thread_pthread_t))
pthread_attr_t attr;
pthread_attr_init(&attr);
pthread_attr_setstacksize(&attr, stack_size);
ret->callback = callback;
ret->user_data = user_data;
pthread_create(&ret->thread, &attr, generic_callback, ret);
*thread = (lv_thread_t *)ret;
return LV_RESULT_OK;
}
lv_result_t lv_thread_delete(lv_thread_t *thread)
{
lv_thread_pthread_t *thrd = (lv_thread_pthread_t *)thread;
if (thrd->thread != NULL) {
int ret = pthread_join(thrd->thread, NULL);
if(ret != 0) {
LV_LOG_WARN("Error: %d", ret);
return LV_RESULT_INVALID;
}
thrd->thread = NULL;
return LV_RESULT_OK;
} else {
return LV_RESULT_INVALID;
}
}so now everything has a common type that is able to be used so there are no name collisions. this void (*callback)(void *)would need to be typed properly and not declared in the structure and the function declarations. But it gives you the basic idea. |
|
I see the problem. Let keep in mind for the next pass. For now, please enable only a single OS, and deal with this problem separately. Otherwise I'm afraid we will never never merge anything. 😄 |
|
I am mentioning it so I don't forget about it. The problem isn't so much enabling every OS because I am not able to do that. The problem stems from how portions of the document build system work and those parts being outside my control. Breathe which is the layer that translates doxygen to sphinx uses doxygen to build XML files and it does this one header file at a time.. There is no preprocessing that gets done when it does this so it takes everything that is seen in the header file exactly how it is. The only way I am able to control is it to not include specific header files from having documentation being generated for it. If the API was written to be common across all of the OS's then there would only be a need to read a single header file and that would work out fine....But because the types of some of the function parameters are not declared in the header file where the functions are declared a whole different set of errors occur because of the types not existing. |
|
I also wanted to mention this so that the work that is being done here is done so it doesn't cause more of this kind of an issue in other portions of the LVGL code. I hate to see work get done and then have to get done again because it caused an issue like the one I mentioned. |
Description of the feature or fix
For #6011
Notes
lv_conf_template.hrun lv_conf_internal_gen.py and update Kconfig.scripts/code-format.py(astyle version v3.4.12 needs to be installed) and follow the Code Conventions.