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

RFC: SPIFFS config for SDK 3.0 builds #2960

Closed
TerryE opened this issue Nov 11, 2019 · 12 comments
Closed

RFC: SPIFFS config for SDK 3.0 builds #2960

TerryE opened this issue Nov 11, 2019 · 12 comments
Assignees
Labels

Comments

@TerryE
Copy link
Collaborator

TerryE commented Nov 11, 2019

Background

The current make includes a target within the tools directory which I think that I broke with the SDK 3.0 changes. I want to fix this but before I do so I would like to establish a consensus view on what we should be doing here.

The SDK 2.x release behaviour was to build 3 SPIFFS if the directory local/fs exists and contains files. This images were sized to start immediately after the end of the firmware and fill the remaining flash size (½, 1 and 4 Mb).

Issues

  • The ½Mb flash size is no longer supported with NodeMCU and many modules now ship with 16Mb.
  • The initial format time, the restart time and SPIFFS access times increase linearly with SPIFFS size, so it makes little sense always configuring all remaining Flash as SPIFFS. A more sensible approach is to size SPIFFS depending on the application needs with a sensible headroom.
  • Also placing the SPIFFS partition immediately after the firmware image is in general a bad idea as this leaves no room for an LFS partition which must be placed in the 1st 1Mb window. (So I typically allocate 1 or 2 Mb starting at the 1Mb boundary to leave the 1st Mb for firmware and LFS partitions.)
  • Should we tune the block size? I discussed SPIFFS configuration for SDK 3.0 in SPIFFS under SDK 3.0 #2687, and which I then implemented, but I missed that I needed to tweak some of the logic in spiffsimg to mirror this. My view is that unless one of the other committer can come up with a compelling reason otherwise we stick with the conclusions of SPIFFS under SDK 3.0 #2687.
  • Documenting SPIFFS configuration. We should provide some clear and easy to follow advice on how best to configure SPIFFS and any automatic build within the make should follow this. E.g we just have a SPIFFS=n target in the make, which if specified triggers the build of a SPIFFS of size n. We should also drop the embedded hex location in the image name as this is a misleading convention as SPIFFS are not position dependent.
@jmattsson
Copy link
Member

  • Yeah I think our biggest legacy support need is for 1meg Sonoff devices.

  • As long as it's easy to (re)configure the SPIFFS partition size, I agree that something like 1meg default max would make sense.

  • From memory someone (maybe Philip, maybe I?) did a bunch of tests with different tuning values before we arrived at the current ones. Sticking with the SPIFFS under SDK 3.0 #2687 values makes sense, as long as we don't start assuming they'll always be those values. I.e. if we later decide that e.g. 16k/512 is smarter for us, that should be as simple as changing the defines - we shouldn't need to then also twiddle something unrelated in the HAL or some such.

  • I like a SPIFFS=n approach, but wouldn't that cause issues with the partition table? Generating SPIFFS images that won't fit into the partition would seem like a user-unfriendly thing to do.

    I'm not convinced about dropping the hex location from the file name - it's a helpful reminder of where that blob should be flashed when you're doing it by hand. I'm also not convinced that we should keep it. What I'm trying to say is that I don't want to lose the hint of where I'm supposed to drop the SPIFFS image, but the information doesn't necessarily have to be embedded in the filename. Maybe a simple report print at the end of the SPIFFS image build target stating where it should be flashed given the current partition layout would do the trick.

@TerryE
Copy link
Collaborator Author

TerryE commented Nov 14, 2019

@jmattsson Johny, In practice I think that we have 3 main usecases for SPIFFS:

  • 1Mb class devices where the SPIFFS will be slotted in above the firmware + LFS and will be whatever needed to lever in the app.
  • 4 or 16Mb devices where the SPIFFS will sit on the 1Mb boundary and will be comfortably sized for the application, but IMO typically ½ or 1Mb is sufficient for most apps. It is better keeping it small because the access times are roughly proportional to the FS size.
  • Logging or other apps where the user wants to have the SPIFFS as big as possible. A mistake IMO, but some developers want this.

In terms of the hex location, etc, the whole idea of having a nodemcu-partition tool is that this allows you to configure the partitions, so I use:

esptool --port /dev/ttyUSB0 --baud 460800 --after no_reset write_flash -fm dio 0x00000 bin/0x00000.bin 0x10000 bin/0x10000.bin
tools/nodemcu-partition.py -ls 256K -sa 1M -ss 1M

to image my devices for testing. The SPIFFS image itself is PIC so I don't see the point in labelling it with an address, the tool will load it at the correct location.

@TerryE
Copy link
Collaborator Author

TerryE commented Dec 11, 2019

@albarrett @KT819GM @momkey02 @HHHartmann @devsaurus et al: any views here?

