search

brutella / hkknx-public

hkknx is a HomeKit KNX bridge for KNX.
https://hochgatterer.me/hkknx
97 stars 6 forks source link

Turning on lights with dimmed value causes brief flashing #200

Closed mirkolenz closed 1 year ago

mirkolenz commented 1 year ago

Hello, I just discovered a bug that is related to some special configuration of my KNX system. I have multiple DALI lights and set them up to turn on with a brightness value of 100 % (irrespective of their prior dimming state). Turning them on/off works fine both with my KNX switches as well as HKKNX. However, when turning the lights on with a specific brightness (e.g., 5 %), HKKNX behaves like this:

  1. Send "turn on" telegram
  2. Send the dimming value

Contrary to that, my switch only executes the second step. This difference has the consequence that trying to turn on a light in HomeKit during night causes a very short but also very bright "flash" of the lamp.

My request would thus be to remove the first telegram that turns on the light before sending the actual brightness value.

brutella commented 1 year ago

The telegram to turn on the light corresponds directly to the power characteristic. If HomeKit sets the power characteristic to On, hkknx will send a telegram with the value 1 to the corresponding group address.

The Apple Home app always sets the power to On when the brightness of a light is changed from 0% to any value. That is because setting the brightness to a specific value might not turn on the light automatically.

mirkolenz commented 1 year ago

Thank you for the explanation! Would it be possible to implement some kind of workaround? For example, adding a checkbox "Do not turn on when changing brightness"?

At first I thought of making the switch on group address optional, but that would create a bunch of other issues.

I assume that others will be affected by this behavior as well since an initial brightness value is quite common in DALI and KNX actuators.

remuslazar commented 1 year ago

This issue is very annoying, indeed! IMO a bridge should do this kind of things and not just map the functions 1:1 from the one world to the other.

In KNX there is basically illegal to send a brightness value and the then turn on the switch, having parameters like "initial brightness" which do not make much sense then.

@brutella, any chance to get this implemented in the short run? For me this is not a workaround but a bugfix..

Graefer commented 1 year ago

All KNX dim actors known to me (Jung, MDT, Gira) do have an „initial dim level“ parameter which cannot switched off. hkknx behavior to first send a „turn on“ and next send the dim level clearly interferes with above. This makes e.g. the use of Homekit scenes useless. Please give us a workaround.

brutella commented 1 year ago

Ok, so there is a summary of the expected behaviour of hkknx.

Is there anything else that I missed here?

Graefer commented 1 year ago

No, as suggested my @mirkolenz hkknx should optionally be forced to omit sending any „on“/„off“ commands and rather to send the dim level only. That is to avoid hkknx to compete with the initial brightness function of the KNX dim actor. May be this option should be individually configurable for each object. I suppose this should do the job.

mirkolenz commented 1 year ago

Hi @brutella! Regarding your questions:

If a light is turned on by HomeKit, any brightness value from HomeKit should be ignored.

Exactly, if turning on a light, the brightness value should not be changed. However, I think this already is the case, isn't it? Maybe I am missing some edge cases, but from my observation a simple click on a light in HomeKit only triggers an on telegram without any brightness information.

If the users turns on a light in HomeKit by dragging the brightness value from 0 to any brightness value, we should delay sending the new brightness value by x seconds? (Where x = 1 second?) The delay gives the KNX dimmer time to send their default brightness and then react to the new brightness.

As @Graefer pointed out, this would not solve the issue, but only delay it by x seconds. I also think that there will not be a solution that works for everyone without such a toggle for every entity. For example my DALI gateway has multiple options for each light. Among others, these include:

So my recommendation would be a toggle to "decouple" HomeKit commands from KNX commands. In this decoupled mode:

Thank you for considering changing the default behavior!

Graefer commented 1 year ago

@mirkolenz: Is there any reason which makes sending on/off necessary rather than sending the dim level only? Homekit scenes are neither tapping nor changing brightness. What should hkknx do? I think sending the dim level should do the job in any case, isn’t it?

brutella commented 1 year ago

Exactly, if turning on a light, the brightness value should not be changed. However, I think this already is the case, isn't it?

Yes, but only if the HomeKit sends a brightness value of 100%. This is case is already handled by hkknx.

So my recommendation would be a toggle to "decouple" HomeKit commands from KNX commands. In this decoupled mode:

  • Tapping on a light entity only sends the on/off binary value.

This is what I suggested in the post above.

  • Changing the brightness only sends the new brightness value.

