PatchworkBoy / homebridge-edomoticz

Domoticz Homebridge-Plugin
Other
118 stars 44 forks source link

New Domoticz blinds support bug introduced in 2.1.40, always using legacy commands #253

Closed ThomasRobertFr closed 1 year ago

ThomasRobertFr commented 1 year ago

Hello,

I have an up-to-date setup with Domoticz and Homebridge + edomoticz plugin. In domoticz, I have some "EU venitican blinds" devices set up.

When I click on the open/close buttons from Domoticz UI, it's behaving as expected. However, from Homebridge, it's reversed: the "open" action makes them close and the other way around.

I think the problem might come from the fast that my blinds are of type "RFY", and the code in the latest PR does if (message.stype == 'RFY') { position = 100 - position; }. Indeed, in domoticz I had to check "Reverse Open/Close state" so that Domoticz UI open button indeed opens my blinds. This might be related.

Thanks a lot for this work and I'm completely available to help fixing this.

ThomasRobertFr commented 1 year ago

@ksacca since you worked on the PR, maybe it makes sense to mention you in this issue?

ksacca commented 1 year ago

Sorry for the issue. As I said on the domoticz forum, I don’t own any Venetian blinds and have no idea how they work. That’s why I waited more than a month before making a PR with my changes (hoping some bugs might get detected). I’ll have a look at it tomorrow and might ask you to test a few things.

ThomasRobertFr commented 1 year ago

EDIT: Changed with my final conclusions

Hi! first, thanks for your prompt reply @ksacca and please don't apologise ;)

I have investigated further the issue on my side. The bug is not present on version 2.1.39 on npm, but is present on 2.1.40.

This problem comes from the fact that 2.1.40 reverted a small piece of your PR: these changes are reverted here.

This causes Homebrige to send On/Off commands instead of Open/Close which corresponds to the legacy commands that have changed in Domoticz 2022.2:

# Open command sent by Homebridge
{"command":"switchlight","idx":29,"switchcmd":"Off"}  # Should be "Open" not "Off"
# Close command sent by Homebridge
{"command":"switchlight","idx":29,"switchcmd":"On"}   # Should be "Close" not "On"

@PatchworkBoy Was this change intentional or maybe it was an error during conflict solving? Do you which me to do a PR to revert these lines?

ksacca commented 1 year ago

EDIT: I see you also edited your message. I'm still using my own version, so I did not see the changes in 2.1.40. Commands should indeed be Open or Close. On or Off still works for now in domoticz (this might change), but it messes up when you invert the state I believe. Anyway, my remarks still Apply, so it would be nice if you could test the code below.

@ThomasRobertFr I'm a coding enthousiast and like to learn, but I am not a developer. It was my first PR, so that's maybe why I feel the need to appologise 🤣.

The issue is not in the code if (message.stype == 'RFY') { position = 100 - position; } since this is only used if legacy blinds is set to 1 (for the old way). Nothing changed there.

In your case, the subtype might be RFY, but as you have venetian blinds, the previous check will evaluate to true (else if ((this.swTypeVal == Constants.DeviceTypeBlindsVenetianUS || this.swTypeVal == Constants.DeviceTypeBlindsVenetianEU) && message.nvalue > 1)) so the RFY type will never be checked. But thanks to your remark I also found an issue for Regular RFY blinds which I tried to fix. These blinds use an nvalue 1 for open and 3 for close (and vice versa if reversed state is on). Normal blinds use nvalue 1 and 0.

I suppose you use Venetian blinds of type US and not EU. US venetian blinds uses nvalue of 15 and 16. As you can see in the code, only nvalue == 15 was evaluated. But EU venetian blinds uses nvalues 17 and 18 so I suppose they never worked with this plugin.