@KT819GM
Copy link

KT819GM commented Dec 11, 2019

@TerryE
I always keep SPIFFS <=256Kb (it was before LFS, now it's usually 128Kb). In my, far from professional opinion:

  1. Most important issue is "Documenting SPIFFS configuration." Users will go with any configuration approach made by the man with experience like yours if it will be understandable to them. I personally prefer to build firmware editing user_config.h and so on, but I will follow and will learn other ways with no problems at all.
  2. The re implementation of the SPIFFS from the local/fs should be focused on 4 Mb devices keeping 1 Mb spiffs size and starting at the 1Mb boundary, but as always possibility should be left to alter these parameters in _config.h.
  3. In range of my comprehension I agree with every option mentioned in SPIFFS under SDK 3.0 #2687
    I will try to get my hands again on nodemcu-partition.py concept before Christmas and hopefully will give more useful answers for this issue.

@HHHartmann
Copy link
Member

I think we also have to look at the SPIFFS configuration if no image is given. It should be configurable the same way. It should also allow for an "all available ram" option as is now.

It might also make sense to be able to configure the size in the code itself. Allowing for generic builds by say docker.

Maybe it makes sense to still generate multiple images if no configuration is given, based on different flash sizes.
Remaining space for 1 MB devices and 1 MB for others.

We could also allow giving a flash size instead of SPIFFS size and calculate according to the rules above.

I like the idea of placing the SPIFFS image above 1 MB so it is persisted when updating the firmware. Maybe it would also make sense to place LFS at the upper boundary of the 1st MB

@TerryE
Copy link
Collaborator Author

TerryE commented Dec 12, 2019

@HHHartmann LFS must be inside the first 1Mb because it is accessed through ICACHE mapping; The SPIFFS content is accessed through the SPIread API and so can be anywhere.

Using the "remaining space" in the 1Mb configs is a little dangerous since devs might want to split this between LFS and SPIFFS. I suggest that we add a SPIFFS=size option to the make with a default of 1 Mb and alternatively allow the developer to drop his or her own spiffs make script in tools and let the make use this if it exists.

@HHHartmann
Copy link
Member

@TerryE I know LFS must be below 1MB, I meant to align it at the 1MB border and have it downwards from there.
There is also the option to define the LFS size in user_config.h. This should still work. Also SPIFFS_MAX_FILESYSTEM_SIZE seems useful to me.

@TerryE
Copy link
Collaborator Author

TerryE commented Dec 17, 2019

I want to move this one forward, and of the suggestions above, I feel on balance that we should:

  • Honour the SPIFFS_MAX_FILESYSTEM_SIZE parameter in user_config.h and auto-make any SPIFFS using this setting.
  • The default value for this is 1Mb

Setting the "max within N Mb flash" is problematic as the build process doesn't know the size of the target Flash memory nor the LFS partition size. Any suggestions on how to work around this?

@jmattsson
Copy link
Member

As long as building the SPIFFS image is the last thing in the build process, it should be doable I think? The last used (mapped) address can be read from the NodeMCU.map file and easily converted to the physical address. If the firmware has already allocated size for the LFS internally, that would be all you need. Otherwise grep the location(?) and size definition for the LFS, align the start as needed relative to the physical address, and you'd have the SPIFFS address, possibly after another alignment to give a bit of wriggle room for firmware changes, just as the internal routines do.

@TerryE
Copy link
Collaborator Author

TerryE commented Dec 18, 2019

Johny, with SDK 3.0 the firmware uses the PT settings and nothing else. You have the options of reconfiguring the PT when downloading the firmware or even reconfiguring it through the node API on the provisioned ESP. The user_config settings are really a legacy option to set up a default PT.

If we now want 3 ways of doing this, then which takes precedence? If we aren't careful, then we will just end up with a buggers muddle that will just confuse everyone. 💩 I am getting confused.💩

Maybe it would be better just to merge the 5.3 support PT into dev, and let one of the other committers take point on sorting out SPIFFS.

@jmattsson
Copy link
Member

Ah, I keep forgetting about the partition table support. Yeah, that really makes "max within N MB flash" hard. I don't suppose there's a growfs command for SPIFFS? :D

@TerryE
Copy link
Collaborator Author

TerryE commented Dec 18, 2019

Sorry J. Late night. We had a lot of feedback that the user_config config process was too inflexible for cloud builders, hence on SDK 3.0 version, I added the ability to use / edit the PT througha utility. Also requests to be able to do this at runtime, hence adding this to the node API. Maybe doing these was a mistake since they don't seem to be really used and people still want the full feature functionality or more through user_config. The whole system is just getting layers of complexity IMO. Maybe we need a rethink and simplification

@TerryE TerryE closed this as completed Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants