Handle preset deletion asynchronously for consistency with preset saving

[david-hogberg]

Is your feature request related to a problem? Please describe.
I'm developing a BLE mod for wled which works fine for commands except preset/playlist deletion (device crashes). I then noticed that presets are deleted using immediate file operations rather than asynchronously using flags, which is how presets are saved for example. Same for API calls. Is this there a reason for this or a bug? Issue solved if I move deletion to be handled later in the main loop (see below).

Deletion still works fine using wifi, but thought it might be a general stability improvement anyway.

Describe the solution you'd like
In presets.cpp: Instead of directly initiating file operations in deletePreset(index), called from deserializeState, set a flag presetToDelete = index that's picked up in handlePresets(). Something like this:

void handlePresets()
{
  if (presetToDelete) {
    doDeleteState();
    return;
  }

  if (presetToSave) {
    doSaveState();
    return;
  }
  
  ...
  
}

Adds consistent behaviour between preset deletion and saving, and prevents potential blocking operations during preset deletion.

[DedeHai]

this looks like it could solve some issues users have with corrupted files (as referenced by @softhack007)
@david-hogberg since you already looked into this: are you able to implement the solution you suggested and do a PR?

[david-hogberg]

Sure, I'll create a PR

[blazoncek]

Asynchronous preset handling was introduced to combat preset corruption that was seen on certain ESP32 devices (I am unaware of "regular" corruption on ESP8266, ESP32-C3, ESP32-S2 or ESP32-S3).

As discussed elsewhere the corruption most likely comes from old and inadequate bootloader/framework configuration used in those devices.

If there was no such corruption occurring all preset handling would still be synchronous/immediate. Which would render your implementation of BLE useless. So this raises the question: where should this be fixed? In WLED core or in your solution?
What is most likely wrong with your implementation is that it calls preset handling routines while in ISR or while flash is locked for other operations.

IMO when we update ESP32 binaries to use newer bootloader and recent framework preset handling should be executed synchronously (as asynchronous has its drawbacks that had to be mitigated) as it was in the past.

[david-hogberg]

@blazoncek Thanks for your input. My point here is more about consistency, that saving and deleting are approached in different ways as of now. Which option to choose of course depends on where WLED is heading in general with immediate vs. asynchronous operations. Let me know if a PR might be useful still.

[blazoncek]

I would prefer synchronous preset handling as it has less drawbacks than current implementation. After we solve corruption issues, one way or the other.

[willmmiles]

Probably we need a mutex over the presets database. I think the current code can get in to a lot of trouble if there's requests via different APIs in flight.

[blazoncek]

I disagree. We need to fix preset corruption and then return original immediate/synchronous preset (load/save/delete) handling.

[willmmiles]

File corruption can be caused by attempting parallel operations on the same file from different tasks (eg. web server callback is writing while loop is trying to read -- or worse, also trying to write). Maybe I haven't been around long enough, but if I was facing corruption issues, the first thing I'd try is making sure only one task can use the file at a time, so every operation runs to completion before the next operation can see the changes.