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

functions not returning proper type #265

Open
kdschlosser opened this issue Mar 20, 2023 · 7 comments
Open

functions not returning proper type #265

kdschlosser opened this issue Mar 20, 2023 · 7 comments

Comments

@kdschlosser
Copy link
Contributor

https://sim.lvgl.io/v9.0/micropython/ports/javascript/index.html?script_startup=https://raw.githubusercontent.com/lvgl/lvgl/e26a46c43c23e91198318659c8214cc34be5cee2/examples/header.py&script=https://raw.githubusercontent.com/lvgl/lvgl/e26a46c43c23e91198318659c8214cc34be5cee2/examples/get_started/lv_example_get_started_3.py&script_direct=a20481aa27d8b699a38d15fccb3dea978c3d5131

lv.color_t methods not returning the types they are supposed to be.

static inline lv_color1_t lv_color_to1(lv_color_t color)                        ** exists but the return type is not lv_color1_t **
static inline lv_color8_t lv_color_to8(lv_color_t color)                        ** exists but the return type is not lv_color8_t **
static inline lv_color16_t lv_color_to16(lv_color_t color)                    ** exists but the return type is not lv_color16_t **
static inline lv_color32_t lv_color_to32(lv_color_t color)                    ** exists but the return type is not lv_color32_t **

There are also missing functions

static inline void lv_color_set_int(lv_color_t * c, uint32_t v)               **missing**
static inline void lv_color8_set_int(lv_color8_t * c, uint8_t v)             **missing**
static inline void lv_color16_set_int(lv_color16_t * c, uint16_t v)       **missing**
static inline void lv_color32_set_int(lv_color32_t * c, uint32_t v)       **missing**

static inline lv_color_t lv_color_from_buf(const uint8_t * buf)           **missing**
static inline lv_color8_t lv_color8_from_buf(const uint8_t * buf)       **missing**
static inline lv_color16_t lv_color16_from_buf(const uint8_t * buf)   **missing**
static inline lv_color32_t lv_color32_from_buf(const uint8_t * buf)   **missing**

static inline uint32_t lv_color_to_int(lv_color_t c)                            **missing**
static inline uint8_t lv_color8_to_int(lv_color8_t c)                          **missing**
static inline uint16_t lv_color16_to_int(lv_color16_t c)                    **missing**
static inline uint32_t lv_color32_to_int(lv_color32_t c)                    **missing**

This function should have the first 2 parameters flip flopped

LV_ATTRIBUTE_FAST_MEM static inline lv_color_t lv_color_mix_premult(uint16_t * premult_c1, lv_color_t c2, uint8_t mix) **exists but should be changed so the c2 parameter is first**

a function that should be added is

lv_color_t  lv_hsv_to_color(lv_color_hsv_t hsv);

There is also a naming convention oddity going on.

take the function lv_obj_set_style_bg_color as an example. This function when exposed to MicroPython gets added to lv_obj_t as a method and that method name becomes set_style_bg_color. The complete qualified name is obj.set_style_bg_color.

take the function lv_color_to32 as an example. This function gets put into the lv_color_t structure. Strange thing is what the functions name ends up being.. color_to32. This does not follow the API seen everywhere else. There are a bunch of functions that get mapped to lv_color_t that are like this.

Here are the names of the functions that are in lv_color_t

color_to_hsv
color_mix_with_alpha
color_change_lightness
color_lighten
color_darken
color_brightness
color_mix
color_premult
color_to16
color_to32
color_to8
color_to1
color_fill

Here are the names of functions and structures as they are in the MicroPython binding. Note that color1_t, color8_t, and color16_t are all missing.

color_t
color_hsv_t
color_hex3
color_mix_premult
color_chroma_key
color_hsv_to_rgb
color_filter_dsc_t
color_rgb_to_hsv
color_hex
color_make
color32_ch_t
color32_t
color_white
color_black

You had stated that the structures that do not get added to the binding are the ones that are not used in functions.

but here are the functions... These are also missing as well

static inline uint32_t lv_color_to_int(lv_color_t c)
static inline uint8_t lv_color8_to_int(lv_color8_t c)
static inline uint16_t lv_color16_to_int(lv_color16_t c)
static inline uint32_t lv_color32_to_int(lv_color32_t c)

The reason why I bring this to your attention is because when using the binding on an ESP32 this issue occurs.

color = 0xDE65D5
c1 = lv.color_hex(color}
c2 = c1.color_to32()

print(color == c2)

The result is False. Originally I had thought it was because I goofed because the code stated the return type should be lv_color32_t. But in this case something is not working right because the return type from that method is an int. so the question now is why does the equality check fail? it shouldn't.. I don't know if this is an issue in the binding or in lvgl itself. I suspect it might be the binding because the return type is wrong.

@amirgon
Copy link
Collaborator

amirgon commented Apr 15, 2023

Hi @kdschlosser
I think that most of the problems you are pointing at are not present any more on the latest version of the simulator.

See:
https://sim.lvgl.io/v9.0/micropython/ports/javascript/index.html?script_startup=https://raw.githubusercontent.com/lvgl/lvgl/e26a46c43c23e91198318659c8214cc34be5cee2/examples/header.py&script=https://raw.githubusercontent.com/lvgl/lvgl/e26a46c43c23e91198318659c8214cc34be5cee2/examples/get_started/lv_example_get_started_3.py&script_direct=b12c1c9ce2e8e0355a6d518b389a5ebf32563ce3

lv.color_t methods not returning the types they are supposed to be.

Cannot reproduce on latest version.

