lvgl / lv_binding_micropython

LVGL binding for MicroPython
MIT License
250 stars 161 forks source link

Fix fs_driver casting issue #128

Closed PGNetHun closed 3 years ago

PGNetHun commented 3 years ago

Fix casting issue in lib/fs_driver.py

For couple of weeks Dynamic_loading_font_example.py example crashes with error message: KeyError: 'type' object isn't subscriptable

Example works now again.

PS: lv.font_load() should be called in global scope (not in functions / local scope), otherwise app crashes with segmentation failure (unix port)

amirgon commented 3 years ago

Hi @PGNetHun ! Thank you for your contribution.

A slightly different version was suggested by @uraich , where the fs driver is a class and the file object is kept as a member. The advantage is that the casts are not needed.

See: https://github.com/uraich/lv_mpy_examples/blob/main/demo_printer/filesys_driver.py

(But @uraich version also needs a little cleanup)

PGNetHun commented 3 years ago

Hi @amirgon, I have also seen @uraich's version, that the file object is stored in class member/variable (self.fd). The only issue with it is that the class is singleton in the example (and apps), and when more files are opened parallel (by LVGL), then the newly opened files will overwrite that class member (self.fd). In actual example the font files are opened, read, then immediately closed, one by one, so simultaneous file opening issue does not occur. (but I think storing the "file object" in file_p->file_d (lv_fs.c) - as it is designed in LVGL - would be better way)

But please correct me if I am wrong, that's what I've found during code investigation.

uraich commented 3 years ago

Actually I agree with you. I also find it more elegant to save the file descriptor in file_p->file_d (lv_fs.c) Probably I would have done the same thing had I be able to figure out, how to correctly cast. I have already come back to your new version of fs_driver.py on my printer_demo examples.

amirgon commented 3 years ago

The only issue with it is that the class is singleton in the example (and apps), and when more files are opened parallel (by LVGL), then the newly opened files will overwrite that class member (self.fd). In actual example the font files are opened, read, then immediately closed, one by one, so simultaneous file opening issue does not occur. (but I think storing the "file object" in file_p->file_d (lv_fs.c) - as it is designed in LVGL - would be better way)

@PGNetHun You are correct, your change make sense. I tried rebasing and adding some changes to your branch at PGNetHun:master and somehow GitHub closed the PR, sorry about that. Maybe because I tried to force push it, I'm not sure.

Anyway, I'll integrate your change. I've modified it to use lv.C_Pointer on fs_open_cb instead of fs_file_t as this is a generic void* and is not related to fs_file_t.

amirgon commented 3 years ago

Merged on 4c6804a03acae90a93b45dd486116164cb5659d3

Thank you.

PGNetHun commented 3 years ago

Thank you, I will also test the final fix.