Add sensor support

[herbrechtsmeier]

Add support for Hue sensors. Move common parts from HueLight class to a new HueThing class and add a new HueSensor class.

Questions

• Why are all functions are virtual?

ToDo

• Move unit tests of HueThing
• Add unit tests for HueSensor

[lgtm-com[bot]]

This pull request introduces 1 alert when merging a99e61ba296ee36467c2fd0a434d2d4590a475c9 into 07df3e273fdb0c0a8a3405e0c59fb28b5a4e78ae - view on LGTM.com

new alerts:

• 1 for Non-virtual destructor in base class

[Jojo-1000]

Great work! Our goal is to support most Hue API functions in the future.

I am currently working on adding group support (branch group-api), which contains some changes that also affect HueLight.

• Why are all functions are virtual?

They are virtual for the test mock classes.

[sieren]

That's pretty cool! Not sure how deep you'd plan on going with this, but I suppose it's worth taking over the supported devices from the deCONZ project (which covers pretty much all Zigbee smart home appliances that work with Hue) ( https://github.com/dresden-elektronik/deconz-rest-plugin/blob/master/de_web_plugin.cpp L106+)

I might look into that at some point

[herbrechtsmeier]

@Jojo-1000 Isn't it strange to adapt the source code to a tests?

[herbrechtsmeier]

@sieren Are you sure that all this devices are supported by the Hue bridge? Doesn't the hue bridge only support ZigBee Light Link?

[herbrechtsmeier]

Should I rename HueThing into HueDevice because this term is already used?

[sieren]

@herbrechtsmeier Youre right, I think thats where they diverge a little. DeCONZ certainly supports a lot more devices than the Hue bridge (on a Zigbee level), but the way they are exposed through the REST API is consistent with the Hue Bridge.

I'm still interested in adding those to Hueplusplus since deCONZ is a much cheaper and way better zigbee hub than the Philipps/Signify Bridge.

[enwi]

Should I rename HueThing into HueDevice because this term is already used?

Regarding my requested change this would make sense and then you could also add the 'Device' types to HueDeviceTypes.cpp

Or do we want to draw a line between light and sensor or other devices?

[herbrechtsmeier]

@sieren The deCONZ REST API also supports Websocket for live updates.

[herbrechtsmeier]

Should I rename HueThing into HueDevice because this term is already used?

Regarding my requested change this would make sense and then you could also add the 'Device' types to HueDeviceTypes.cpp

Should I add a MakeHueSensor::operator()?

I wonder if we should add a HueSensors class instead and rework the data holding. At the moment the light resources are buffered in the Hue and HueLight class and differently updated. I would move the data holding into the HueSensor class. The HueSensors class can forward its information to the HueSensor class and the Hue class to the HueSensors class. Especially for the sensors it makes sense to update all sensors together because its maybe faster.

[enwi]

@herbrechtsmeier @Jojo-1000 introduced some changes yesterday, which have already been rebased into the development branch, did you take a look at these?

[herbrechtsmeier]

@enwi I have take a look at the changes. Maybe the groups handling could move into its own class to separate it from the hue class.

Why is the class called Group and not HueGroup? Should I remove the Hue from the Sensor class?

Why you cache the requests and not the system state? A request to /api/<apikey>/, /api/<apikey>/lights and /api/<apikey>/lights/<id> share the same state of a light.

[Jojo-1000]

@herbrechtsmeier The Hue in HueLight is from the time when we had no namespace. I found it redundant to have hueplusplus::HueGroup instead of hueplusplus::Group.

I am not sure if sharing so much state between the classes is a good idea. It makes it difficult to see what is going on from an outside perspective, and it prevents use in a multithreaded environment. I would maybe even remove the map of cached HueLights from Hue and instead have the user handle that if they want to.

Regarding the tests, feel free to rewrite them without mocks, but I believe that will make them less maintainable.

[herbrechtsmeier]

@herbrechtsmeier The Hue in HueLight is from the time when we had no namespace. I found it redundant to have hueplusplus::HueGroup instead of hueplusplus::Group.

Should I remove the Hue from HueSensor?

I am not sure if sharing so much state between the classes is a good idea. It makes it difficult to see what is going on from an outside perspective, and it prevents use in a multithreaded environment.

Agree

I would maybe even remove the map of cached HueLights from Hue and instead have the user handle that if they want to.

Does this means getAllLights() would request /api/<apikey>/lights and creates a vector of objects on the fly and getLight() creates a new object?

[Jojo-1000]

Does this means getAllLights() would request /api/<apikey>/lights and creates a vector of objects on the fly and getLight() creates a new object?

Yes, or you could use the current state of the bridge to create the new objects.

The way it currently is, there should only be one instance of every light, but the user can just do HueLight light = bridge.getLight(1) and you have an unnecessary copy sitting in the bridge cache.

[enwi]

Should I remove the Hue from HueSensor?

Also to not let your question be unanswered, I would say yes

[Jojo-1000]

@herbrechtsmeier I am now at the point where the sensor and rule apis are pretty much the only missing pieces, so I could start working on sensor support. Would you like to continue working on this issue yourself, or should I take over here?

[herbrechtsmeier]

@Jojo-1000 Please take over. The HUE sensors and especially the CLIP sensors are much more heterogeneous as expected.