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

Fixes for MP support for LVGL 3rd party libraries #2666

Merged
merged 6 commits into from Oct 20, 2021

Conversation

amirgon
Copy link
Contributor

@amirgon amirgon commented Oct 14, 2021

Add missing lv_qrcode_class

Remove 'struct JDEC' from public API. This struct is needed intenally on tjpgd.c and lv_sjpg.c, but doesn't need to be exposed in the public API. When exposed, it increases Micropython binding program size and some fields are not supported today (uint8_t* huffbits[2][2]). To overcome this, moved it to a new H file which is not included in public API, only in sjpg C files

Related: lvgl/lv_binding_micropython#180

Add missing lv_qrcode_class

Remove 'struct JDEC' from public API. This struct is needed intenally on tjpgd.c and lv_sjpg.c, but doesn't need to be exposed in the public API. When exposed, it increases Micropython binding program size and some fields are not supported today (uint8_t* huffbits[2][2]). To overcome this, moved it to a new H file which is not included in public API, only in sjpg C files

Related: lvgl/lv_binding_micropython#180
Copy link
Member

@embeddedt embeddedt left a comment

Choose a reason for hiding this comment

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

Would it make more sense to just add an #ifdef wrapper around struct JDEC and define a macro in tjgpd.c? It would mean a much smaller diff and significantly reduces the changes to the original file.

@amirgon
Copy link
Contributor Author

amirgon commented Oct 15, 2021

Would it make more sense to just add an #ifdef wrapper around struct JDEC and define a macro in tjgpd.c?

Would that be more readable? To define macros before including H files to change their behavior?
Even without Micropython, why create a larger public API for the C user?
In such cases I perfer opaque structs
I think that moving the struct to a new private H file is a simpler and more straightforward solution, but your suggestion could work as well.

Must define a distinct class for every widget, to allow Micropython bindings convert lv_obj_t into the specific class
@kisvegabor
Copy link
Member

kisvegabor commented Oct 15, 2021

As a 3rd possible solution: on previous issues I suggested something like

#ifndef INCLUDE_FROM_LVGL_H

struct ...

#endif

@amirgon
Copy link
Contributor Author

amirgon commented Oct 15, 2021

As a 3rd possible solution: on previous issues I suggested something like

#ifndef INCLUDE_FROM_LVGL_H

Could you remind me where this macro would be defined? It can't be defined in lvgl.h, because all files eventually include it.

@kisvegabor
Copy link
Member

Not all files, but certainly more than I anticipated. In the core features in included only the required headers but the extra widgets/libs and fonts really include lvgl.h.

What about creating an lvgl_internal.h and include it in widgets.

For example (using a different name for the define):

//lvgl.h

#ifndef LV_USE_PRIVATE_API
#define LV_USE_PRIVATE_API 0 //If included by the user do not use the private API
#endif 

#include "core/lv_obj.h"
#include "core/lv_style.h"
...
//lvgl_internal.h

#define LV_USE_PRIVATE_API 1 //Enable the private API

#include "lvgl.h"
//tjpgd.h

#include "../../../lvgl_internal.h"

#if LV_USE_PRIVATE_API
/* Decompressor object structure */
typedef struct JDEC JDEC;
struct JDEC {
	size_t dctr;				/* Number of bytes available in the input buffer */
	uint8_t* dptr;				/* Current data read ptr */
...
};
#endif

This way we could solve the private/public API easily in a readable way.

@amirgon
Copy link
Contributor Author

amirgon commented Oct 15, 2021

What about creating an lvgl_internal.h and include it in widgets.

I'm not sure I understand the benefit of adding another mechanism and another LVGL specific macro to solve a general, well-understood problem.

The common practice is to hide implementation details from the user, and in C this is often achieved with Opaque pointers.
The benefit for the the C user (regardless of Micropython) is a smaller public C API and the ability to change implementation details without affecting the interface.

Your macro related suggestions keep the implementation details in the public header and only enable/disable them depending on where the header is included from. In my opinion this is a non-standard and confusing approach.

Imagine a C user looking at your header. Would he notice the #if LV_USE_PRIVATE_API line? Would he immediately know its meaning and its scope?
On the other hand, if a C user sees an opaque struct with no implementation (typedef struct JDEC JDEC; for example) he would immediately know that this an opaque struct with private implementation. No need to notice some #if lines, no need to be familiar with library specific conventions etc. because opaque struct is a well known and widely used technique in C.

So could you explain what is the benefit in inventing a new way to declare "private" structs and not using well known practices?

ESP32 reports some potentially uninitialized variables. Initialize them to prevent the errors
@kisvegabor
Copy link
Member

kisvegabor commented Oct 18, 2021

The problem with Opaque pointers is that they can be used only as pointer. So if a C file declares a struct, and other C files can instantiate that struct using opaque pointers.


Anyway, I suggest not blocking this PR with the discussion of this apparently complicated topic.

@amirgon
Copy link
Contributor Author

amirgon commented Oct 18, 2021

The problem with Opaque pointers is that they can be used only as pointer.

True, we cannot use Opaque Pointers if the user needs to explicitly create an instance of a struct allocated on stack or data segment.
But in many cases the user doesn't need to. The user can still call a function that returns an Opaque Pointer, pass this pointer around etc. which is good enough for many use cases.


Specifically in this case, I have another suggestion.
Usually users would only use SJPG through the image decoder, so really the only public function can be

void lv_split_jpeg_init(void);

Everything else can be in private headers. The user could still include these non-public headers explicitly to gain access to SJPG internals.

In such case we can simply remove the include to tjpgd.h from lv_sjpg.h public API, and move other structs (io_source_t, SJPEG) to private headers. No need for any opaque pointers.

What do you think?

@kisvegabor
Copy link
Member

But in many cases the user doesn't need to. The user can still call a function that returns an Opaque Pointer, pass this pointer around etc. which is good enough for many use cases.

True. I believe most of these cases are already changed like this in LVGL, but there were where it didn't work. I don't remember concrete examples now, but do remember that when I tried I hit into many walls.

In such case we can simply remove the include to tjpgd.h from lv_sjpg.h public API, and move other structs (io_source_t, SJPEG) to private headers. No need for any opaque pointer

Good idea! void lv_split_jpeg_init(void); doesn't depend on anything, so no special includes are required in the header.

@amirgon
Copy link
Contributor Author

amirgon commented Oct 19, 2021

Good idea! void lv_split_jpeg_init(void); doesn't depend on anything, so no special includes are required in the header.

Done.

@kisvegabor
Copy link
Member

Great, thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants