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

Single FileSystem class #84

Merged
merged 39 commits into from
Apr 9, 2024
Merged

Single FileSystem class #84

merged 39 commits into from
Apr 9, 2024

Conversation

frostmorn
Copy link
Collaborator

@frostmorn frostmorn commented Mar 29, 2024

closes #28

Still need to check some places on possible mistakes

@frostmorn frostmorn added wip Work in progress scope:sdk labels Mar 29, 2024
@frostmorn frostmorn removed the wip Work in progress label Mar 29, 2024
@frostmorn frostmorn requested a review from and3rson March 29, 2024 12:49
@and3rson
Copy link
Owner

and3rson commented Mar 29, 2024

Хороша ідея, і рано чи пізно це мало статись, тому я радий, що ти добрався до цього :) Але я все ж маю трохи критики щодо цього підходу.

Я згоден, що треба викинути lilka::SD і lilka::SPIFFS. Але абстракція FileSystem, як на мене, лише приховує та ускладнює вже існуючі SD і SPIFFS з arduino-фреймворку esp32.

Також я згоден, що добре мати "хелпер" FileSystem з методами типу isXXXAvailable / mountXXX, а також методи для конверсії шляхів між канонічними та відносними. Крім того, там можна залишити функцію listDir.

Проте стосовно решти функцій - чому б не використовати вже існуючі SD і SPIFFS (не наші, а з esp32)? В більшості випадків нам треба буде працювати з SD-шляхами, а канонічні шляхи потрібні лише для низькорівневих бібліотек чи портів (наприклад, nofrendo чи doom).

Тим паче, існуючі (ардуїнівські) SD і SPIFFS наслідують FS, тому з ним теж можна без проблем мати єдину функцію для браузера з сигнатурою fileBrowser(FS* driver, String path). А для конверсії шляхів в абсолютні можна якраз додати методи в FileSystem (як я описав тут: #15 (comment))

TL;DR: Я вважаю, що FileSystem не має бути обгорткою для SD та SPIFFS, а лише хелпером, що здіснює ініціалізацію носіїв та надає деякі методи для зручності (конверсія шляхів і лістінг директорій). (Інший приклад - lilka::Audio: він ініціалізує звук, але НЕ обгортає існуючі I2S чи i2s_***)

FileSystem в такому вигляді, як ти пропонуєш, мав би сенс, якби не було вбудованих класів SD і SPIFFS.

@and3rson
Copy link
Owner

and3rson commented Mar 30, 2024

Виглядає супер! От тільки я б залишив sdcard в Lua як є, все-таки юзери переважно будуть працювати в ній лише з SD-картою. Але можна трохи змінити код, щоб замість open(LILKA_SDROOT + path) були SD.open(path). Тоді ми пізніше зможемо зробити інші Lua-модулі для інших ФС.

А stdlib-варіанти функцій в майбутньому буде краще заімплементувати як модуль io (конвенційна назва модуля Lua). До речі, здається, io в Lua є по дефолту :) Тому не буде сенсу писати свій io.

@frostmorn frostmorn added the wip Work in progress label Mar 30, 2024
@frostmorn frostmorn removed the wip Work in progress label Mar 30, 2024
Copy link
Owner

@and3rson and3rson left a comment

Choose a reason for hiding this comment

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

Кльово, мені це дуже подобається!

sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.h Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.h Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.h Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.h Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.h Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.h Show resolved Hide resolved
@frostmorn frostmorn requested a review from and3rson April 1, 2024 06:02
Copy link
Owner

@and3rson and3rson left a comment

Choose a reason for hiding this comment

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

Дуже добре! Ми вже дууууже близько!

sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.cpp Outdated Show resolved Hide resolved
sdk/lib/lilka/src/lilka/fileutils.cpp Show resolved Hide resolved
firmware/keira/src/apps/launcher.cpp Outdated Show resolved Hide resolved
@frostmorn frostmorn requested a review from and3rson April 3, 2024 20:59
@and3rson
Copy link
Owner

and3rson commented Apr 9, 2024

Все супер, все працює!
Допишу трохи документації, внесу дрібні правки і можна буде мерджити. :)

and3rson
and3rson previously approved these changes Apr 9, 2024
@and3rson and3rson merged commit 7bffdc4 into main Apr 9, 2024
3 checks passed
@and3rson and3rson deleted the single_class_for_filesystems branch April 9, 2024 19:17
@and3rson
Copy link
Owner

and3rson commented Apr 9, 2024

🎉 🎉 🎉

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

Successfully merging this pull request may close these issues.

Eliminate lilka::SD and lilka::Filesystem into a single helper class
2 participants