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

Changes adopted by the PR 2401 #2979

Merged
merged 11 commits into from
Jan 24, 2022
Merged

Changes adopted by the PR 2401 #2979

merged 11 commits into from
Jan 24, 2022

Conversation

eismeraldo
Copy link
Contributor

feat(fs) add caching

This PR is based on the [PR 2401] (#2401) from PGNetHun. At the moment this PR is not functional. The further procedure must be discussed with those responsible for LVGL.

Checkpoints

  • Follow the styling guide
  • Run code-format.py from the scripts folder. astyle needs to be installed.
  • Update the documentation

@kisvegabor
Copy link
Member

Hi,

Thanks for opening this PR. I've made some fixes and clean up and now it works well for me.

Note the changes in the configuration of FS drivers.

@kisvegabor
Copy link
Member

kisvegabor commented Jan 11, 2022

@brgl The changes in the PR affect yours about Kconfig. #2955
Finally I took your approach here about using LV_USE_FS_... explicitly.
Please review it and confirm if it works this way for Kconfig too.

@brgl
Copy link
Contributor

brgl commented Jan 11, 2022

@brgl The changes in the PR affect yours about Kconfig. #2955 Finally I took your approach here about using LV_USE_FS_... explicitly. Please review it and confirm if it works this way for Kconfig too.

@kisvegabor Thanks for the heads-up. I'm focusing on the zephyr fork currently, once the lvgl v8 support is upstream, I will try to get it up to date and send any downstream changes your way.

@kisvegabor
Copy link
Member

Great, thanks!

@kisvegabor
Copy link
Member

@GorgonMeducer
Does these changes affect cmsis-pack?

@GorgonMeducer
Copy link
Contributor

@kisvegabor YES, I need to re-run the gen_pach.sh and that's all.

@kisvegabor
Copy link
Member

Great, I can do that too 🙂 Anyway I'll ping you before we merge it to confirm if it look good.

@eismeraldo did you have a chance to try it out?

@eismeraldo
Copy link
Contributor Author

@kisvegabor I could test if the fixed PR works. Under which URL can I download the fixed source?

@eismeraldo
Copy link
Contributor Author

I noticed that I was up to date. Everything compiles without errors. The behavior has not changed, 200 bytes are still read instead of 512.
I found out what the reason is: It uses twice the width of the 100x100 BMP, i.e. 200 bytes. The block should actually be independent of the size of the BMP, but this is probably not that easy to solve.

@kisvegabor
Copy link
Member

kisvegabor commented Jan 12, 2022

I could test if the fixed PR works. Under which URL can I download the fixed source?

You can git pull your master branch. git log -3 should show the last commits are from me.

How does the config look like in your lv_conf.h now?

@eismeraldo
Copy link
Contributor Author

I have activated the various options in the File System section, but only the two options LV_USE_FS_STDIO and LV_USE_FS_POSIX compile without errors. But as already mentioned, they do not change the behavior.
lv_conf.txt

@eismeraldo
Copy link
Contributor Author

I took some pictures of the stack trace.
In Image1.png you can see that the width of the image is being read.
In Image2.png you can see that the color depth 16 divided by 8 bits results in 2 bytes multiplied by the image width of 100 bytes results in 200 bytes.
In Image3.png you can see that btr is now exactly 200 and that the cache_size is not set.
And in Image4.png the user read function is now called with the btr of 200.
Image 1
Image 2
Image 3
Image 4

I also attached the log that documents the open, close, read, seek statements
platformio-device-monitor-220113-095525.log

@kisvegabor
Copy link
Member

In Image3.png you can see that btr is now exactly 200 and that the cache_size is not set.

Why isn't it set? The cache size should be set here. Could you debug what happens with the set value?

@eismeraldo
Copy link
Contributor Author

It's maddening, yesterday I tried to debug the ESP32 all day, the craziest error messages were displayed. Today I tried again and after a few hours the PC died. I'm typing here from my wife's PC, which is just good enough to run LibreOffice and accounting. I will order a new one, but that will take a few days.

@eismeraldo
Copy link
Contributor Author

I was allowed to test the PR on my friend's PC. I have now discovered the following:

  1. The fs_drv.cache_size is set correctly in lv_fs_stdio.c.
  2. However, in the lv_fs_read function in lv_fs.c, file_p→drv→cache_size is set to 0.

I found this out with logging. I tried to print out the addresses of the two objects and they are different, maybe I'm doing something wrong. The logs are:
lv_fs_stdio.c line 69
LV_LOG_USER("==== cache_size: %i, address %p", fs_drv.cache_size, &fs_drv.cache_size);
and in
lv_fs.c line 211
LV_LOG_USER("----- cache_size: %i, address %p", file_p->drv->cache_size, &file_p->drv->cache_size);

@kisvegabor
Copy link
Member

Hi,

I made some clean up and checked it again but it still works well for me.

I've searched for cache in lvgl to were we assign a value to it, but its only the initialization of the driver.
Can you track where cache_size loses its value?

@kisvegabor
Copy link
Member

We are planning to release v8.2 soon and I'd like to make it part of the release.

It seems working for me, but if you find the root your issue, please comment here.

@kisvegabor kisvegabor merged commit 3e0b8c9 into lvgl:master Jan 24, 2022
kisvegabor added a commit that referenced this pull request Jan 24, 2022
BREAKING CHANGE:
The `LV_FS_...` related configs needs to be updated.

Co-authored-by: Gabor Kiss-Vamosi <kisvegabor@gmail.com>
@@ -62,7 +66,9 @@ void lv_fs_fatfs_init(void)
lv_fs_drv_init(&fs_drv);

/*Set up fields...*/
fs_drv.letter = LV_USE_FS_FATFS;
fs_drv.letter = LV_FS_FATFS_LETTER;
fs_drv.cache_size = LV_FS_FSTFS_CACHE_SIZE;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo here.

fs_drv.cache_size = LV_FS_FSTFS_CACHE_SIZE;

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, both are fixed here: 74091c4

#define LV_USE_FS_FATFS 0
#if LV_USE_FS_FATFS
#define LV_FS_FATFS_LETTER '\0' /*Set an upper cased letter on which the drive will accessible (e.g. 'A')*/
#define LV_FS_FATSF_CACHE_SIZE 0 /*>0 to cache this number of bytes in lv_fs_read()*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well.
#define LV_FS_FATSF_CACHE_SIZE 0 />0 to cache this number of bytes in lv_fs_read()/

kisvegabor added a commit that referenced this pull request Jan 29, 2022
@eismeraldo
Copy link
Contributor Author

My new PC is now set up and I was able to debug. The first driver is created based on lv_conf.h and the second is mine based on https://docs.lvergleich.io/master/overview/file-system.html . I have now deactivated the driver in lv_conf.h and now everything runs perfectly.

@kisvegabor
Copy link
Member

And BMPs are loaded faster too? 🙂

@eismeraldo
Copy link
Contributor Author

I haven't measured the running time, but it feels a little better. Now 40 read and seek operations are performed instead of 100. Are the seek and open close operations really necessary? If I read in the same BMP by hand, it gives Open → read until end → close

@kisvegabor
Copy link
Member

Some of them yes as LVGL needs to collect some info about the image on various places.
Try setting LV_IMG_CACHE_DEF_SIZE 1. I think it will reduce the open count a little.

@eismeraldo
Copy link
Contributor Author

I took measurements and found something surprising.
LV_IMG_CACHE_DEF_SIZE 0 throughput time = 1,196
LV_IMG_CACHE_DEF_SIZE 1 throughput time = 5.11
In this variant, two files are somehow read in overlapping. But the display works perfectly. Thus, the time measured between the first open and the last close means nothing.

Now I made measurements with different cache_size:
cache_size = 0 throughput time = 1,029
cache_size = 512 throughput time = 1,196
cache_size = 2048 throughput time = 2.07

What happened here? I noticed that after every read of the size of the cache_size, a subsequent seek is called, which moves the position by exactly 200 bytes instead of the cache_size. That's why the cache_size is now counterproductive.

Now I have implemented a dummy function that does not seek to the SD card.
Without seek, cache_size = 2048 throughput time = 0.721
The throughput time is now very good, unfortunately the images are no longer displayed correctly.
In the case of cache_size > 0, the seek would probably have to be handled by the cache functionality.

@kisvegabor
Copy link
Member

lv_fs_seek should handle seek in there is cache.

At this moment I have no time to debug it, but if you find something please send a PR or comment here.

@eismeraldo
Copy link
Contributor Author

To me it looks like the BMP is read from back to front, but the data is stored in the cash from front to back.
In the first pass, image 1, position(p) is set to 19938.
Image01

As a result, the driver's seek is executed (Figure 2) because file_p→cache→end = 2048 and file_p→cache→file_position = 19938.
Image02

The next time the position is 19738, then 19538 etc. See LOG
log5_seek_problem.txt

@kisvegabor
Copy link
Member

To me it looks like the BMP is read from back to front, but the data is stored in the cash from front to back.

Ahh, it really results in a lot of seek operations and renders cache useless. Even worse with a larger cache LVGL reads more data for nothing...

What if you covert the BMP image to a binary here?
The result file is just like a BMP image but not mirrored. So it should be much faster.

@eismeraldo
Copy link
Contributor Author

The conversion works perfectly and the images are rendered in a fraction of the variant via SD card. My problem was a memory problem, I'm not moving on a PC but on an ESP32 with limited memory. For this reason, I tried to import the background images from the SD card, which represent different schemes of a heating system, and render them. In the meantime, I've decided to forget the graphical schemes and limit myself to the on-board resources of LVGL.
So thanks again for the help and the great LVGL.

@kisvegabor
Copy link
Member

Anyway, it was a meaningful lesson. I learnt a lot from it. Thank you for all the tests and debugging. 👍

amirgon added a commit to lvgl/lv_binding_micropython that referenced this pull request Mar 5, 2022
Add an optional cache_size argument to fs_register

More information about font caching: lvgl/lvgl#2979
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

5 participants