Your (fixed) script prints:

<class 'lv_color32_t'> <--- correct, lv_color_t maps to lv_color32_t in the simulator
<class 'lv_color32_t'> <--- wrong, should be lv.color32_t
<class 'lv_color16_t'> <--- wrong, should be lv.color16_t
<class 'lv_color8_t'> <--- wrong, should be lv.color8_t
<class 'lv_color_hsv_t'> <--- correct, returns lv_color_hsv_t

There are also missing functions

I think these are not missing on latest version.
Note that "from_buf" functions are not member functions since their first argument is not the color class.

There is also a naming convention oddity going on.

You are right about that.
color_t is aliased by color32 and it confuses the script because the C function names do not start with"color32" but "color".
In such case the script does not remove the prefix from the member names which results in odd names such as color_to_hsv etc.

We can probably fix that, I'm not sure when I'll have time to dig into this, feel free to open a PR if you want.

Note that color1_t, color8_t, and color16_t are all missing.

color8_t, and color16_t are present on latest version.
lv_color1_t is indeed missing, but this is expected as it is not used anywhere (as a function argument or return value)

The reason why I bring this to your attention is because when using the binding on an ESP32 this issue occurs.

The result of color_to32() is lv_color32_t. Since this is a struct, its pointer value is compared not its contents.
This will return True: c1.to_int() == c2.to_int()

@kdschlosser
Copy link
Contributor Author

it is not returning the correct value

https://sim.lvgl.io/v9.0/micropython/ports/javascript/index.html?script_startup=https://raw.githubusercontent.com/lvgl/lvgl/53dcb8cd998a6f98701a5702f1c9cc62246b313a/examples/header.py&script=https://raw.githubusercontent.com/lvgl/lvgl/53dcb8cd998a6f98701a5702f1c9cc62246b313a/examples/event/lv_example_event_4.py&script_direct=29e57f2ee17b5df146c482a17b3791f74527ffe7

the integer being output is 0xFF7B63FB which is not the same as the input color of 0x7B63FB. I am not able to supply 0xFF7B63FB to the color_hex function so I am not sure why the FF is being added.

Using a Freshly compiled master branch on my esp32 and run the same code it doesn't match either the value returned by to_int is 0x7B1F which is not the same as 0x7B63FB

@amirgon
Copy link
Collaborator

amirgon commented Apr 18, 2023

the integer being output is 0xFF7B63FB which is not the same as the input color of 0x7B63FB. I am not able to supply 0xFF7B63FB to the color_hex function so I am not sure why the FF is being added.

After calling in C lv_color_hex(0x7b63fb) the returned lv_color_t struct is:

{blue = 0xfb, green = 0x63, red = 0x7b, alpha = 0xff}

Looking at lv_color_hex in lv_color.h:

#elif LV_COLOR_DEPTH == 32
    lv_color_t r;
    lv_color_set_int(&r, c | 0xFF000000);
    return r;

So looks like it's by design to set alpha to 0xFF.
Perhaps @kisvegabor could explain the motivation.

@kdschlosser
Copy link
Contributor Author

I could understand it being added if I was able to set the alpha using color_hex but I am not able to. I would have to directly set the alpha using the structure.

@kisvegabor
Copy link
Member

So looks like it's by design to set alpha to 0xFF.
Perhaps @kisvegabor could explain the motivation.

As 32 bit colors can be treated ARGB8888 too we need to the set the alpha to 0xff. Else the color could be rendered as transparent.

@kdschlosser
Copy link
Contributor Author

But there is no way to set the alpha channel using lv_color_hex which is the best way to set a color without needing to know the color depth the display has been set to..

There are not all that many displays that support an alpha channel so what exactly is the alpha channel used for in lv_color32_t?? It's not used by the widgets because they have set_style_bg_opa to set the alpha channel. (This next question is rhetorical and is only intended to get your thinking) If a display doesn't support alpha channels then how am I able to set the opacity of a widget background? The answer is it does in using layers and via software....

I mean if you think about it how much additional memory would end up getting consumed if you used lv_color32_t as the public color entry point? I would imagine that when I set the color of a widget a buffer of that widget is not what gets stored but instead the single color gets stored and if something changes on the widget then the color gets written to the frame buffer which at that time is when a conversion from the 32 bit color takes place. This also removes the need to set the color at compile time. It can be set at runtime instead. all of the "*_opa" functions can be removed because they would no longer be necessary. lv_color_hex can also get removed. all colors would be stored as ARGB8888 so when making a color to pass to say lv_obj_set_style_bg_color all that would have to be done is passing a pointer to a created color.

I am not a c guy so excuse this probably being incorrect. You will get the general idea tho.

lv_color_t * color = (lv_color_t *)0xFF559933;
lv_set_style_bg_color(obj, color, 0);

// now if I want to change only the opacity of the background

color->alpha = 50;
lv_obj_invalidate(obj);

and internally before the buffer gets written to a color conversion would take place to whatever has been set for a specific display and the converted color is what gets written to the buffer.

It fixes problems with color comparisons because lv_obj_get_style_bg_color would return the same pointer that it was passed in the get or the pointer to the default color. It sucks there is no overloading in C99 as it would be nice to be able to use (r, g, b), (r, g, b, a), RGB, ARGB or lv_color_t to the color functions.

@kisvegabor
Copy link
Member

The exact same topic (use ARGB8888 in API) came up in lvgl/lvgl#4059 (comment) too. Please read the last few comments where I described why it's not as good idea as it seemed at first look.

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

No branches or pull requests

3 participants