How would hkknx know that only the brightness value is changed via HomeKit? Once you change the brightness of a light using the Home app (or Siri), also the on command is sent. This means that both commands are sent in the same request. So hkknx doesn't know which command triggered the other one. (Does this make sense?)

Graefer commented 1 year ago

Omit sending on/off at all?

mirkolenz commented 1 year ago

@brutella

This is what I suggested in the post above.

Exactly! I just included it in my description to provide full context.

How would hkknx know that only the brightness value is changed via HomeKit? Once you change the brightness of a light using the Home app (or Siri), also the on command is sent. This means that both commands are sent in the same request. So hkknx doesn't know which command triggered the other one. (Does this make sense?)

I get the problem. Could it be a solution to check whether the on command and the brightness are included in the same request? I have no experience with HomeKit, but besides the use case of turning on a light with a specified brightness, is there any other scenario where both values are changed at the same time? If so, I think it would be a viable workaround.

@Graefer

Is there any reason which makes sending on/off necessary rather than sending the dim level only? Homekit scenes are neither tapping nor changing brightness. What should hkknx do? I think sending the dim level should do the job in any case, isn’t it?

Yes, if a user has configured its KNX actuator to not turn on the light if the brightness value has changed. As I said in my post, my DALI gateway offers the option to change the brightness value without turning on the light. Since such configurations are possible and are fully supported by HKKNX as of now, I think one should not break compatibility in a future release.

brutella commented 1 year ago

[...] is there any other scenario where both values are changed at the same time?

Yes, when you define a scene in HomeKit, where you want the light to be turned on at a specific brightness, hkknx will receive the on and brightness command in the same request.

mirkolenz commented 1 year ago

Yes, when you define a scene in HomeKit, where you want the light to be turned on at a specific brightness, hkknx will receive the on and brightness command in the same request.

Ok, that makes sense. However, I think that even in this case it would be valuable to have the "workaround" we are discussing here as it basically mimics the user turning on a light with a specific brightness. So it would solve the same issues for scenes as well :)

Graefer commented 1 year ago

1) @mirkolenz What is the workaround for scenes? On/off, only? Dim level, only? 2) Workaround 1: hkknx sends the dim level and with a delay on/off. (KNX actor initial brightness parameter needs to be configured to remember the dim level) 3) Workaround 2: hkknx omits sending on/off at all. (KNX actor needs to be configured to turn on by dim level) As hkknx needs to translate full compatibility is not possible.

remuslazar commented 1 year ago

[...] is there any other scenario where both values are changed at the same time?

Yes, when you define a scene in HomeKit, where you want the light to be turned on at a specific brightness, hkknx will receive the on and brightness command in the same request.

sounds good! In this case, send only the brightness value to the KNX device. Basically this is the same logic Home Assistant has had implemented, see https://github.com/home-assistant/core/blob/428a33c00f17830d3e86f5d620a720411d75e408/homeassistant/components/knx/light.py#L283

if (
            not self.is_on
            and brightness is None
            and color_temp is None
            and rgb is None
            and rgbw is None
            and hs_color is None
            and xy_color is None
        ):
            await self._device.set_on()

hkknx could also handle the other cases, when HomeKit turns on a light using a new RGB value of color_temp and so on.

brutella commented 1 year ago

I'm still not sure how sending just the brightness would fix the issue. There are KNX dimmers, which let you change the brightness without the light being turned on. So this wouldn't work.

mirkolenz commented 1 year ago

I'm still not sure how sending just the brightness would fix the issue. There are KNX dimmers, which let you change the brightness without the light being turned on. So this wouldn't work.

Yes, that is correct. Thus my request is to add a toggle to light entities that allow to change this behavior on a per-device basis. It would basically be a different mode if you want to call it this way.

brutella commented 1 year ago

So what if hkknx always sends the brightness value first, and then the on command? Would this work with your dimmer?

remuslazar commented 1 year ago

I'm still not sure how sending just the brightness would fix the issue. There are KNX dimmers, which let you change the brightness without the light being turned on. So this wouldn't work.

IMO only some KNX DALI Gateways behave like this. Which KNX dimmer will not turn on the light after receiving a new brightness value?

mirkolenz commented 1 year ago

This would be yet another mode, because then I could not configure an initial brightness. Or in other words, HKKNX sends the brightness value to the dimmer, but it is not respected as it will default to the initial brightness configured in ETS.

But I would assume that as a default behavior (replacing the current one) this may even be desirable. As of now, the light will turn on to its last configured brightness (which could be 100%) and only after that dim down again. But this is just an assumption, I never tested this scenario.

Nonetheless, I think two different modes would be ideal. I understand that adding config options adds complexity for the user, but as there are so many options in KNX actuators, this may be the way to go here.

mirkolenz commented 1 year ago

IMO only some KNX DALI Gateways behave like this. Which KNX dimmer will not turn on the light after receiving a new brightness value?

I think I also saw this on some newer MDT LED drivers (as a config choice in ets), but I am not completely sure.

remuslazar commented 1 year ago

This would be yet another mode, because then I could not configure an initial brightness. Or in other words, HKKNX sends the brightness value to the dimmer, but it is not respected as it will default to the initial brightness configured in ETS.

I mean hkknx should not send the ON telegram to KNX if HomeKit sends a brightness value AND a "power on" command in ONE request. After the user just switches on a light in HomeKit, hkknx should send a power on command, this will then use the initial brightness value configured.

mirkolenz commented 1 year ago

I mean hkknx should not send the ON telegram to KNX if HomeKit sends a brightness value AND a "power on" command in ONE request. After the user just switches on a light in HomeKit, hkknx should send a power on command, this will then use the initial brightness value configured.

Yes, this is precisely the mode that I would like to request. However, as @brutella pointed out, this may not be desirable for some lights.

remuslazar commented 1 year ago

I mean hkknx should not send the ON telegram to KNX if HomeKit sends a brightness value AND a "power on" command in ONE request. After the user just switches on a light in HomeKit, hkknx should send a power on command, this will then use the initial brightness value configured.

Yes, this is precisely the mode that I would like to request. However, as @brutella pointed out, this may not be desirable for some lights.

HomeAssistant does implement just that (not configurable per device) and everything just works nicely: turning on the light via HomeKit will use the configured brightness value and using scenes works also as expected, HA will then just send the brightness value with the ON telegram being suppressed.

What could be a valid use case to change the brightness without turning on the specific device? Does anybody want to do that in KNX?

mirkolenz commented 1 year ago

What could be a valid use case to change the brightness without turning on the specific device? Does anybody want to do that in KNX?

Good question, I can think of none. It just is the case that some actuators allow you to do that. But I completely agree that I would decouple turning on a light from setting the brightness.

Graefer commented 1 year ago

Or in other words, HKKNX sends the brightness value to the dimmer, but it is not respected as it will default to the initial brightness configured in ETS.

Some KNX do have an option that initial brightness is the last dim level. And the latter could be sent by hkknx. However, I think this is too complicate and a rare use case. I would favor the per-device option „do not send on/off when changing brightness“. This would fit for 90% of all use cases and may only require minor configuration changes of KNX actors (if not already default) by the users. And it should be easy to implement for brutella as well.

brutella commented 1 year ago

Why not just send brightness first and then on? This would work in all cases, right?

remuslazar commented 1 year ago

Why not just send brightness first and then on? This would work in all cases, right?

no, I have tested just that using the KNX virtual device and simulator. It does not solve the issue. At least with the MDT dimmer I was using for my tests.

mirkolenz commented 1 year ago

This sadly does not work if an initial brightness is defined:

brutella commented 1 year ago

Got it. I propose the following changes to hkknx.

These changes have the effect that if the user defines a scene in HomeKit to turn on the light with 100% brightness, it doesn't work because hkknx only sends on.

mirkolenz commented 1 year ago

These changes have the effect that if the user defines a scene in HomeKit to turn on the light with 100% brightness, it doesn't work because hkknx only sends on.

Ok, now I also got the problem you are facing. I was not aware of the fact that tapping in Homekit leads to an on command together with 100% brightness.

This would basically mean that turning on a light with 100% brightness uses the default brightness of the light, am I right?

brutella commented 1 year ago

Yes

Graefer commented 1 year ago

Why not just send brightness first and then on? This would work in all cases, right?

no, I have tested just that using the KNX virtual device and simulator. It does not solve the issue. At least with the MDT dimmer I was using for my tests.

This is not correct. MDT actors have the initial brightness option „memory“ which, remembers the last dim level.

remuslazar commented 1 year ago

Why not just send brightness first and then on? This would work in all cases, right?

no, I have tested just that using the KNX virtual device and simulator. It does not solve the issue. At least with the MDT dimmer I was using for my tests.

This is not correct. MDT actors have the initial brightness option „memory“ which, remembers the last dim level.

