Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the kelvin handling. #18

Open
mabels opened this issue Oct 4, 2019 · 9 comments
Open

Refactor the kelvin handling. #18

mabels opened this issue Oct 4, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@mabels
Copy link
Contributor

mabels commented Oct 4, 2019

The kelvin validation should make use of the configuration of the device.

@mabels
Copy link
Contributor Author

mabels commented Oct 4, 2019

There is a small opstacle. The discovered devices does not contain any device type parameters today. They have to recovered manualy.
I would propose to fix the kelvin validation by first extend the device discovery to provide the type of the device implicit.
Is there anything which is known not to do so?

@Sawtaytoes
Copy link
Collaborator

I like that idea. To confirm, are you saying the device discovery process, which returns a "light" should also statically return the type of light on the main object?

A little confused about your question. Are you asking if other functions don't have a device type?

@ristomatti
Copy link
Collaborator

@mabels I've known for a long time this ought to be done. The kelvin values (+ other info) can be found in src/lifx/products.json. The file is maintained by LIFX and can be found here github.com/LIFX/products/blob/master/products.json. I'm not sure if the current version is up-to-date but it seems to have Tiles at least so it might be the latest.

@ristomatti
Copy link
Collaborator

I read too fast. So the issue was that the current discovery process does not do the request that would give the light type? Extending the process to do this would then be a good idea. Although I have a faint memory that at some point there was an issue that some lights didn't respond to the light.getHardwareVersion request - so it might need some experimentation first. Possibly it might be a good idea to do this after the devices have been discovered to not make the process any slower. It's already somewhat random for me at least.

@Shakesbeard
Copy link

Sorry for nosing around ^^
I just came to read this. For my Homey Driver App I actually did this on driver app level as I need the min and max for the frontend slider scaling. When discovering a light I actually set default to 2500 to 9000 and then do a lazy update using getHardwareInfo and if the Kelvin values are present I update them on my light object :D

@ristomatti
Copy link
Collaborator

ristomatti commented Dec 10, 2019

@Shakesbeard This could be a valid approach especially if for example the older lights do not respond to the query (they all had 2500-9000K range). I'm sadly lacking time to do much work for this library at the moment.

I have an idea. If someone's got the time, it would be useful to have a snippet of code that would first discover the lights then do a hardware query for each and in the end print out the result in a compact list. This way it would be easy for all participants in this thread to test it on their setup. With the result we'd get better info if there still are issues with the hardware query on the current firmware and better discuss the strategy in solving this.

I would have lights ranging from the first E27 bulbs to most newer EU market products: E27 IR bulb, newer E27 classic bulbs, E27 Mini Color bulbs, Z Strips, GU10 downlights and a Tile set to test with. Naturally I'm missing all the US models.

@ristomatti
Copy link
Collaborator

BTW, I've been using the 1500-9000K range for all my lights for months now without any issues.

The lights that don't support values between 1500-2499 just pick the closest value on the firmware level and change to that. I think this greatly simplifies implementing custom user interfaces as you can just use the same range for all lights. This seems to be what the LIFX app UI also does when controlling a group of lights with mixed color temp capabilities.

It's easy to think unnecessary packets could be avoided by blocking the requests to an unsupported range but the end user would still arguably want the light to change to at least 2500K if 1500K is requested. As far as I see it, from the library perspective the only use for this improvement would then be to try to map let's say 1500K to 2500K if that is the lowest value the light supports. But would it be just waste if this is already done on the hardware level?

The downside of this would also be additional getHardwareInfo requests flooding the lights which might not be needed by all users. Any thoughts?

@ristomatti ristomatti added the enhancement New feature or request label Dec 12, 2019
@Shakesbeard
Copy link

Hmm.. I only do that until I once succesfully received the hardware info from the device. Then I store the values in my app and never rerequest the info until complete restart.
I guess this could be an optional toggle on the client.init call for people who don't want that.

@Shakesbeard
Copy link

I implemented the hardware info call in my app too and can confirm that sometimes the response ends up to be empty. However, I simply rerequest the info until I got it and only then I flag the device as operational.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants