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

Refactor out staticDIR #3162

Draft
wants to merge 1 commit into
base: community
Choose a base branch
from
Draft

Refactor out staticDIR #3162

wants to merge 1 commit into from

Conversation

stellar-aria
Copy link
Collaborator

Removes global staticDIR in favor of local FatFS::Directory instances

Copy link
Contributor

Test Results

106 tests  ±0   106 ✅ ±0   0s ⏱️ ±0s
 16 suites ±0     0 💤 ±0 
 16 files   ±0     0 ❌ ±0 

Results for commit c3b082f. ± Comparison against base commit 547b62b.

Copy link
Collaborator

@m-m-adams m-m-adams left a comment

Choose a reason for hiding this comment

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

I think we also need to configure fatfs as re entrant to do this safely, this change means multiple different dirs can be held but fatfs still can't access anything other than the last one it opened. To do that I think we need mutex support (and blocking) as well

Copy link
Collaborator

@jamiefaye jamiefaye left a comment

Choose a reason for hiding this comment

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

The reason we have staticDIR is because iterating through a large directory is quite slow. Since there is a limit on how many directory entries can go into a SysEx reply, we limit each request to a small number of line items. If we iterated from the beginning to get to the subrange requested each time, it really does get slow. So I came up with a cache scheme.

@stellar-aria
Copy link
Collaborator Author

stellar-aria commented Dec 28, 2024

I think we also need to configure fatfs as re entrant to do this safely, this change means multiple different dirs can be held but fatfs still can't access anything other than the last one it opened. To do that I think we need mutex support (and blocking) as well.

@m-m-adams Looks like FatFS needs to be provided some stuff: ff_mutex_take, ff_mutex_give, ff_mutex_create and ff_mutex_delete.

The reason we have staticDIR is because iterating through a large directory is quite slow. Since there is a limit on how many directory entries can go into a SysEx reply, we limit each request to a small number of line items. If we iterated from the beginning to get to the subrange requested each time, it really does get slow. So I came up with a cache scheme.

@jamiefaye What do you mean? Git blame for staticDIR and staticFNO point to Rohan's original commit to the codebase. And staticDIR is never used in a function that doesn't call f_opendir() first, so it's never used contextually without a relevant creation the way that I would expect piecemeal loading works

@stellar-aria
Copy link
Collaborator Author

@jamiefaye if you're referring to sxDIR in

this PR does not touch that

@jamiefaye
Copy link
Collaborator

jamiefaye commented Dec 28, 2024 via email

@stellar-aria stellar-aria self-assigned this Dec 31, 2024
@stellar-aria stellar-aria marked this pull request as draft December 31, 2024 12:53
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.

3 participants