skydiver / ewelink-api

eWeLink API for JavaScript
https://www.npmjs.com/package/ewelink-api
MIT License
265 stars 109 forks source link

Add custom device power state to allow sending custom params #78

Closed jonathan-reisdorf closed 4 years ago

jonathan-reisdorf commented 4 years ago

:wave: This small change allows to send custom params for advanced use cases - in my case changing the color temperature and brightness of an LED bulb. While injecting that into setDevicePowerState is not perfect, because we are not really changing the power state, it felt wrong also to create a different mixin just for this. Let me know if a test needs to be added.

ttz642 commented 4 years ago

This small change allows to send custom params for advanced use cases - in my case changing the color temperature and brightness of an LED bulb. While injecting that into setDevicePowerState is not perfect, because we are not really changing the power state, it felt wrong also to create a different mixin just for this. Let me know if a test needs to be added.

I would prefer a new mixin updateDevice which is a generic update method allowing all updates and for the setDevicePowerState to eventually use updateDevice. Then more descriptive / specific methods could be easily added eg setDeviceBulb which would set the color temperature and brightness of the LED bulb using updateDevice.

Comments ?

jonathan-reisdorf commented 4 years ago

Adding an updateDevice mixin and using that in setDevicePowerState seems like a viable solution to me. I'm reluctant, though, regarding the specific methods: I have a multi-white bulb here, and I'm not even sure if all other ewelink-supported multi-white bulbs have to be spoken to in the same way (or is that normalized by ewelink?). Then there's multi-color bulbs and bulbs that just can switch on/off. The more methods around specific device groups we add, the more needs to be maintained. Imo adding a generic update mixin is already enough to "unlock" all different sorts of use cases. WDYT?

ttz642 commented 4 years ago

Adding an updateDevice mixin and using that in setDevicePowerState seems like a viable solution to me. I'm reluctant, though, regarding the specific methods: I have a multi-white bulb here, and I'm not even sure if all other ewelink-supported multi-white bulbs have to be spoken to in the same way (or is that normalized by ewelink?). Then there's multi-color bulbs and bulbs that just can switch on/off. The more methods around specific device groups we add, the more needs to be maintained. Imo adding a generic update mixin is already enough to "unlock" all different sorts of use cases. WDYT?

Perhaps a compromise, add the updateDevice mixin and use in setDevicePowerState and only add methods for things that are genuinely simple/universal, eg setDeviceWiFiState (led enabled or not).

It would also be useful if we had a generic getDevice mixin.

Also these generic updateDevice and getDevice mixins should support lan / ZeroConf mode.

jonathan-reisdorf commented 4 years ago

Yes, there are all great ideas. Unfortunately I'm currently lacking time to work on that, so I'll close this PR for now and when I have more time I can reopen it.