Making effect metadata a little bit more consistent with the JSON API

[RadianM]

The JSON API is able to handle the setting of the three color slots with col [array of colors]. Hence these colors are retrieved from saved presets. However, unlike the other key parameters that can also be set in FX metadata, col is not included.

This means that while effects can be preset to start with a particular palette using pal=n, if n is 2 to 5 (i.e. one or more combinations of color slot colors) those actual colors themselves cannot be preset.

Adding something like col=[[RGB1],[RGB2],[RGB3]] to the existing metatdata parser would allow effects to specify defaults for the color slots, and allow effects authors to embed specific spot colors into their effects that, like specific slider positions (for example, using sx, ix metadata), can be changed by the user and saved as overrides in presets.

[blazoncek]

Effect metadata is only used for UI.
It has no influence on API. If you create API calls manually you can always enter all required information including all three color slots.
When using UI, metadata hides UI elements that cannot influence LED output at particular setting. If you want to set i.e. all three color slots select appropriate palette (* Color gradient or * Colors) first.

If you want to embed default colors or palette into effect, you can do so within effect itself using if (SEGENV.call==0) {...}.

[RadianM]

If you want to embed default colors or palette into effect, you can do so within effect itself using if (SEGENV.call==0) {...}

    if (SEGENV.call == 0) {   //initialise FX
 		SEGMENT.setColor(0, MAGENTA);
		SEGMENT.setColor(1, CYAN);
		SEGMENT.setColor(2, YELLOW);
	}

That was the first thing I tried but it doesn't reliably update the color slots. Sometimes they retain their previous values until you start moving a color control. I think there's a race going on somewhere. This is why I thought about the metadata. This always sets FX controls and palette number and, as you say, also sets which slots are visible in the UI. But setting their colors with .setColor() appears to be a little bit flakey.

[RadianM]

So having said that SEGMENT.setColor() doesn't reliably update the color slots in the UI, I'm wondering if it would be better etiquette to raise this as separate issue or continue to discuss it here?

