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.

Sign up for GitHub

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

Jump to bottom

Allow to config the maximum number of VFS entries. (IDFGH-9641) #10987

Closed
wants to merge 2 commits into from
Closed

Allow to config the maximum number of VFS entries. (IDFGH-9641) #10987

wants to merge 2 commits into from

Conversation

oliverschmidt
Copy link
Contributor

Our project currently needs a copy of the vfs component solely to have an increased maximum number of VFS entries: FujiNetWIFI/fujinet-firmware@8598182

@CLAassistant
Copy link

CLAassistant commented Mar 14, 2023 •
edited

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Mar 14, 2023
@github-actions github-actions bot changed the title Allow to config the maximum number of VFS entries. Allow to config the maximum number of VFS entries. (IDFGH-9641) Mar 14, 2023
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@igrr igrr added the PR-Sync-Merge Pull request sync as merge commit label Apr 12, 2023
@igrr
Copy link
Member

igrr commented Apr 12, 2023

sha=6beeecbbcfd5b76d1951f6fbf2289faed1768f32

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Apr 12, 2023
igrr
igrr reviewed Apr 13, 2023
View reviewed changes
components/vfs/vfs.c
@@ -33,15 +33,14 @@

static const char *TAG = "vfs";

#define VFS_MAX_COUNT 8 /* max number of VFS entries (registered filesystems) */
Copy link
Member

@igrr igrr Apr 13, 2023 •
edited

Choose a reason for hiding this comment

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

One note, here I had to re-introduce VFS_MAX_COUNT macro so it's still defined even if CONFIG_VFS_SUPPORT_IO is not set. Otherwise the build would fail when CONFIG_VFS_SUPPORT_IO is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your time and sorry for wasting it on this not fully functional pull request!

I'm now instead following a pattern I find all over the place in the ESP-IDF code base.

@oliverschmidt
Make sure that VFS_MAX_COUNT is always defined.
e64761b 
Make sure that the VFS_MAX_COUNT macro so it's still defined even if CONFIG_VFS_SUPPORT_IO is not set -  and therefore the macro CONFIG_VFS_MAX_COUNT is not defined.
oliverschmidt
oliverschmidt commented Apr 14, 2023
View reviewed changes
Copy link
Contributor Author

@oliverschmidt oliverschmidt left a comment

Choose a reason for hiding this comment

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

Many thanks in advance for having another look at this pull request.

components/vfs/vfs.c
@@ -33,15 +33,14 @@

static const char *TAG = "vfs";

#define VFS_MAX_COUNT 8 /* max number of VFS entries (registered filesystems) */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your time and sorry for wasting it on this not fully functional pull request!

I'm now instead following a pattern I find all over the place in the ESP-IDF code base.

@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable labels Apr 17, 2023
@igrr
Copy link
Member

igrr commented Apr 20, 2023 •
edited

@oliverschmidt thanks again for the contribution! I have ended up merging the first version of your change (6beeecb, in master now) along with a follow-up commit (fee3082). It's roughly the same as the updated version of your PR, except for a different comment and a different default value. Hopefully this works for you.

@igrr igrr closed this Apr 20, 2023
@oliverschmidt
Copy link
Contributor Author

oliverschmidt commented Apr 20, 2023 •
edited

Thanks for the merge - and the explanation :-)

Hopefully this works for you.

For sure it does!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igrr igrr igrr approved these changes

Assignees

@igrr igrr

Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@oliverschmidt @CLAassistant @Alvin1Zhang @igrr @espressif-bot