enwi / hueplusplus

A simple C++ library to control Philips Hue lights on Linux, MacOS, Windows, Espressif ESP32 SDK and Arduino. Full documentation at
https://enwi.github.io/hueplusplus/
GNU Lesser General Public License v3.0
55 stars 22 forks source link

Dynamic light types #44

Closed Jojo-1000 closed 4 years ago

Jojo-1000 commented 4 years ago

Uses the type and the capabilities field to find out which type a light has. This way, no hardware models need to be added manually. The old types have been left in for compatibility. @sieren, @herbrechtsmeier could you verify that this works with all light types you have access to?

codecov[bot] commented 4 years ago

Codecov Report

Merging #44 into development will increase coverage by 0.38%. The diff coverage is 99.44%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development      #44      +/-   ##
===============================================
+ Coverage        78.01%   78.39%   +0.38%     
===============================================
  Files               55       57       +2     
  Lines             4534     4610      +76     
===============================================
+ Hits              3537     3614      +77     
+ Misses             997      996       -1     
Impacted Files Coverage Δ
include/hueplusplus/Group.h 50.00% <ø> (ø)
include/hueplusplus/Hue.h 100.00% <ø> (ø)
include/hueplusplus/HueLight.h 0.00% <ø> (-50.00%) :arrow_down:
test/test_Group.cpp 95.42% <80.00%> (+0.09%) :arrow_up:
include/hueplusplus/HueDeviceTypes.h 100.00% <100.00%> (ø)
src/Hue.cpp 75.98% <100.00%> (+0.98%) :arrow_up:
src/HueDeviceTypes.cpp 100.00% <100.00%> (ø)
test/test_Hue.cpp 98.98% <100.00%> (-0.17%) :arrow_down:
test/test_HueLight.cpp 99.39% <100.00%> (ø)
test/test_HueLightFactory.cpp 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a2a4a81...80cbc17. Read the comment docs.

herbrechtsmeier commented 4 years ago

Why you use the Factory method pattern``? We only have a Base class and no derived class. Why you don't move the configuration of the class into the constructor? TheFactory method pattern` would be match if the project would use a derived class per light device class. In this case the factory would return a base class and the user have to up cast it to the correct class depending on the type of the object. But the project use a super class with the methods of all light classes.

Jojo-1000 commented 4 years ago

Why you don't move the configuration of the class into the constructor?

That makes creating a light more complicated, because the strategies, HueCommandAPI and refresh duration would have to be passed in with every call. This way, they are stored in the factory and do not have to be specified again.

If you find the name Factory confusing, because it implies different sub classes, can you suggest a better name? I personally don't see much of a problem there.

herbrechtsmeier commented 4 years ago

If you find the name Factory confusing, because it implies different sub classes, can you suggest a better name? I personally don't see much of a problem there.

Maybe LightManager, LightStore or Lights to move the lights resource handling from the Hue class into this class later.

herbrechtsmeier commented 4 years ago

@Jojo-1000 Why you merge this pull request? It removes the support for the on/off plug-in unit.

Jojo-1000 commented 4 years ago

@herbrechtsmeier I believed that would be covered under the type on/off light. The API documentation did not mention any other types. Is on/off plug-in unit the type entry for it?

Jojo-1000 commented 4 years ago

OK, I found the ZigBee specs, which also has on/off plug-in unit as well as dimmable plug-in unit. I will add these

herbrechtsmeier commented 4 years ago

Thanks