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

Reorder Preset IDs #3929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 106 additions & 2 deletions wled00/presets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ static char *tmpRAMbuffer = nullptr;
static volatile byte presetToApply = 0;
static volatile byte callModeToApply = 0;
static volatile byte presetToSave = 0;
static volatile byte rearrangePresets = 0;
static volatile int8_t saveLedmap = -1;
static char quickLoad[9];
static char saveName[33];
Expand All @@ -20,6 +21,95 @@ static const char *getFileName(bool persist = true) {
return persist ? "/presets.json" : "/tmp.json";
}


// Function to retrieve the preset name from a JSON object
const char * getPresetNameFromJsonObject(const JsonObject& presetObj) {
if (presetObj.containsKey("n")) {
return (const char*)presetObj["n"];
} else {
return nullptr;
}
}

// Function to find the preset ID by its name
int findPresetIdByName(const char* targetName) {
String currentName;
byte maxPresetId = 251;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maximum allowable preset ID is 250, not 251.

for (byte i = 1; i <= maxPresetId; i++) {
if (getPresetName(i, currentName)) {
if (currentName == targetName) {
return i; // Return the matching preset ID
}
}
}
return -1; // Return -1 if no matching preset is found
}

void rearrangePresetIds(int oldId, int newId) {

// Check if the IDs are the same or invalid
if (oldId == newId) {
//Serial.println(F("Old ID and New ID are the same. No changes required."));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would require using DEBUG_PRINT macros or remove these comments.

return;
}
if (newId < 1 || oldId < 1) {
/*Serial.print(F("Invalid ID provided. IDs: "));
Serial.print(newId);
Serial.print(F(", "));
Serial.println(oldId);*/
return;
}

rearrangePresets = 1;

// Determine the highest ID to dictate array size
int highestId = max(oldId, newId);
StaticJsonDocument<4096> *tempPresets = new StaticJsonDocument<4096>[highestId + 2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why StaticJsonDocument? For dynamic allocations you should use PSRAMDynamicJsonDocument.

The other thing is the limit to 4k. That's not enough. At minimum it should be the size of JSON_BUFFER_SIZE. Do not forget, you can store PixelMagic images inside preset.


// Load all presets into temporary storage
for (int i = 1; i <= highestId; i++) {
if (!readObjectFromFileUsingId(getFileName(true), i, &tempPresets[i])) {
//Serial.print(F("Error reading preset: "));
//Serial.println(i);
} else {
String presetName;
if (!getPresetName(i, presetName)) {
//Serial.println("Error reading preset name for ID: " + String(i));
presetName="ERROR READING NAME";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not all presets have name. Some do not use name at all.

// You may choose to continue the loop or handle the error differently.
}
JsonObject preset = tempPresets[i].as<JsonObject>();
preset["n"] = presetName;
}
}
tempPresets[highestId+1] = tempPresets[oldId];
// Swap preset data in memory according to the new rearrangement
if (oldId < newId) {
for (int i = oldId; i < newId; i++) {
tempPresets[i] = tempPresets[i + 1];
}
} else {
for (int i = oldId; i > newId; i--) {
tempPresets[i] = tempPresets[i - 1];
}
}

// Assign the originally moved preset to its new position
tempPresets[newId] = tempPresets[highestId+1];

// Save all modified presets back to the filesystem
for (int i = 1; i <= highestId; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is very inefficient and prone to flash corruption we are fighting since 0.11.

if (!writeObjectToFileUsingId(getFileName(true), i, &tempPresets[i]));
updateFSInfo();
// Introduce a delay to prevent corruption of the presets.json file.
delay(100);
}
// Clean up
delete[] tempPresets;
tempPresets = nullptr;
rearrangePresets = 0;
}

static void doSaveState() {
bool persist = (presetToSave < 251);
const char *filename = getFileName(persist);
Expand Down Expand Up @@ -81,7 +171,8 @@ static void doSaveState() {

bool getPresetName(byte index, String& name)
{
if (!requestJSONBufferLock(9)) return false;
//had to remove this so the rearange presets would work
//if (!requestJSONBufferLock(9)) return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a showstopper, unfortunately.
As ESP32 & S3 are dual core, concurrent use of JSON buffer needs to be prohibited. That's the purpose of requestJSONBufferLock(). If lock cannot be acquired you cannot be sure that pDoc points to a valid JSON buffer.

bool presetExists = false;
if (readObjectFromFileUsingId(getFileName(), index, &doc))
{
Expand Down Expand Up @@ -205,7 +296,6 @@ void savePreset(byte index, const char* pname, JsonObject sObj)
if (sObj["n"].is<const char*>()) strlcpy(saveName, sObj["n"].as<const char*>(), 33);
else sprintf_P(saveName, PSTR("Preset %d"), index);
}

DEBUG_PRINT(F("Saving preset (")); DEBUG_PRINT(index); DEBUG_PRINT(F(") ")); DEBUG_PRINTLN(saveName);

presetToSave = index;
Expand All @@ -222,6 +312,20 @@ void savePreset(byte index, const char* pname, JsonObject sObj)
if (sObj[F("playlist")].isNull()) {
// we will save API call immediately (often causes presets.json corruption)
presetToSave = 0;
if(rearrangePresets==0){
String presetName;
if(getPresetName(index, presetName)){
if(presetName!=saveName){
int oldId=findPresetIdByName(saveName);
if(oldId==-1){
}
else{
rearrangePresetIds(oldId, index);
return;
}
}
}
}
if (index > 250 || !fileDoc) return; // cannot save API calls to temporary preset (255)
sObj.remove("o");
sObj.remove("v");
Expand Down
Loading