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

Fixed the SPIFFS size calculation. #1653

Merged
merged 5 commits into from
Mar 28, 2019

Conversation

slaff
Copy link
Contributor

@slaff slaff commented Mar 26, 2019

Closes #1313.

@slaff slaff added this to the 3.8.0 milestone Mar 26, 2019
@slaff
Copy link
Contributor Author

slaff commented Mar 26, 2019

@jonnykl Please, check this PR and tell me if it is working for you.

@@ -67,14 +73,14 @@ bool spiffs_format_internal(spiffs_config *cfg)
sect_last = flashmem_get_sector_of_address(sect_last);
debugf("sect_first: %x, sect_last: %x\n", sect_first, sect_last);
ETS_INTR_LOCK();
int total = sect_last - sect_first;
int total = sect_last - sect_first + 1;
Copy link

Choose a reason for hiding this comment

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

This is not correct (see PR 1313)

Copy link

Choose a reason for hiding this comment

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

As you can see (here) I also did the same change but in conjunction with the change in line 67 (which is the fix of the bug).

@jonnykl
Copy link

jonnykl commented Mar 26, 2019

Except the change I commented a few minutes ago everything is correct but does not solve the problem. You prevent the developer from using illegal values which is good but anyways the function spiffs_format_internal() erases the sectors for SpifFS and one sector after the end of SpifFS. E.g. if SPIFF_SIZE is set to the maximum allowed value, the sector where esp_init_data_default.bin is flashed will be erased.

@jonnykl
Copy link

jonnykl commented Mar 27, 2019

The fix works now if cfg->phys_addr and cfg->phys_size is a multiple of the physical block size.

Pro (for this fix):

  • almost all of the space requested is used ("almost" because SpifFS is organized in logical blocks, e.g. if logical block size would be 5 and the request space is 13, then only 2 logical blocks / 10 bytes would be used)

Con:

  • data which does not belong do SpifFS may be deleted

This fix is OK, but this behavior should be documented (bytes from cfg->phys_addr + cfg->phys_size (inclusive) to the end of the physical block will be erased when spiffs_format_internal() is called).

Pro (my fix):

  • no other data which does not belong to SpifFS is deleted

Con:

  • less space than requested is used (if requested space is not of multiple of physical block size)

I think both versions of the fix are OK. They only differ if the developer specifies a offset/size that the FS does not fit exactly in physical blocks.

maxAllowedEndAddress = INTERNAL_FLASH_SIZE - 1;
requestedEndAddress = cfg.phys_addr + SPIFF_SIZE - 1;
if(requestedEndAddress > maxAllowedEndAddress) {
cfg.phys_addr &= 0xFFFFF000; // get the start address of the sector
Copy link

Choose a reason for hiding this comment

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

I think the following code would be better:

cfg.phys_addr -= cfg.phys_addr % INTERNAL_FLASH_SECTOR_SIZE;

@slaff slaff merged commit 4b4501a into SmingHub:develop Mar 28, 2019
@slaff slaff removed the 3 - Review label Mar 28, 2019
@slaff slaff mentioned this pull request Apr 5, 2019
4 tasks
@slaff slaff deleted the fix/spiffs-internal-format branch July 22, 2019 12:47
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