-
Notifications
You must be signed in to change notification settings - Fork 13
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
VFS additions #12
base: master
Are you sure you want to change the base?
VFS additions #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the concept here but the second commit, the one that pulls in a ton of files from other repositories would be much smaller if we made use of submodules instead. Is that possible?
components/littlefs/LICENSE.md
Outdated
@@ -0,0 +1,165 @@ | |||
Apache License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible to add littlefs as a submodule vs. copying all of the files into the repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to, can you help figure out how?
.gitmodules
Outdated
@@ -1,3 +1,7 @@ | |||
[submodule "components/libesphttpd"] | |||
path = components/libesphttpd | |||
url = https://github.com/chmorgan/libesphttpd.git | |||
[submodule "components/mkspiffs"] | |||
path = components/mkspiffs | |||
url = https://github.com/phatpaul/mkspiffs.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to use the main GitHub repository for mkspiffs? Igrr's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified that project to get it to work as a submodule in ESP-IDF. I don't know if there is a cleaner way.
phatpaul/mkspiffs@04d419f
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phatpaul maybe reach out to igrr to see what he thinks? It may be that we can perform the configuration parts as a part of the build inside of this repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit linked above seems to be the correct approach, if you want to include mkspiffs as a submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igrr @phatpaul had to add this commit, phatpaul/mkspiffs@04d419f
Would that commit be accepted into your tree if submitted as a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phatpaul can you submit that commit as a PR? I tried but GitHub is reporting an error, probably because it requires some permissions on your fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and created PR. See igrr/mkspiffs#57
@@ -0,0 +1,156 @@ | |||
// Copyright 2015-2017 Espressif Systems (Shanghai) PTE LTD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use mkfatfs via a submodule as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to. I looked at doing that but couldn't figure out how.
The problem is that the source directory for that project is several layers deep.
https://github.com/jkearins/ESP32_mkfatfs/tree/master/components/mkfatfs/src
And it is my understanding that you can't check-out a sub-directory of a repo as a submodule.
Ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phatpaul thoughts about asking jkearins to split that off into a separate repository so other people can reuse it? You are correct that git doesn't allow what we need because he embedded his component into the subdirectory. There are some hacks to make it work like add a submodule to another directory like repos/ and then symlink from repos/mkfatfs/components/mkfatfs/src to components/mkfatfs but that seems kind of blah.
@phatpaul it may take a little while to discuss with igrr and jkearins but lets see how that goes first. If there is some impasse then we can discuss working around it. Does that sound ok? |
@phatpaul have you had any luck with those guys? To clarify my concerns if we merge back changes here we'll be copying or relying on forks of external repositories. If we can get those repositories adjusted then we can include them directly and won't have to worry about maintaining forks or copied files. Would it help if I reached out to them? I'd like to do what I can to facilitate getting your PR merged in as soon as possible. |
Yes please feel free help out. I have been busy at work finishing my
esp32-based product.
…On Sat, Dec 8, 2018, 8:39 PM Chris Morgan ***@***.***> wrote:
@phatpaul <https://github.com/phatpaul> have you had any luck with those
guys? To clarify my concerns if we merge back changes here we'll be copying
or relying on forks of external repositories. If we can get those
repositories adjusted then we can include them directly and won't have to
worry about maintaining forks or copied files. Would it help if I reached
out to them? I'd like to do what I can to facilitate getting your PR merged
in as soon as possible.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADVVdzW8TxIBtZlhdiLTc27smRmR9NPnks5u3GnMgaJpZM4Ycame>
.
|
@chmorgan I will split this up to leave out the building local fs stuff. I would like to merge this in - The UI for VFS is in here and it does not depend on the mkfatfs. |
Actually it is already split into 2 commits. What's the best way to merge in just the first commit? Do I need to update the pull-request? Or rewind the branch? I would like to leave the 2nd commit in this or another pull-request. |
Hi @phatpaul. I'd do something like:
The branch makes a copy of your current branch and during the rebase you'd drop the commit you'd like to lose. The rebased branch with the single commit should show up here replacing the previous commits after the force push. |
Adding a vfs_upload example GUI. Depends on chmorgan/libesphttpd#48 (already merged)
Also add helper script to generate filesystem images (stolen from loboris/MicroPython_ESP32_psRAM_LoBo).
(I only got FatFS to work so far, and I don't have time right now to troubleshoot SPIFFS and LittleFS. I put a disclaimer in the readme to this effect)
The example still builds and runs fine without any of this VFS stuff turned on. (the filesystem upload functions just fail if used)