Configuration changes are not persisted to SD card on ESP32

[toholio]

I have been working on a large scale mobile decoder running on an esp32 (with a lot of assistance from and collaboration with @atanisoft). Currently my code is using the Arduino framework.

I have been attempting to change from storing the configuration file using SPIFFS to an SD card via SD_MMC. However if I make changes to the configuration, say setting the node name, via JMRI the changes are not written to the SD card. This is true even using AutoSyncFileFlow with the file descriptor from SimpleStackBase::create_config_file_if_needed().

After adding some very chatty logging I noticed that the file descriptor from SimpleStackBase::create_config_file_if_needed() is different to the one used when saving data from JMRI. @atanisoft made the suggestion to make these to be the same by changing

openmrn/src/openlcb/SimpleStack.cxx

Lines 141 to 142 in 0b5f09f

	auto *space = new FileMemorySpace(
	SNIP_DYNAMIC_FILENAME, sizeof(SimpleNodeDynamicValues));

and

openmrn/src/openlcb/SimpleStack.cxx

Line 160 in 0b5f09f

auto *space = new FileMemorySpace(CONFIG_FILENAME, CONFIG_FILE_SIZE);

to both use the file descriptor from configUpdateFlow_.open_file(CONFIG_FILENAME). This same suggestion is in #398.

In addition it looks like there should be an optional call to fsync() just before

openmrn/src/openlcb/MemoryConfig.cxx

Line 145 in 0b5f09f

return ret;

to ensure changes actually get persisted on the esp32's SD card. @atanisoft suggested adding either

#ifdef ESP32
fsync(fd_);
#endif

or, assuming appropriate use of DEFAULT_CONST_FALSE() and OVERRIDE_CONST_TRUE() elsewhere,

if (config_enable_fsync_on_write() == CONSTANT_TRUE)
{
  fsync(fd_);
}

just before FileMemorySpace::write() returns on completing a write operation.

The latter seems preferable. I'm happy to create a pull-request for this change if there's no problem with that approach.

[atanisoft]

@balazsracz thoughts on these updates? I've tested the usage of configUpdateFlow_ for the config file but not for SNIP. Based on the HASSERTs though it may be best to use just one FD for both usages. We had also discussed adding the call to fsync() on write. We have seen this caching behavior on a few FS now (SPIFFS flushes to storage on write implicitly due to the nature of the FS).

[balazsracz]

I think it is a good idea to reduce the number of fds that are open for the same file. I believe for all users of simplestack the SNIP_DYNAMIC_FILENAME and the CONFIG_FILENAME are the same.

I am not supportive of adding fsync after each write. A POSIX compatible filesystem should not need fsync to persist the data. Any filesystem that has write caching should have some form of background thread that slowly but surely flushes the write cache to disk. The flushflow does this for SPIFFs that we integrated. What you might need to evaluate is what does the flush flow exactly do for your filesystem.

Adding a flush after each write is equivalent of disabling write caching. Maybe you can do that in your filesystem driver.

The reason I don't recommend to run without write caching is that block aligned flash based filesystems deal rather poorly with small writes. For any write (even 1 byte) they typically have to copy/re-write an entire block and update all of the block map structures. The OpenLCB protocol has no guarantees that writes that are incoming are large. Typically JMRI sends every single field as a separate write. The 1-second flush timer was chosen specifically so that when JMRI starts a sequence of updates, the timer gets restarted again and again until JMRI stops the write sequence. This way only one flush happens, after the sequence is complete.

[toholio]

That seems pretty reasonable and makes sense. Thanks @balazsracz

Changing simplestack to obtain the fds using configUpdateFlow_.open_file(CONFIG_FILENAME) avoids the problem for my immediate use case, as well as freeing up some additional fds for sound playback and letting me drop the SPIFFS partition.

I'll create a pull-request for #398 for discussion. I may not understand the implications fully.

[atanisoft]

I believe for all users of simplestack the SNIP_DYNAMIC_FILENAME and the CONFIG_FILENAME are the same.

It would be near impossible for them to be anything but the same file due to https://github.com/bakerstu/openmrn/blob/master/src/openlcb/SimpleStack.cxx#L310 and https://github.com/bakerstu/openmrn/blob/master/src/openlcb/SimpleStack.cxx#L331. The SimpleStack::create_config_file_if_needed method is called from many of the app_main instances and those that don't have that call are usually calling SimpleStack::check_version_and_factory_reset.

The 1-second flush timer was chosen specifically so that when JMRI starts a sequence of updates, the timer gets restarted again and again until JMRI stops the write sequence.

That is similar to what AutoSyncFlow does, it calls fsync() after N seconds on the fd returned from the two linked methods above. The problem that @toholio is reporting is that this FD differs from the FDs used by SNIP/CONFIG in the MemoryFlow interface. I've been testing with the proposed change (but only for CONFIG_FILE) and it works fairly well for that, but I agree it may be confusing to have the filename ignored entirely on the second open call.

What you might need to evaluate is what does the flush flow exactly do for your filesystem.

In the case of SD on the ESP32 it will flush the cache to the SD card. If the write cache is empty it will be a no-op. There are complications in both SD and LittleFS regarding opening the same file from two FDs as they can diverge since each will have it's own unique cache (not ideal). For SD this caching per FD may not be fixable as it appears to be from the FatFS layer and not the ESP-IDF side. LittleFS has a pending PR but will not merge it until the next major release due to potentially being a breaking change. I've stopped using LittleFS for this reason.

Maybe you can do that in your filesystem driver.

I've checked the ESP-IDF config options and there is only one option related to caching and that is to move to a "global" cache vs per-FD cache. There is no option to disable it entirely unfortunately. I'd also recommend against disabling it due to the cost of a few SPI transactions for each read/write which the cache avoids typically. The Arduino users would also have no access/ability to modify the pre-compiled ESP-IDF libraries used by the Arduino IDE.