sde1000 / python-dali

Library for controlling DALI lighting systems
Other
150 stars 71 forks source link

two improvements for the `led` module #115

Closed jktjkt closed 1 year ago

jktjkt commented 1 year ago
jktjkt commented 1 year ago

For the "OPERATING MODE" commit I'd appreciate some guidance on choosing an approach which plays best with the other parts of the library. I tried to look at the int-based enumerations, but the actual values did not appear to be used anywhere, so in the end I chose a simpler approach.

sde1000 commented 1 year ago

Operating modes: are you confusing commands 239 and 252? Command 239 does not have bit 4 defined, while command 252 does.

The response classes for these are LEDOperatingModesResponse and LEDOperatingModeResponse respectively in the current code — which I agree is a bit ambiguous, but we might be stuck with them for now.

jktjkt commented 1 year ago

Ah, right, indeed. Let me drop that patch, and fix the linter error in the other one.

sde1000 commented 1 year ago

Fast fade time: shouldn't you be multiplying the response by 25 rather than 50 to get the fade time in ms?

sde1000 commented 1 year ago

NumericResponse subclass implementation note: I'd access the value attribute to get the value, and be prepared to deal with the case where value is a string owing to missing response or framing error.

jktjkt commented 1 year ago

Fast fade time: shouldn't you be multiplying the response by 25 rather than 50 to get the fade time in ms?

Indeed, sorry for that. My earlier, unpushed, approach with enums named ms_0, ms_25, ms_50 etc was correct in this regard, but very verbose, and I apparently screwed up during refactoring.

NumericResponse subclass implementation note: I'd access the value attribute to get the value, and be prepared to deal with the case where value is a string owing to missing response or framing error.

OK, that makes sense. I'll push an update later once I'm back to my testing lab, I mean, home, and have a chance to verify how it works. Thanks for the quick review!

jktjkt commented 1 year ago

Done. Tested on OTi DALI 80/220-240/24 4CHDT6/8 (for valid responses) and on Lunatone Jalousie relay (for a missing response).

jktjkt commented 1 year ago

A gentle ping.