I was not using this "memory" option, having the initial brightness set to 100% (I think this being the more common use case in the knx world).

What I have tested so far:

  1. Use selects a scene via HomeKit, the scene wants to set a brightness level to lets say 20%
  2. send an ON and after the on telegram the brightness telegram
  3. the MDT knx Dimmer will turn on the light and set the brightness to 100%, ignoring the second brightness telegram

I have also tested that using the KNX virtual device in ETS which also ignores the brightness telegram if this will be send just after the on request if there is no delay between the 2 requests.

Graefer commented 1 year ago

What if hkknx never sends on/off but the dim level only?

remuslazar commented 1 year ago

What if hkknx never sends on/off but the dim level only?

this will trigger the correct behaviour (turning on the light with the specified brigntness)

hkknx should send the on telegram only if HomeKit does not also specify a brightness value in the request. Basically this is also what HomeAssistant is doing from the very beginning. No complaints there so far...

I mean, you want hkknx to send the on telegram if the user just flips the switch in HomeKit. This will then use the configured initial brightness of the dimmer (which could also be the last brightness value)

brutella commented 1 year ago

hkknx should send the on telegram only if HomeKit does not also specify a brightness value in the request.

The only problem is that there is no case where only the on command is sent by HomeKit. Whenever you turn on the light (by asking Siri or tapping the tile in the Home App), HomeKit always includes the brightness value. This means that default brightness values from KNX dimmers (ex. dimmed at night) are always overridden by HomeKit.

mirkolenz commented 1 year ago

In my opinion, it should then even be best practice to not send the value 100% as it will always override what is configured in ETS. With the change we are discussing here, the light should behave much more "naturally". Also thank you for the insights in how HomeKit works internally!

brutella commented 1 year ago

In my opinion, it should then even be best practice to not send the value 100%

This is what hkknx already does since version 2.2.0 → #161

mirkolenz commented 1 year ago

Good to know, thanks 😄

remuslazar commented 1 year ago

hkknx should send the on telegram only if HomeKit does not also specify a brightness value in the request.

The only problem is that there is no case where only the on command is sent by HomeKit. Whenever you turn on the light (by asking Siri or tapping the tile in the Home App), HomeKit always includes the brightness value. This means that default brightness values from KNX dimmers (ex. dimmed at night) are always overridden by HomeKit.

ok, I was not aware of that! Now I am able to understand your objections, thanks!

I want to summarise it here:

IMO this should be the new default; making this configurable will not hurt, though..

mirkolenz commented 1 year ago

if the brightness from HomeKit is 100%, it should be suppressed: hkknx should only send the ON telegramm.

That would lead to not being able to dim to 100 manually, causing some strange behavior with a default brightness. If I understood it correctly, hkknx only needs special habdling if Homekit send a brightness command together with an on command.

I might be nitpicky here, but I think it is inportant to have a complete summary of the desired behavior :

remuslazar commented 1 year ago

if the brightness from HomeKit is 100%, it should be suppressed: hkknx should only send the ON telegramm.

That would lead to not being able to dim to 100 manually, causing some strange behavior with a default brightness. If I understood it correctly, hkknx only needs special habdling if Homekit send a brightness command together with an on command.

I might be nitpicky here, but I think it is inportant to have a complete summary of the desired behavior :

See what @brutella wrote:

The only problem is that there is no case where only the on command is sent by HomeKit. Whenever you turn on the light (by asking Siri or tapping the tile in the Home App), HomeKit always includes the brightness value.

mirkolenz commented 1 year ago

Yes, I know. As I said, special care is needed if brightness and turn on are sent. But not if only the brightness is changed. Otherwise manual dimming will be quite broken.

brutella commented 1 year ago

That's what I suggested in https://github.com/brutella/hkknx-public/issues/200#issuecomment-1359383423

mirkolenz commented 1 year ago

@brutella Correct, just slightly different wording ☺️

remuslazar commented 1 year ago

Yes, I know. As I said, special care is needed if brightness and turn on are sent. But not if only the brightness is changed. Otherwise manual dimming will be quite broken.

gotcha! Yes, you are right, in all other cases, when just the brightness is being updated via HomeKit, it should also be send "as is" to KNX.

brutella commented 1 year ago

A fix is now available in 2.4.2-b8.

mirkolenz commented 1 year ago

Great! I am on a trip for the next few days, so I will only be able to test the new feature sometime this weekend.

flotzi commented 1 year ago

Works perfect with my MDT dimming actor now since 2.4.2-b8 Thanks brutella