Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deviceState (doorstatus) not updating correctly when using multiple doors #6

Closed
danoday opened this issue Dec 19, 2015 · 8 comments · Fixed by #8
Closed

deviceState (doorstatus) not updating correctly when using multiple doors #6

danoday opened this issue Dec 19, 2015 · 8 comments · Fixed by #8

Comments

@danoday
Copy link

danoday commented Dec 19, 2015

My implementation has two Liftmaster 8500W doors, with two distinct device IDs. Because the 8500W's have embedded MyQ, I'm using harpchad's code (from pull request #1), along with setting the "requiredDeviceID" flag in the config.json. This allowed Siri to recognize and control the doors, but still has issues with the status. After looking at the code, it looks like the status is being pulled by code which only runs when a device ID hasn't been explicitly set, but does not run when the device ID IS explicitly set. Bottom line, it found the door, but never pulled the current status.

As a workaround, I modified the sanity check code in index.js to pull both the deviceID and the deviceState. Not putting this in as a pull request, as I'm not clear what the implications of the change are to other configurations. Try this at your own risk - if you have any issues, please let me know and I'll try to write up a pull request for Nick after some cleanup and testing.

           // We specified a door ID, sanity check to make sure it's the one we expected
            else if (that.requiredDeviceId == device.MyQDeviceId) {
           // Added attribute loop here to pull doorstate
              var thisDeviceId = device.MyQDeviceId;

              for (var j = 0; j < device.Attributes.length; j ++) {
                var thisAttributeSet = device.Attributes[j];
                if (thisAttributeSet.AttributeDisplayName == "doorstate") {
                  thisDoorState = thisAttributeSet.Value;
                }
              }
              that.deviceId = device.MyQDeviceId;
              that.deviceState = thisDoorState;
              break;
            }

With the above code added, Siri now correctly responds to queries like "Is the big garage door open?". Well, she still gets confused if the door is in the process of closing or has an issue like an obstruction, but that is kind of expected.

@JerGitHub
Copy link

@danoday Yes! I have the same issue now that I have all 3 of my Garage Doors being seen by homebridge (and the Elgato Eve app). All 3 of my doors show "Open" with "Obstruction Detected - NO" when they are all closed. I can't open them or close them with Siri cuz she thinks they are already open or vice versa.

I can try and edit the code you put in above or wait until we hear from @nfarina ?

Let me know if you want me to try anything to help.

@JerGitHub
Copy link

In case you want to see a screenshot of the Elgato Eve app. Keep in mind all 3 doors are closed when this screenshot was taken.
img_4497

@danoday
Copy link
Author

danoday commented Dec 28, 2015

@JerGitHub - I'd try my code revision for now, at least it will get you working until a more "elegant" fix is found. It works pretty consistently for me, although it only does the basics.

Some additional background on this, though. The original code pulls the attributes (including the doorstate). @nfarina pulls the status as a JSON array and places it in "body", and then looks through that array for devices. There is a section with the comment "// If we haven't explicity specified a door ID, we'll loop to make sure we don't have multiple openers, which is confusing", which appears to go through and find the door state for the door, but this is only run when one doesn't explicitly define "requiredDeviceID". For multiple doors where "requiredDeviceID" is defined, this piece of code never runs. This is important, because this is the only piece of code that gets "doorstate". My suggested fix copies the logic from there down to where it will run in a multiple door scenario.

It isn't elegant, and there is probably a better way to do this. I don't know Java well enough to do more than just hack around a bit.

You might want to add a line of code somewhere around line 95 to copy the contents of "body" to the log (added "that.log("Body: "+body);" as shown below. This is the entire JSON string returned from lift master, and contains some interesting stuff. It helps to use a web tool to put it in a proper JSON format, and you might find a compare tool helpful too (so you can compare different versions when you've changed state).

    // request details of all your devices
    request.get({
      url: "https://myqexternal.myqdevice.com/api/v4/userdevicedetails/get",
      qs: query,
      headers: headers
    }, function(err, response, body) {

      that.log("Body: "+body);

You can comment that line out with a "//" when you don't want to see "body", of course.

HomeKit still doesn't read the various states associated with the door properly. Lift master returns a doorstate of 1 for Closed, 2 for Open, 4 for Opening and 5 for Closing, but Siri interprets 4 as "Open" and 5 as "Closed". The plugin also doesn't appear to handle any obstruction status, even when I manually trigger an obstruction condition (read that as "I put my foot in front of the door sensor when it was closing"). This doesn't seem to alter the doorstate returned by liftmaster, so I suspect that the obstruction flag might be a different attribute altogether. If someone finds the attribute, please let me know.

@nfarina
Copy link
Owner

nfarina commented Dec 28, 2015

Hey guys - this plugin uses code that is pretty old and was modified after the fact by others to "support" multiple doors (in an inelegant fashion as you've seen). I embarked on a project to completely rewrite this plugin but ran out of time. If anyone wants to pick up on that I can publish a branch with the new in-progress one, otherwise I'm open to PRs that fix the state fetching issue you're describing.

@JerGitHub
Copy link

@danoday thanks for the workaround with the sanity check code in index.js to pull both the deviceID and the deviceState. I just added it today and it seems to be working.

I'm not an expert in coding compared to you and @nfarina but it looks like this should be put into the liftmaster plugin code. Per the comment above, it sounds like the liftmaster plugin code is old too - should the entire plugin be re-written? Again, I am not as skilled in this as you guys but want to see this working as optimally as possible.

Thanks again for the workaround!

@AppleTechy
Copy link
Collaborator

@JerGitHub based of what @nfarina said the plugin should be rewritten but I think the issue is all the people who know how to code are to busy to do so. I would do it but I have little experience coding and am not up to the task. :( . The new Plugin probably should include @danoday 's sanity check fix along with @harpchad 's embedded MYQ device adaptation. Those of which could be easily implemented into the current addition for the time being but from my understanding the plugin is outdated and is in need of some TLC. @nfarina please correct me if I misunderstood what was said about the plugin

@nfarina
Copy link
Owner

nfarina commented Jan 6, 2016

@AppleTechy That's an accurate view of things, yes.

@nfarina
Copy link
Owner

nfarina commented Jan 6, 2016

If someone wouldn't mind wrapping this up in a pull request, it would be much easier for me to review! I'd be happy to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants