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

Fix compatability with Sylvania tunable white bulbs #71

Merged
merged 3 commits into from Apr 4, 2017

Conversation

Projects
None yet
2 participants
@emunsing

emunsing commented Mar 31, 2017

It seems that Osram/Sylvania Lightify tunable white bulbs report their status as float strings, which led to a typecasting issue when casting directly to int. This has now been tested to work with the Sylvania Lightify A19 Tunable White 60W equivalent bulb.

@emunsing

This comment has been minimized.

Owner

emunsing commented on 6086779 Mar 31, 2017

It seems that Osram/Sylvania Lightify tunable white bulbs report their status as float strings, which led to a typecasting issue when casting directly to int. This has now been tested to work with the Sylvania Lightify A19 Tunable White 60W equivalent bulb.

@pavoni

Thanks - looks good - couple of minor suggestions, then happy to merge and release.

Assuming you're using this with HA - would you like to do the PR - or should I?

@@ -105,7 +105,7 @@ def _update_state(self, status):
if not value:
value = None
elif ':' in value:
value = tuple(int(v) for v in value.split(':'))
value = tuple(int(float(v)) for v in value.split(':'))

This comment has been minimized.

@pavoni

pavoni Apr 2, 2017

Owner

Probably best to do int(round(float(v))) which will round rather than truncate - and work on Python 2 & 3.

@@ -105,7 +105,7 @@ def _update_state(self, status):
if not value:
value = None
elif ':' in value:
value = tuple(int(v) for v in value.split(':'))
value = tuple(int(float(v)) for v in value.split(':'))
else:
value = int(value)

This comment has been minimized.

@pavoni

pavoni Apr 2, 2017

Owner

Could you also do int(round(float(v))) . Probably not needed for your devices - but could help with some other device!

This comment has been minimized.

@pavoni

pavoni Apr 4, 2017

Owner

@emunsing Don't know if you saw this - could you add the same code in line 110?

@emunsing

This comment has been minimized.

emunsing commented Apr 4, 2017

Great, good catch- I like the update, and have pushed a new commit that addresses this. Let me know if there's anything else you catch! I'm new to pull requests, so let me know what's needed to wrap this up :)

@pavoni

This comment has been minimized.

Owner

pavoni commented Apr 4, 2017

@emunsing One small suggestion that I think you missed (doing the same cast in the else).

Then I'll to merge and release. Thanks for the contribution.

@emunsing

This comment has been minimized.

emunsing commented Apr 4, 2017

Great, thanks for the catch- updated!

@pavoni

pavoni approved these changes Apr 4, 2017

@pavoni pavoni merged commit 7d385e4 into pavoni:master Apr 4, 2017

@pavoni

This comment has been minimized.

Owner

pavoni commented Apr 4, 2017

Released as 0.4.16. Thanks.

Would you like to do the HA pull request - or shalI?

@emunsing

This comment has been minimized.

emunsing commented Apr 4, 2017

I'm not actually working with the the Home Assistant system, if that's what you're referring to by HA- go ahead and do the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment