Skip to content

Fix the issue #3612 , Updating the SPIFFS to f5e26c4 #3623

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

Merged
merged 3 commits into from
Sep 22, 2017

Conversation

Lan-Hekary
Copy link
Contributor

@Lan-Hekary Lan-Hekary commented Sep 19, 2017

the same suggested modification in this pull request igrr/mkspiffs#27

further info in this issue pellepl/spiffs#165 and the commit pellepl/spiffs@f5e26c4

also don't forget to update the mkspiffs release
I guess this can only be done by you 😃

@igrr
Copy link
Member

igrr commented Sep 20, 2017

Thanks for the PR!

To make it easier to follow SPIFFS updates in the future, would you mind adding SPIFFS commit ID to your commit description (e.g. "SPIFFS: update to f5e26c4")?

I will then update this branch by adding new mkspiffs release.

@Lan-Hekary
Copy link
Contributor Author

Done I will Update this PR description too

@Lan-Hekary Lan-Hekary changed the title Fix the issue #3612 , Updating the SPIFFS with the latest fix Fix the issue #3612 , Updating the SPIFFS to https://github.com/pellepl/spiffs/commit/f5e26c4e933189593a71c6b82cda381a7b21e41c Sep 20, 2017
@Lan-Hekary Lan-Hekary changed the title Fix the issue #3612 , Updating the SPIFFS to https://github.com/pellepl/spiffs/commit/f5e26c4e933189593a71c6b82cda381a7b21e41c Fix the issue #3612 , Updating the SPIFFS to f5e26c4 Sep 20, 2017
@me-no-dev
Copy link
Collaborator

@igrr are you postponing this to be in sync with mkspiffs?

@igrr
Copy link
Member

igrr commented Sep 21, 2017

Yes, I still need to update package links for mkspiffs, and revert the SPIFFS config option which caused compatibility issues with 2.3.0.

@Lan-Hekary
Copy link
Contributor Author

about SPIFFS config options .. what are the options that are causing the compatibility issues ??

@igrr
Copy link
Member

igrr commented Sep 21, 2017

According to @me-no-dev it is https://github.com/igrr/mkspiffs/blob/master/spiffs/spiffs_config.h#L202, but it would be nice to test that (i.e. prepare FS with older version, try to read it with the new one).

@Lan-Hekary
Copy link
Contributor Author

OK I got that .. I will test it now
I am using PlatformIO so it's easy to change between 2.30 and 2.4.0 cores in a line :D
should I make a pull request if I verified it of leave that to you guys ?

@me-no-dev
Copy link
Collaborator

I would vote for a separate PR just so it outlines the change, but whatever the boss says :)

@igrr
Copy link
Member

igrr commented Sep 21, 2017

Well, I didn't mean adding these extra changes to this PR. It can go in as is. Just would make sense to merge all changes (roughly) at the same time, to prevent someone from grabbing non-compatible versions of mkspiffs and arduino-esp8266.

@Lan-Hekary
Copy link
Contributor Author

I tested the changes .. it seems OK and the compatibility is maintained across the different versions and I tested with various files some of them well above 25k and it's Perfectly OK ..

I will make another pull request in the mkspiffs with the this change ..
but I won't be able to do another PR here because I edited the master branch in my fork
( I know I should've made a new branch ... I won't make this mistake again :D )

actually I've been meaning to ask .. Do you have a Guide on how to make a proper PRs and mastering git commands ??? sometimes I have difficulties with git in how to manage the remote fork I am making modifications to ..

@Lan-Hekary
Copy link
Contributor Author

@me-no-dev , actually this change is directly related to the issue mentioned in the PR's title
but I will do whatever the boss says :D

@igrr igrr merged commit 1683b12 into esp8266:master Sep 22, 2017
d-a-v pushed a commit to d-a-v/Arduino that referenced this pull request Sep 29, 2017
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