lvgl / lv_binding_micropython

LVGL binding for MicroPython
MIT License
237 stars 156 forks source link

st77xx.py is broken #263

Closed amirgon closed 1 year ago

amirgon commented 1 year ago

After adapting the Micropython Bindings to the new LVGL driver API (https://github.com/lvgl/lvgl/issues/4011), st77xx.py broke.

Related: https://forum.lvgl.io/t/issue-st77xx-py-generic-driver/11216

The problem is as following:

https://github.com/lvgl/lv_binding_micropython/blob/cf206e14253b2823522f139f3fa60b4219bd654f/driver/generic/st77xx.py#L438-L443

st77xx.py accesses disp_drv.draw_buf.buf_act directly. This is no longer possible because draw_buf_act is now a private LVGL member that is not exposed to the users. It is not part of the LVGL public API and therefore not part of the Micropython bindings.

The user is supposed to access the data through color argument which represents the buffer.

@eudoxos - What was the reason you used buf_act directly instead of color?

ngc6589 commented 1 year ago

Fix color.dereference in st77xx.py

self.blit(area.x1,area.y1,w:=(area.x2-area.x1+1),h:=(area.y2-area.y1+1),color.__dereference__(lv.color_t. __SIZE__*w*h),is_blocking=False)

lv.color_t.SIZE cannot be resolved.

Described at def init(....) import lvgl as lv import lv_utils Better yet, move the import 2 line to the top of the file.

thank you.

eudoxos commented 1 year ago

@amirgon IIRC buf_act is the current output buffer and __dereference__ gets memoryview starting at its address with the size of 2*w*h (2 is bytes per pixel, of course). I dont't see how can this be replaced by dereferencing color?

amirgon commented 1 year ago

lv.color_t.SIZE cannot be resolved.

@ngc6589 I changed it back to 2 instead of lv.color_t. __SIZE__.
I think the original idea was to import and use LVGL only in St77xx_lvgl constructor, which makes the driver easily portable to other libraries and not LVGL specific, so I kept the imports in the constructor.

Please let me know if it works now, as I cannot test this on my side.

I dont't see how can this be replaced by dereferencing color?

@eudoxos Looking at the C callback:

/**
 * Set the flush callback whcih will be called to copy the rendered image to the display.
 * @param disp      pointer to a display
 * @param flush_cb  the flush callback
 */
void lv_disp_set_flush_cb(lv_disp_t * disp, void (*flush_cb)(struct _lv_disp_t * disp, const lv_area_t * area,
                                                             lv_color_t * color_p));

color_p is a pointer to the display buffer, which is an array with element type of lv_color_t.
I agree that this is not well documented, and the name "color" here is a poor choice... (cc: @kisvegabor) draw_buf_act is internal to LVGL and not part of the API. The driver is expected to access the color_p array instead.

Here is an example from Linux FB driver:

static void flush_cb(lv_disp_t * disp, const lv_area_t * area, lv_color_t * color_p)
{
...
lv_memcpy(&fbp32[location], (uint32_t *)color_p, (area->x2 - area->x1 + 1) * 4);
ngc6589 commented 1 year ago

@ngc6589 I changed it back to 2 instead of lv.color_t. SIZE. I think the original idea was to import and use LVGL only in St77xx_lvgl constructor, which makes the driver easily portable to other libraries and not LVGL specific, so I kept the imports in the constructor.

Please let me know if it works now, as I cannot test this on my side.

I've seen a fix that changed lv.color_t.SIZE back to 2. As a result of confirmation, it worked without error.

kisvegabor commented 1 year ago

I agree that this is not well documented, and the name "color" here is a poor choice... (cc: @kisvegabor) draw_buf_act is internal to LVGL and not part of the API. The driver is expected to access the color_p array instead.

What name would you find more intuitive? E.g. rendered_img?

amirgon commented 1 year ago

What name would you find more intuitive? E.g. rendered_img?

First, I think that documentation does not say anything about the callback parameters and their meanings. Instead of color, even buffer is more intuitive for me, to stand for the buffer where the rendered image data is.

eudoxos commented 1 year ago

Or perhaps pixmap, which conveys that it is a place for pixels?

kisvegabor commented 1 year ago

px_map? We use the px abbreviation.

amirgon commented 1 year ago

px_map? We use the px abbreviation.

Sounds good to me. But please also add comments/docs that explain the callback arguments, so everything would be clear.

kisvegabor commented 1 year ago

I updated it.