Supereg / homebridge-http-lightbulb

Powerful http lightbulb for Homebridge: https://github.com/homebridge/homebridge
ISC License
23 stars 7 forks source link

Add options to replace placeholders with numeric values #4

Closed lumaxis closed 4 years ago

lumaxis commented 4 years ago

When trying to use the plugin with my Elgato Key Light I ran into issues because the light's API requires the new settings values to be sent as numbers instead of strings.

This adds a numericValue config option to all values that I believe make sense to occur as numbers. Since the Key Light only supports brightness and temperature, I was not able to test hue and saturation myself but the other two work for me.

Happy about suggestions for the API design, I was thinking about e.g. making this a global option instead of per area but opted for maximum flexibility in the end. Also, I'm not particularly happy with the mechanism for switching between string and number but this was the easiest way I could make it work with homebridge-http-base's bodyReplacer function.

Supereg commented 4 years ago

Okay I think I do not quite understand the change and why it is needed πŸ˜… If the option numericValue is set the replacer searches in the body for the pattern "%s" instead of just %s. Why is this needed in the first place? Could I get a glimpse at your configuration to understand what you are trying to achieve? Why not just modify the body pattern in the first place?

lumaxis commented 4 years ago

Hah, sure πŸ™‚It's also entirely possible that I'm just confused and was missing another obvious way for how to do this.

Essentially, what this change is intended to do is replace e.g. "brightness":"%s" in a JSON string with "brightness":100, hence replacing "%s" with 100 instead of just the %s. Does that help?

My homebridge config
"accessories": [
  {
    "accessory": "HTTP-LIGHTBULB",
    "name": "Elgato Key Light",
    "debug": true,
    "onUrl": {
      "url": "http://elgato-key-light-600d.local:9123/elgato/lights",
      "method": "PUT",
      "body": {
        "numberOfLights": 1,
        "lights": [{
          "on": 1,
          "brightness": "%brightness",
          "temperature": "%colorTemperature"
        }]
      }
    },
    "offUrl": {
      "url": "http://elgato-key-light-600d.local:9123/elgato/lights",
      "method": "PUT",
      "body": {
        "numberOfLights": 1,
        "lights": [{
          "on": 0,
          "brightness": "%brightness",
          "temperature": "%colorTemperature"
        }]
      }
    },
    "statusUrl": "http://elgato-key-light-600d.local:9123/elgato/lights",
    "statusPattern": "\"on\":1",
    "brightness": {
      "statusUrl": "http://elgato-key-light-600d.local:9123/elgato/lights",
      "statusPattern": "\"brightness\":([0-9]{1,3})",
      "unit": "percent",
      "numericValue": true,
      "setUrl": {
        "url": "http://elgato-key-light-600d.local:9123/elgato/lights",
        "method": "PUT",
        "body": {
          "numberOfLights": 1,
          "lights": [{
            "on": "%on",
            "brightness": "%s",
            "temperature": "%colorTemperature"
          }]
        }
      }
    },
    "colorTemperature": {
      "statusUrl": "http://elgato-key-light-600d.local:9123/elgato/lights",
      "statusPattern": "\"temperature\":([0-9]{3})",
      "unit": "mired",
      "numericValue": true,
      "minValue": 143,
      "maxValue": 344,
      "setUrl": {
        "url": "http://elgato-key-light-600d.local:9123/elgato/lights",
        "method": "PUT",
        "body": {
          "numberOfLights": 1,
          "lights": [{
            "on": "%on",
            "brightness": "%brightness",
            "temperature": "%s"
          }]
        }
      }
    }
  }
]
lumaxis commented 4 years ago

Oh no, I feel dumb πŸ€¦β€β™‚ Totally overlooked that I could simply use an already stringified JSON object as the "body" value in the config. Changing my config to something like "body": "{\"numberOfLights\": 1, \"lights\": [{ \"brightness\": %s }] }" totally works and makes this PR totally unnecessary. πŸ™‚

Well, still, great learning experience, I know much more about HomeKit now. Thanks for bearing with me on this ✌️

Supereg commented 4 years ago

Ah now I see. I totally forgot about the feature, that json stuff will automatically be stringified πŸ˜… Now I see what the issue is. Although changing the config parameters to a string makes it probably a bit messy, I think it really is the better solution for this. Cause a config option would only be engineered for this one specific case, and still wouldn't solve the problem completely. Also I think there is no good way to properly name such an option πŸ˜…πŸ˜‚ And in general the code will get just again a little more messy. So I would not implement it and still rely on the ability to just specify the body as string.

But thanks for the PR anyways. ☺️

lumaxis commented 4 years ago

Oh yea, definitely agree! I was not a big fan of this change myself in the first place so very happy to make it work with the existing code.

Maybe I'll add an example of using a JSON string to the config docs to make this more discoverable.

Supereg commented 4 years ago

Feel free to submit any improvement to the documentation. Any help to make any explanations more understandable is well appreciated. It's often pretty hard to evaluate if something is good explained or not if you know all the details πŸ˜