[BUG] invalid json format

[chromoxdor]

"Plugin Description":[Normal][Energy],

This is not a valid json format...

Edit:
You probably meant "Plugin Description": "[Normal][Energy]",

[chromoxdor]

And shouldn´t it be "[Energy]" only if it is an energy build?

[TD-er]

I think this is an unforeseen bug introduced by a fix for JSON elsewhere.
I only check to see if a given string is "wrapped" in [] as if it were an array.
But "[][]" isn't a JSON array.

No it consists of the "normal" plugin set and the energy plugin set.

[TD-er]

A quick fix in StingGenerator_System.cpp is could be:

String getPluginDescriptionString() {
  return F(
    ""
  #ifdef PLUGIN_BUILD_NORMAL
  "(Normal)"
  #endif // ifdef PLUGIN_BUILD_NORMAL
  #ifdef PLUGIN_BUILD_COLLECTION
  "(Collection)"
  #endif // ifdef PLUGIN_BUILD_COLLECTION
  #ifdef PLUGIN_BUILD_DEV
  "(Development)"
  #endif // ifdef PLUGIN_BUILD_DEV
  #ifdef PLUGIN_DESCR
  "(" PLUGIN_DESCR ")"
  #endif // ifdef PLUGIN_DESCR
  #ifdef BUILD_NO_DEBUG
  "(No Debug Log)"
  #endif
  #if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
  "(24tasks)"
  #endif // if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
  );
}

N.B. the bug is introduced in StringConverter.cpp

String to_json_value(const String& value, bool wrapInQuotes) {
  if (value.isEmpty()) {
    // Empty string
    return F("\"\"");
  }
  if (value.length() > 2) {
    // Check for JSON objects or arrays
    const char firstchar = value[0];
    const char lastchar = value[value.length() - 1];
    if ((firstchar == '[' && lastchar == ']') ||
        (firstchar == '{' && lastchar == '}')) 
    {
      return value;
    }
  }
[....]

This still needs to be like this, but since I don't want to make this function more complex for 1 odd example, I guess we can just change the braces like suggested above.

[chromoxdor]

What about this?

String getPluginDescriptionString() {
  return F(
    "["
  #ifdef PLUGIN_BUILD_NORMAL
  "\"Normal\""
  #endif // ifdef PLUGIN_BUILD_NORMAL
  #ifdef PLUGIN_BUILD_COLLECTION
  "\"Collection\""
  #endif // ifdef PLUGIN_BUILD_COLLECTION
  #ifdef PLUGIN_BUILD_DEV
  "\"Development\""
  #endif // ifdef PLUGIN_BUILD_DEV
  #ifdef PLUGIN_DESCR
  ",\"" PLUGIN_DESCR "\""
  #endif // ifdef PLUGIN_DESCR
  #ifdef BUILD_NO_DEBUG
  ",\"No Debug Log\""
  #endif
  #if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
  ",\"24tasks\""
  #endif // if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
  "]");
}

output: "Plugin Description":["Normal","Energy"] or "Plugin Description":["Normal","Collection_A ESP32"]

[chromoxdor]

Ahh, i think i didn't quite understand how it works.... :)
I thought the first three are either one or the other and the last three are appendages....
🤷‍♂️

Edit:
Because i tried a custom build: "Plugin Description":[,"No Debug Log"]
That doesn´t work of course...

[TD-er]

Like I said, I didn't want to make it too complicated...

[chromoxdor]

I know, it´s not super important but i thought I'll give it another try. :)
What do you think about that?:

String getPluginDescriptionString() {
  String result = F("[");
  #ifdef PLUGIN_BUILD_NORMAL
    result += F("\"Normal\"");
  #endif // ifdef PLUGIN_BUILD_NORMAL
  #ifdef PLUGIN_BUILD_COLLECTION
    result += F("\"Collection\"");
  #endif // ifdef PLUGIN_BUILD_COLLECTION
  #ifdef PLUGIN_BUILD_DEV
    result += F("\"Development\"");
  #endif // ifdef PLUGIN_BUILD_DEV
  #ifdef PLUGIN_DESCR
    result += F("\"" PLUGIN_DESCR "\"");
  #endif // ifdef PLUGIN_DESCR
  #ifdef BUILD_NO_DEBUG
    result += F("\"No Debug Log\"");
  #endif
  #if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
    result += F("\"24tasks\"");
  #endif // if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
  result += F("]");
  result.replace("\"\"", "\",\"");
  return result;
}

[TD-er]

Well I think it does add quite a lot of code to the binary.

This will save a number of assignments and new flash string pointers.

String getPluginDescriptionString() {
  String result = F("[
  #ifdef PLUGIN_BUILD_NORMAL
    "\"Normal\""
  #endif // ifdef PLUGIN_BUILD_NORMAL
  #ifdef PLUGIN_BUILD_COLLECTION
    "\"Collection\""
  #endif // ifdef PLUGIN_BUILD_COLLECTION
  #ifdef PLUGIN_BUILD_DEV
    "\"Development\""
  #endif // ifdef PLUGIN_BUILD_DEV
  #ifdef PLUGIN_DESCR
    "\"" PLUGIN_DESCR "\""
  #endif // ifdef PLUGIN_DESCR
  #ifdef BUILD_NO_DEBUG
    "\"No Debug Log\""
  #endif
  #if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
    "\"24tasks\""
  #endif // if FEATURE_NON_STANDARD_24_TASKS && defined(ESP8266)
  "]");
  result.replace("\"\"", "\",\"");
  return result;
}

[chromoxdor]

Ahh..right. Yours looks better.. as always :)

But why anyhow do it like this. Wouldn´t it be much more effective to generate a string when compiling and use that?
This doesn´t need to be created dynamically since it only changes with uploading a new firmware. (same with getSystemLibraryString())
If this is even possible. I still not fully understand the workings of this magic machinery...

[TD-er]

Well we don't know which plugins will be present, so it is wrong to start with a comma (or end with one).
Thus you have to replace something.
You could always add a comma and then replace using result.replace("[,", "[");
This way you save a few characters in the replace call, but your flash string is increased with a few more commas.

[TD-er]

Just a tip for future pull requests..
Add Fixes: #4944 in the description of the PR and the issue will be closed when I merge the PR. (thus mention the issue nr)