steilerDev / homebridge-openhab2-complete

A homebridge plugin for openHAB, that has the expectation to fully support all Services offered by Apple's Homekit Accessory Protocol (HAP)
GNU General Public License v3.0
52 stars 16 forks source link

Maximum brightness is unreachable depending on OH thing type #57

Closed weakfl closed 2 years ago

weakfl commented 4 years ago

If using dimmers with the OH Zigbee binding (and probably other Zigbee based bindings), it is impossible to set a dimmer level of 100.

The Zigbee binding will ignore ON commands if the dimmer state is already ON. This is probably due to the fact, that the binding either restores the last known dimmer level or uses the user defined On Level if it receives an ON command.

In the current implementation, an ON command is send instead of 100 when adjusting the brightness level to maximum:

https://github.com/steilerDev/homebridge-openhab2-complete/blob/1c24051804c40521b92a22c83a618750c5ecf8f5/accessory/characteristic/SetAndCommitLight.js#L89-L101

I was able to work around this issue by adding an additional state check to the relevant if clauses in lines 90 and 96:

if(binary && (brightness === 0 || brightness === 100) && !(service.getCharacteristic(this.Characteristic.On).value)) { // For some reaason when invoking Siri to turn on a light brightness is sometimes set to '0'

However: I don't know the code well enough to be certain that these changes won't impact other functionality or if there is a better way to solve this issue...

grzegorz914 commented 4 years ago

Hi,

Just create rule in OH:

rule "Dimmer"
when
    Item Dimmer received command
then
     if (!(receivedCommand instanceof Number))
        switch receivedCommand {
            case ON :     Dimmer.sendCommand(100)
            case OFF  :  Dimmer.sendCommand(0)
      else 
           if (receivedCommand instanceof Number && (receivedCommand != 0 && receivedCommand != 100))
                     Dimmer.sendCommand(receivedCommand)
end
weakfl commented 4 years ago

Thanks for the suggestion @grzegorz914, but this would also bypass the functionality of the Zigbee binding (restore last dimmer level or use On Level if defined), so I'd rather see a proper fix implemented.

steilerDev commented 4 years ago

I am struggling with the correct implementation of the light accessory. There has been a couple of issues and discussions on the project on how to do it right (See #13 and #43). Unfortunately OpenHAB and the Binding-Dev community does not agree on a unified behavior, therefore this is always depending on your accessories and bindings.

To be honest I can't see a "one-fits-all" solution and considering some kind of "choose-your-behaviour" approach. Any suggestions on how to do this @weakfl @grzegorz914 (Maybe even @torpex77, @extric99 or @jewesta since you were involved in the previous issue)

weakfl commented 4 years ago

Understood @steilerDev :)

I think my proposal should be independent of whatever binding is linked to the dimmer:

It's more that my implementation is more like a proof of concept and there might be a better way to implement this.

I don't think that it has any impact on #13 or #43, but I'd be happy to here from @grzegorz914 or anyone else who knows the codebase better than I do.

grzegorz914 commented 4 years ago

@weakfl I have tested Your proposal and we lose the command(ON) after press the switch in HomeKit app, also if light is OFF. Best to resolve this issue is create the rule in OH with fake item.

If Your light is on and fake_dimmer send command (ON) then dimmer send command (100). This make nothing more as only change the command(ON) to (100) if light is already ON.

The state wchih is right now is optimal for all cases because we have all possible command to use.

Thanks

weakfl commented 4 years ago

@grzegorz914

First of all, thanks for testing!

I have tested Your proposal and we lose the command(ON) after press the switch in HomeKit app

What difference does it make if the item is already ON?

also if light is OFF

How is that possible if the item is indeed OFF? The if clause checks the state of the item and will send an ON command if the state is OFF. I can't reproduce the behaviour you are describing when testing it, but I might have missed something:

Oct 09 11:12:08 pi homebridge[15761]: [10/9/2019, 11:12:08 AM] [openHAB2-Complete] Updating state of Deckenlampe Küche with item KitchenCeilingLight to OFF
Oct 09 11:12:11 pi homebridge[15761]: [10/9/2019, 11:12:11 AM] [openHAB2-Complete] Updating state of Deckenlampe Küche with item KitchenCeilingLight to ON

Best to resolve this issue is create the rule in OH with fake item.

I have to disagree. Check the other UI implementations (PaperUI, iOS App, etc.), none will send an ON command when using a slider to change the brightness level to maximum.

If Your light is on and fake_dimmer send command (ON) then dimmer send command (100). This make nothing more as only change the command(ON) to (100) if light is already ON.

This would certainly work, but the thought of having to use proxy items for all my dimmers and have to keep them in sync makes me shiver.

What might also work is to send both, 100 followed by ON. But that would certainly require more changes...

grzegorz914 commented 4 years ago

Hi,

as I previous described after test Your code the command ON not exist anymore(always send command 100 instead ON), why? I don’t know just is it. The slider work correct and send command from 0 to 100.

I have today make more test with different configurations but without success. Looks like we can not correct compare the state of light.(Characteristic.On)

Optimal will be:

Light is OFF >>> press button >>> send command ON
Light is ON >>> moved slider >>> send command 0 to 100 ( now is 0 to 99 and if 100 send ON)
Light is ON >>> press button >>> send command OFF

I have no idea how to realize , may be @steilerDev help us.

For me this state is OK and I have no any problems with all my dimmers (switch On/Off, set brightness, saved last brightness value) all working correct.

Thanks

weakfl commented 4 years ago

@grzegorz914

as I previous described after test Your code the command ON not exist anymore(always send command 100 instead ON), why? I don’t know just is it.

This is strange, but it is what it is. Again, thanks for testing!

Optimal will be:

Light is OFF >>> press button >>> send command ON
Light is ON >>> moved slider >>> send command 0 to 100 ( now is 0 to 99 and if 100 send ON)
Light is ON >>> press button >>> send command OFF

Agreed, this is exactly what I tried to achieve.

For me this state is OK and I have no any problems with all my dimmers (switch On/Off, set brightness, saved last brightness value) all working correct.

How the ON command will interpreted seems to depend on the binding and even on the device. For example, some Zigbee dimmers will allow you to set an On Level, others won't. Some dimmable bulbs will remember the previous level when mains is turned off/on (eg. Tradfri bulbs), others won't (eg. Hue bulbs).

I have a variety of Zigbee dimmers (Busch-Jaeger dimmer inserts, Tradfri, Hue, ...) and all of them offer different settings :/

I have no idea how to realize , may be @steilerDev help us.

I hope so too, as I would really like to avoid proxy items or running a fork of this great plugin.

grzegorz914 commented 4 years ago

Hi,

my complete home is control over S71200 PLC, controll of Outputs is realized over Input source, all data is stored in PLC, OH is only bridge between S7 and Homebridge. I use @docbender binding to communicate between OH and S7: https://github.com/docbender/openHAB-Simatic.

Here my items:

Switch    PLC_KuchniaMebleTaster    "Kuchnia Meble" <wallswitch>    (TasterParter, Taster)  { simatic="plc:M21.1:byte", autoupdate="false"}
Dimmer    KuchniaMebleDimmer
Dimmer    PLC_KuchniaMebleDimmer   "Kuchnia blat ściemniacz"    <dimmer>    (SciemniaczParter) { simatic="plc:DB26.DBB44:byte" }
Switch   PLC_KuchniaMebleSwiatlo    "Kuchnia blat światło" <light>    (SwiatloParter, SwiatloParterAktywne, Swiatlo)  { simatic="plc:Q21.1:byte" }

rules:

rule "Oświetlenie kuchnia meble dimmer on/off i zmiana poziomu"
when
    Item KuchniaMebleDimmer received command
then
     if (!(receivedCommand instanceof Number))
        switch receivedCommand {
            case ON :    if (PLC_KuchniaMebleSwiatlo.state == OFF) {
                                 PLC_KuchniaMebleTaster.sendCommand(ON)
                                 PLC_KuchniaMebleTaster.sendCommand(OFF)}
            case OFF  :  if (PLC_KuchniaMebleSwiatlo.state == ON) {
                                 PLC_KuchniaMebleTaster.sendCommand(ON)
                                 PLC_KuchniaMebleTaster.sendCommand(OFF)}   
                               }
      else 
           if (receivedCommand instanceof Number && (receivedCommand != 0 && receivedCommand != 100))
                       PLC_KuchniaMebleDimmer.sendCommand(receivedCommand)
end 

rule "Oświetlenie kuchnia meble"
when
    Item PLC_KuchniaMebleSwiatlo received command
then
         PLC_KuchniaMebleTaster.sendCommand(ON)
         PLC_KuchniaMebleTaster.sendCommand(OFF)
end

rule "Oświetlenie kuchnia meble ustawienie poziomu po załączeniu"
when
    Item PLC_KuchniaMebleSwiatlo changed
then
      if (PLC_KuchniaMebleSwiatlo.state == ON) {
          KuchniaMebleDimmer.postUpdate(PLC_KuchniaMebleDimmer.state)
        } else if (PLC_KuchniaMebleSwiatlo.state == OFF) {
                   KuchniaMebleDimmer.postUpdate(OFF)
        }       
end

rule "Oświetlenie kuchnia meble dimmer aktualizacja poziomu"
when
    Item PLC_KuchniaMebleDimmer changed
then
      var Number poziom1 = PLC_KuchniaMebleDimmer.state
      if (PLC_KuchniaMebleSwiatlo.state == ON && (KuchniaMebleDimmer.state != PLC_KuchniaMebleDimmer.state)) {
          KuchniaMebleDimmer.postUpdate(poziom1)
        } 
end

all working like a harm with save last level of dimmers in DB of S7. In this config I can change level of dimmer and state of all lights from any available site(HMI, OH app, HomeKit app, web) all with real-time synchronization between all devices.

Here #45 is the complete log from OH.

Thanks

extric99 commented 4 years ago

Would it be an option to have different logics available if there is not one size fits all and make this configurable with a parameter at item level?

grzegorz914 commented 4 years ago

U can make what U want, just create different rules in OH.

weakfl commented 4 years ago

U can make what U want, just create different rules in OH

@grzegorz914 the whole point is to avoid additional rules that serve no real purpose

kq9914 commented 3 years ago

I'm running into a similar issue. When I specifically tell Siri to set a dimmable light to 100, the command ON is sent to OpenHAB rather than 100.

To add to this, a homebridge accessory of type "Fan" sends "100" to OpenHAB rather than "ON" when Siri is told to set it to "100". Both are of type "Dimmer" in OpenHAB. It seems odd that this behavior is not uniform.

kq9914 commented 3 years ago

U can make what U want, just create different rules in OH

@grzegorz914 the whole point is to avoid additional rules that serve no real purpose

Agree. This isn't an OpenHAB behavior that needs to be changed.

weakfl commented 3 years ago

@kq9914 fwiw, I'm running a fork with my proposed changes for more than a year now without any problems. I never had a problem like @grzegorz914 described.

But ultimately it's up to @steilerDev what the default implementation should be…

kq9914 commented 3 years ago

@kq9914 fwiw, I'm running a fork with my proposed changes for more than a year now without any problems. I never had a problem like @grzegorz914 described.

But ultimately it's up to @steilerDev what the default implementation should be…

@weakfl is your forked plugin available? since this is the behavior @steilerDev wants, this plugin isn't going to work for me. I'm not going to write proxy items for every dimmer in OH. The version in your repo looked like it hadn't been modified.

weakfl commented 3 years ago

@kq9914 it's in the fix-level branch.

kq9914 commented 3 years ago

@kq9914 it's in the fix-level branch.

Works perfectly now. Thanks so much!

Matsuo3rd commented 2 years ago

Reporting that I am also using this "patch" for my (Zwave) lights to operate as expected. Any chance the plugin could offer a boolean setting (global or item level - false by default)? If true then behave as the patch indicate, otherwise execute current behavior.

steilerDev commented 2 years ago

@weakfl @kq9914 @Matsuo3rd I added a configuration sendOnOnlyWhenOff (happy for a better naming of this) with commit f340d95 on the master branch, that should enable @weakfl 's additional check.

Unfortunately I don't have any lights in my configuration to test this with at the moment. Could you please see if everything works as expected when setting the flag? It should log an error at the moment, to see if the configuration is applied properly.

Matsuo3rd commented 2 years ago

Reporting positive feedback on sendOnOnlyWhenOff setting. This is working on my Zwave lights as expected (eg. set brightness from 33% to 100%). Thanks @steilerDev

steilerDev commented 2 years ago

Should be fixed with v1.3.4 / f340d95