Added DPT-3s and corresponding sensor types (second try) #167
Conversation
|
Thank you very much @buergi , that's what was missing in xknx for me. |
|
@nodefeet could you please explain your usecase for DPT-3 in the Light module? My only usage of xknx is through home-assistant - which has no concept of holding down buttons / switches. Are you using xknx outside of HA? |
|
Yes and it's really just that for that: dimming directly with KNX switches outside of the HA interface. A lot of visualizations for smart homes offer the relative dimming, but I would agree that as soon as you have a virtual slider or knob it’s not really necessary. But with KNX compatible switches it is very useful to dim the light directly from your physical switch (the fastest app is still no app). From my experience the stepwise dimming is rarely used in practice, partly due to that it is swiftly flooding the bus with excess telegrams, but, the start stop dimming is imho part of any basic KNX installation with lights. After the brightness is modified via relative dimming (i. e. after receiving the stop telegram or reaching 0 or 100 % brightness ) KNX dimming actuators typically return the actual brightness to the bus (so that interfaces like HA can update their status). Let me know if I can provide any additional input. |
|
@farmio I see now that you were specifically asking about the use case of DPT-3 in the class Light and not just about the use case of DPT-3 in general. So let me add that since I think DPT-3 or relative dimming is essential for an abstraction of a KNX Light I would expect to not just find the |
|
Adding a stepwise dimming GA to the Light class would be no big deal. As far as im concerned it is a stateless action so we wouldn't even need a state GA or a |
|
Which bitshifting do you mean? The few lines inside the to_knx method? After some pondering I couldn't make up any better representation that shows the origin of the values better of course I could encapsulate the bitshifting like this, if you prefer this, but I wouldn't consider this to increase the readability |
There was a problem hiding this comment.
Hi @buergi sorry for my late reply. I hope I am not too picky.
xknx/devices/remote_value_dpt3.py
Outdated
| sign = 0 if value < 0 else 1 | ||
| if self.invert: | ||
| sign = 1 if sign == 0 else 0 | ||
|
|
There was a problem hiding this comment.
I would add a local function within to_knx():
def get_knx_value(value):
""""Returns the knx value according to the theshold defined on of the XXX-knx standard."""
if abs(value) >= 100:
return 1
elif abs(value) >= 50:
return 2
elif abs(value) >= 25:
return 3
elif abs(value) >= 12:
return 4
elif abs(value) >= 6:
return 5
elif abs(value) >= 3:
return 6
elif abs(value) >= 1:
return 7
return 0 # <--- not sure about this, that should be the behaviour from line 53: ret = DPTBinary(0)
def format_knx_bitmap(sign, value):
return sign << 3 | value & 7
knx_value=get_knx_value(value)
return DPTBinary(format_knx_bitmap(sign, knx_value)
What do you think?
xknx/devices/remote_value_dpt3.py
Outdated
| def from_knx(self, payload): | ||
| """Convert current payload to value.""" | ||
| if payload.value & ~0x0F != 0: # more than 4-bit | ||
| pass # raises exception below |
There was a problem hiding this comment.
We should raise the Exception here.
xknx/devices/remote_value_dpt3.py
Outdated
|
|
||
| def from_knx(self, payload): | ||
| """Convert current payload to value.""" | ||
| if payload.value & ~0x0F != 0: # more than 4-bit |
There was a problem hiding this comment.
why not either if payload.value > 15: or if payload.value >> 4: ?
xknx/devices/remote_value_dpt3.py
Outdated
| if payload.value & ~0x0F != 0: # more than 4-bit | ||
| raise CouldNotParseTelegram("payload invalid", payload=payload, device_name=self.device_name) | ||
| # calculated using floor(100/2^((value&0x07)-1)) | ||
| value = [0, -100, -50, -25, -12, -6, -3, -1, 0, 100, 50, 25, 12, 6, 3, 1][payload.value & 0x0F] |
There was a problem hiding this comment.
I personally find this hard to read :) why not doing something like
THRESHOLDS = [0, 100, 50, 25, 12, 6, 3, 1]
invert = self.invert ^ (payload.value & 0x8) > 0
value = threshold_array[payload.value & 0x07]
return -value if invert else value
But well, not sure if this makes it better.
xknx/devices/remote_value_dpt3.py
Outdated
| def from_knx(self, payload): | ||
| """Convert current payload to value.""" | ||
| if payload.value & ~0x0F != 0: # more than 4-bit | ||
| pass # raises exception below |
There was a problem hiding this comment.
We should raise the Exception here.
xknx/devices/remote_value_dpt3.py
Outdated
|
|
||
| def from_knx(self, payload): | ||
| """Convert current payload to value.""" | ||
| if payload.value & ~0x0F != 0: # more than 4-bit |
There was a problem hiding this comment.
why not either if payload.value > 15: or if payload.value >> 4: ?
Would at least make it more clear that you are testing for the 4 right bits.
xknx/devices/remote_value_dpt3.py
Outdated
| """Convert current payload to value.""" | ||
| if payload.value & ~0x0F != 0: # more than 4-bit | ||
| pass # raises exception below | ||
| elif payload.value & 0x07 == 0: |
There was a problem hiding this comment.
Just that i understand this right, this evaluates for value=8 and value=0 ...?
(I'm not too good in bitwise calculations, bit i think we should be more explicit here:
if payload.value in [0, 8]: if this is intended.
xknx/devices/remote_value_dpt3.py
Outdated
| pass # raises exception below | ||
| elif payload.value & 0x07 == 0: | ||
| return self.Direction.STOP | ||
| elif payload.value & 0x08 == 0: |
|
Integrated your most if not all of your remarks and ported the remote value to a new dpt class. What do you think? |
|
Thank you for updating it. This is very much appreciated. I think remote_value_sensor should be used for numeric datatypes only. I would add a new class (maybe "remote_value_control") for this - and maybe DPT2 in the future. This would also enable us to pass tuples holding control and step values so we can use them too. |
|
I see your point, having a sensor for a control state is a bit weird, especially as you cannot set the value for controlling it. As shortly explained in the docstring, there are two control types stepwise dimming and start/stop dimming. The frontend for the start/stop could probably look similar to the cover control. For the stepwise dimming it would be similar, plus evtl. another slider to allow setting the step size. For easy use of the control without any further automatisation the control's state would probably include an internally accumulated absolute value of the dimming/cover state. |
|
You're right, the remote_value class is really not a big deal - its mostly boilerplate. I like xknx classes to be explicit. HA is another issue - we have to adapt to the available platforms. We could
In ExposeSensor we have something similar: xknx/xknx/devices/expose_sensor.py Line 33 in 213c6fc For stepwise dimming it would also be an option to make the step size configurable via yaml. Just like KNX switches where the stepsize is set once via ETS. It could always be bypassed by eg. HA-automations with an optional argument. |
|
I made similar work and only now saw that there exists this still unmerged PR. |
|
Thanks @recMartin for your implementation. I looked over the code and like the idea of having DPTControl implemented as "control + stepcode". It is closer to the KNX standard as my "increment" implementation and especially as it is bijective (invertable). However, from a user perspective I'd expect to work with increment values, e.g. make the light 25% brigher. I'm currently consider moving the increment conversion to a corresponding RemoteValue type and stripping the DPT implementation down to the control+stepcode approach. As @recMartin said he won't do any changes on his PR at the moment I'd suggest I keep working in this PR and try to merge both PRs. |
|
I had a look in the code, its a different approach than mine (#262), but also good work!
|
|
Thanks @recMartin for your input. I've been working on the code yesterday and a lot has changed. I've not pushed it yet as I have some open points I still have to ponder about. Especially as I already have a future control device implementation in mind when developing. I currently implemented a base DPTControl class based mostly on your code with a few esthetical changes. From this I derived a DPTControlStepwise which is mostly my code. I renamed the DPTControlDimming/Blinds to DPTControlStartStopDimming/Blinds (which I actually already had in the original code of this PR). I still like to add an intermediate DPTControlStartStop class to reduce the amount of duplicated code, but I'm not done with it yet I already added unit tests for all important cases I could think of. I currently don't plan to add a new Control class in this PR, but I have some ideas and will make another PR when I'm ready. In short I plan to make a Control with an integrated absolute counter which can be configured to stepwise or startstop mode. The first case is simple and the absolute value is changed directly when a step-message is received. For the second case I have an implementation in mind which is similar to the one of the cover device, which uses a method which is added as a job to home assistant (see cover.py) |
|
I again rebased the PR and updated the DPT mapping in the RemoteValueControl to match that of RemoteValueSensor. There is a slight problem with the Currently, this value class cannot be used from within HA. I still plan to add a new kind of device which has an internal value that can be controlled with this type of messages. However, in the meantime, and also for flexibility reasons I would suggest extending the RemoteValueSensor type to allow DPTBinary payloads just by adding |
|
Hi! All I can say right now is that I'd not be in favour of adding |
xknx/dpt/dpt_1byte_control.py
Outdated
| @classmethod | ||
| def _decode(cls, value): | ||
| """Decode value into control-bit and step-code.""" | ||
| control = 1 if (value & cls.APCI_CONTROLMASK) != 0 else 0 |
There was a problem hiding this comment.
| control = 1 if (value & cls.APCI_CONTROLMASK) != 0 else 0 | |
| control = (value & cls.APCI_CONTROLMASK) >> 3 |
I think this would be easier to read here.
There was a problem hiding this comment.
Would be possible, however, I don't like it a lot as it is not directly obvious what possible values control could get. One first has to look up the value of the mask and count the bits to understand where the 3 is coming from. Especially having in mind the implementation for the DPT2 class we would only need to change the APCI_xxxMASK values to reuse it for a 2-bit value.
xknx/dpt/dpt_1byte_control.py
Outdated
| def _encode(cls, control, step_code): | ||
| """Encode control-bit with step-code.""" | ||
| value = 1 if control > 0 else 0 | ||
| value = (value << 3) | (step_code & cls.APCI_STEPCODEMASK) | ||
| return value |
There was a problem hiding this comment.
| def _encode(cls, control, step_code): | |
| """Encode control-bit with step-code.""" | |
| value = 1 if control > 0 else 0 | |
| value = (value << 3) | (step_code & cls.APCI_STEPCODEMASK) | |
| return value | |
| def _encode(cls, control: bool, step_code: int): | |
| """Encode control-bit with step-code.""" | |
| return (control << 3) | (step_code & cls.APCI_STEPCODEMASK) |
There was a problem hiding this comment.
I actually don't like this at all, as you put in the intrinsic assumption that control is always either 0 or 1. You added the type hints, so the type checker should detect this, but users of the xknx library might not use type checking in their code. Just imagine I would pass control=3.
It would be possible to use use (control & cls.APIC_CONTROLMASK) << 3, but as above this diminishs the abstraction, as the 3 contains redundant information which is already contained in APIC_CONTROLMASK. So I'll add the type hints, but would prefer to keep the rest.
xknx/dpt/dpt_1byte_control.py
Outdated
| if invert: | ||
| control = 0 if control > 0 else 1 | ||
|
|
||
| return {"control": control, "step_code": step_code} |
There was a problem hiding this comment.
| if invert: | |
| control = 0 if control > 0 else 1 | |
| return {"control": control, "step_code": step_code} | |
| return {"control": control, "step_code": step_code ^ invert} |
There was a problem hiding this comment.
I guess you meant control ^ invert. Would be possible, but do you really consider this easier to read? I find it somewhat crypting whereas the explicit version is more obvious. Plus, it would cause a mess if invert is not 0 or 1, which, however, is safe to assume as long as a boolean is passed.
|
Hey! I think this can be merged now, but for some reason this is showing 99 files edited now. There are some commits in the changeset that are already merged a while ago. Can you please clean this up (hopefully) a last time? |
Co-authored-by: Matthias Alphart <farmio@alphart.net>
|
Upps, what did I do there, git keeps on surprising me. I now cherry-picked all my commits again upon the current head. I'm not sure why the coverage test fails. I don't understand the output of this unit test. |
|
Thank you very much! |
|
@buergi thanks for contributing this! I am trying to figure out how to control my dimmers with HA and I found this change. It seems that the DPTControlStartStopDimming should do exactly what I need. I am, however having trouble figuring out how to use this in HA. You are only providing a sensor in this change and it's not obvious to me how this helps with HA integration. If you managed to control your dimmers with HA, I'd really appreciate if you shared how you did it. |
|
@nicolamomchev there is no built in entity type in HA that would properly fit to send these. If you want other integrations entities to action received DPT3 telegrams you could use this blueprint: https://community.home-assistant.io/t/knx-relative-dimming-for-lights/273473 Or try to do it via the |
Added all missing DPT-3 datatypes, DPT 3.007 and 3.008, each in their stepwise and startstop mode, see ABB Applications Manual for details.
I'm using them to control my Hues with my Busch Jäger push button-coupling 6108/02.