Could you replace the code starting from line 1317 until 1327 with this code. I was able to create dummy RFY blinds (including venetian EU and US) and all possible reversed combinations worked for me:

                        } else if ((this.swTypeVal == Constants.DeviceTypeBlindsVenetianUS || this.swTypeVal == Constants.DeviceTypeBlindsVenetianEU) && message.nvalue > 1) {
                            //position = (message.nvalue == 15 ? 100 : 0);
                            if (message.ReverseState == 'true') {
                                position = ((message.nvalue == 16 || message.nvalue == 18)  ? 100 : 0);
                            } else {
                                position = ((message.nvalue == 15 || message.nvalue == 17) ? 100 : 0);
                            }
                        } else {
                            if (message.ReverseState == 'true') {
                                position = (message.nvalue == 1 ? 0 : 100);
                            } else {
                                position = (message.nvalue == 1 ? 100 : 0);
                            }
                          }
                    }
PatchworkBoy commented 1 year ago

@ThomasRobertFr unintentional - mistake my end at a guess whilst merging several revisions... drop in a PR to undo it and I'll merge it straight in and push up 2.1.41

ksacca commented 1 year ago

@ThomasRobertFr Could you test my changes on your RFY blinds first (if possible also by changing your Venetians temporary to regular RFY blinds)? If this works for you as well, you can include it in your PR. Thank you already for testing.

PatchworkBoy commented 1 year ago

Hold up a mo...! Just grab domoticz_accessory.js from the repo and drop it over your current copy and restart homebridge and see if that resolves. This (rather essential) chunk of code was missing from the merges after a few minor fixes at my end to another device type oopsie sorry!

if (this.platform.config.legacyBlinds == 1) { var command = (shouldOpen ? "On" : "Off"); } else { var command = (shouldOpen ? "Close" : "Open"); }

EDIT: Try it NOW... cos I uploaded it into root instead of into lib rolls eyes (sorry - need more coffee) Have also added @ksacca's code above...

PatchworkBoy commented 1 year ago

Nudge to notify of edit to above post

Git now at 2.1.42 awaiting confirmation above is all resolved before publishing up to NPM.

ksacca commented 1 year ago

I tested:

All of the above was tested with the option Reverse enabled and disabled. I didn't detect any issues during testing, but it would be nice if @ThomasRobertFr could test it as he owns the real hardware. So for me it is OK now.

Thank you again @PatchworkBoy for your work on this project. Next to domoticz itself, this is the most used feature in our home. Couldn't live without it (mainly for the WAF 🤣 ).

If possible, could you also add the info about dimfix and legacyblinds to the readme? I believe it was also removed from my PR by accident, see https://github.com/PatchworkBoy/homebridge-edomoticz/commit/9e08e09e93204da05aa8c2df7ba30eaf1be13982.

PatchworkBoy commented 1 year ago

Done - now at 2.1.45... with minor tweak to resolve https://github.com/PatchworkBoy/homebridge-edomoticz/issues/200

ksacca commented 1 year ago

Thanks. I posted a message on the domoticz forum to inform the users.

watchaaa commented 1 year ago

Hi, I switched back to the previous version 2.1.39 because blinds are working inverted with the 2.1.40. 2.1.39 is working perfectly with new domoticz blinds system, but 2.1.40 made problems (some blinds inverted), even if I change "blinds system legacy" in advanced settings.

ksacca commented 1 year ago

In version 2.1.40 there was an issue where a crucial part of the PR for the new blind system was overwritten by other changes. This has been fixed in version 2.1.41. We should now be at 2.1.45 which fixes some bugs for venetian and RFY blinds, but I see npm is still at version 2.1.40. You can see when the version is updated if you go to https://www.npmjs.com/package/homebridge-edomoticz.

ThomasRobertFr commented 1 year ago

@PatchworkBoy Can you publish the latest versions on npm so that I can test the fixes properly?

Again, thanks a lot for your quick reaction on this!

PatchworkBoy commented 1 year ago

Done...

watchaaa commented 1 year ago

Hi, now the 2.1.45 version is in homebridge (but the release notes in pop up are still for 2.1.41) I confirm that now everything is working good with the 2.1.45 version ! Thanks for the great work !