As an experiment, setting the slots at the end of the effect metadat string parser Yeilds the expected result i.e. on loading the effect, the slots in the UI correctly reflect the colors[] array.

      // load default values from effect string
      if (loadDefaults) {
        int16_t sOpt;
        sOpt = extractModeDefaults(fx, "sx");   speed     = (sOpt >= 0) ? sOpt : DEFAULT_SPEED;
        sOpt = extractModeDefaults(fx, "ix");   intensity = (sOpt >= 0) ? sOpt : DEFAULT_INTENSITY;
        sOpt = extractModeDefaults(fx, "c1");   custom1   = (sOpt >= 0) ? sOpt : DEFAULT_C1;
        sOpt = extractModeDefaults(fx, "c2");   custom2   = (sOpt >= 0) ? sOpt : DEFAULT_C2;
        sOpt = extractModeDefaults(fx, "c3");   custom3   = (sOpt >= 0) ? sOpt : DEFAULT_C3;
        sOpt = extractModeDefaults(fx, "o1");   check1    = (sOpt >= 0) ? (bool)sOpt : false;
        sOpt = extractModeDefaults(fx, "o2");   check2    = (sOpt >= 0) ? (bool)sOpt : false;
        sOpt = extractModeDefaults(fx, "o3");   check3    = (sOpt >= 0) ? (bool)sOpt : false;
        sOpt = extractModeDefaults(fx, "m12");  if (sOpt >= 0) map1D2D   = constrain(sOpt, 0, 7);
        sOpt = extractModeDefaults(fx, "si");   if (sOpt >= 0) soundSim  = constrain(sOpt, 0, 1);
        sOpt = extractModeDefaults(fx, "rev");  if (sOpt >= 0) reverse   = (bool)sOpt;
        sOpt = extractModeDefaults(fx, "mi");   if (sOpt >= 0) mirror    = (bool)sOpt; // NOTE: setting this option is a risky business
        sOpt = extractModeDefaults(fx, "rY");   if (sOpt >= 0) reverse_y = (bool)sOpt;
        sOpt = extractModeDefaults(fx, "mY");   if (sOpt >= 0) mirror_y  = (bool)sOpt; // NOTE: setting this option is a risky business
        sOpt = extractModeDefaults(fx, "pal");  if (sOpt >= 0) setPalette(sOpt); //else setPalette(0);

/*experiment to simulate setting color slots at end of metadata parser*/
        colors[0]=ORANGE; colors[1]=PINK; colors[2]=PURPLE;

I still think the original intention of the metadata extends to the setting of these color slots. From the wiki I read early on that the metadata was introduced in WLED 0.14 to hide potentially inoperative controls but ultimately they are available to effects developers to shape the UI to match the functionality of their effects. My suspicion is that because the colors array can't be deserialised into a single 16bit int like all the other parameters, it was left out.

[blazoncek]

It was not "left out". It was not envisioned to be used at all. 😁
If you really want to have this (but will not make it into upstream) experiment with hex notation in color slot slots. I.e. ...;1=FF00FF,2=00FF00,3=FFFFFF;... You will need to add parser into index.js.

[RadianM]

You appear to be talking about the hex input field in the UI? If so, this wouldn't help an effects author to create an effect which makes good use of the three default colors in the slots. Perhaps I'm not making my motivation clear.

As a made-up example, if I was going to create a "traffic light" effect, I might want to base the effect around the three color slots. In the metadata I can set pal=5 and enable the display of the three slots (perhaps even naming them ...;stop,ready,go;...) but I can't consistently have the UI display RED, ORANGE, GREEN in those slots using SEGMENT.setColor in the initialization code for the effect (as noted above).

Yes the effect will work, but the UI can confusingly show different colors to those on display in the actual LEDs (I still can't say exactly under what conditions the UI gets out of step but it generally does - and sometimes completely refuses to reflect the colors in play until the color slots themselves are selected and edited).

To reproduce this simply add:

    if (SEGENV.call == 0) {
 		SEGMENT.setColor(0, RED);
		SEGMENT.setColor(1, ORANGE);
		SEGMENT.setColor(2, GREEN );
    }

...at the top of any effect code and:

    if (SEGENV.call == 0) {
 		SEGMENT.setColor(0, MAGENTA);
		SEGMENT.setColor(1, CYAN);
		SEGMENT.setColor(2, YELLOW);
    }

..to another effect, along with the metadata to enable the three color slots, and then you'll see how the UI remains out of step as you switch between the two effects.

Saving the effects in a preset OTH causes the UI to update properly on loading them. The JSON API takes care of this. As does simulating the parsing of a proposed addition to the effect metadata which I suggest is more appropriate than calling setColor() in the effect initialization.

[blazoncek]

Let me iterate: Metadata affects UI only. Nothing more.
So anything you do, has to be done in UI (index.js), not anywhere else. If you want to stick with metadata that is.

UI will reflect color in color slots when it receives a WS (or other) update from device. Color slots always contain last known values from a currently active/selected/main segment.

I'll leave you to experiment with code. There is a lot of it. 😉

[RadianM]

Metadata affects UI only. Nothing more.

Yes, I fully appreciate that.

Color slots always contain last known values from a currently active/selected/main segment.

Yes, as confirmed by setting color slots in an effect's initialization routine and always seeing those colors on the LEDs.
Not so much for the actual appearance of those colors in the UI representation of their respective slots - which are often very stale.

UI will reflect color in color slots when it receives a WS (or other) update from device.

Yes, setting the colors in the initialization routine (currently the only place an effects author can set them) doesn't result in a successful update so stale values are shown. Early on I tried setting the flag:

stateChanged = true; // send UDP/WS broadcast

...in the effect code directly after calling SEGMENT.setColor() but this resulted in crashes, and is set at the end of the setColor() routine anyway:

bool Segment::setColor(uint8_t slot, uint32_t c) { //returns true if changed
  if (slot >= NUM_COLORS || c == colors[slot]) return false;
  if (!_isRGB && !_hasW) {
    if (slot == 0 && c == BLACK) return false; // on/off segment cannot have primary color black
    if (slot == 1 && c != BLACK) return false; // on/off segment cannot have secondary color non black
  }
  if (fadeTransition) startTransition(strip.getTransition()); // start transition prior to change
  colors[slot] = c;
  stateChanged = true; // send UDP/WS broadcast
  return true;
}

...and we know that that particular instance of setting the flag doesn't result in the UI being updated with the new slot colors for some reason. Where it does cause a proper UI sync is when set after loading a preset, or at the end of parsing the effect metadata, or when interacting with the effect controls (sliders). Again, it smacks of a race condition. If I could work out where, then that would resolve the issue.

But I really don't understand the reluctance to cater for properly synchronized updating of the UI when it's easy to demonstrate the current failings and why the treatment of color slot color data should be any different to any other effect defaults like palette used or, indeed, which of the three color slots are shown.

[RadianM]

Overcoming a hatred for String parsing in C++ the following tacked after the last extractModeDefaults() gets the job done:

        sOpt = extractModeDefaults(fx, "pal");  if (sOpt >= 0) setPalette(sOpt); //else setPalette(0);

//////////////////////////// BEGIN COLOR SLOT DEFAULT SETTING EXPERIMENT
        char metaData[256];                                 //same assumption made in extractModeDefaults()
        strncpy_P(metaData, strip.getModeData(fx), sizeof(metaData)/sizeof(char)-1);
        metaData[sizeof(metaData)/sizeof(char)-1] = '\0';   // terminate string
        if (metaData[0] != 0) {
          char* uiControls = strrchr(metaData, ';');        // last ";" in FX metadata is start of controls
          if (uiControls){
            char* colHexPtr = strstr(uiControls, "col=[");  //string starting from "col" if present
            if (colHexPtr){                                 //must match "col=["
              colHexPtr += 5;                               // skip "col=["
              char *end;
              int index;
              end = strchr(colHexPtr, ']');                 // next "]" marks end of hex values
              if(end){
                index = (int)(end - colHexPtr);             //offset to "]" from start of hex string
                colHexPtr[index] = 0;                       //terminate hex array group
                for(int i = 0; i < 3; i++){                 //iterate thru up to 3 color slots
                  end = strchr(colHexPtr, ',');             //start of next "," if present
                  if(end){
                    index = (int)(end - colHexPtr);         //offset to "," from start of param string
                    colHexPtr[index] = 0;                   //terminate hex
                  }else index = 0;                          //last hex before ']'
                  uint8_t rgbw[] = {0, 0, 0, 0};
                  if(colorFromHexString(rgbw,colHexPtr))    // will catch non-string entires
                    colors[i] = RGBW32(rgbw[0], rgbw[1], rgbw[2], rgbw[3]);
                  if(!index) break;                         //maybe finished if less than 3 hex values
                  colHexPtr += (index + 1);                 //point to next hex
                }
              }
            }
          }
        }
//////////////////////////  END OF COLOR SLOT SETTING EXPERIMENT

      }
      markForReset();
      stateChanged = true; // send UDP/WS broadcast

The format for including color slot in the metadata using the above is as ugly and simple as I could make it e.g.

• col=[012345,6789AB,CDEF01]

Obviously it could be a lot more sophisticated but at least it's economical in PROGMEM

I only present it as POC and will include it in any effects code I develop. To summarize, it allows an effects author to specify the contents of the three color slots and guarantee that they are correctly shown in the UI as well as being used in the effect. Without something along these lines, or a bugfix elsewhere, the colors may be set and show on the LEDs but won't necessarily be displayed correctly in the UI.

[blazoncek]

Good. But I do not like duplicating code so I propose you enhace extractModeDefaults() and include hex parsing in it.
It may require changing metadata into something like "s1=RRGGBBWW,s2=RRGGBBWW,s3=RRGGBBWW" instead of array.
I would approve that.
When done, make a PR please.

Oh, and BTW. Code lacks spaces. 😉

[RadianM]

I propose you enhace extractModeDefaults()

extractModeDefaults() is currently returning a signed int16_t and the sign is tested for failure in each call from Segment::setMode() so that would seem to rule out returning arbitrary 32-bit color values. If it's not to be done in the segment class then it has to be done by passing a pointer to a color value and copying it to colors[] on a successful return.
I do like your idea of breaking the array out into three separate slot ID's though.

[blazoncek]

extractModeDefaults() is currently returning a signed int16_t and the sign is tested for failure in each call from Segment::setMode() so that would seem to rule out returning arbitrary 32-bit color values

I'm sure you can be resourceful. 😉 IMO none of the current fields take 0xFFFFFFFF as a valid input

[RadianM]

IMO none of the current fields take 0xFFFFFFFF as a valid input

Are you hinting that extractModeDefaults() might be changed to return a signed 32-bit int?

[blazoncek]

Not signed, but yes, 32-bit instead of 16.
You can (almost) always treat 0xFFFFFFFF as an invalid/not used result (and is technically the same as -1).