search

node-alarm-dot-com / homebridge-node-alarm-dot-com

Alarm.com plugin for Homebridge using Node.js
MIT License
58 stars 23 forks source link

Update timerLoop() variable factor #79

Closed DMBlakeley closed 3 years ago

DMBlakeley commented 3 years ago

Reduce timerLoop() variable factor from 300000 to 30000. Variable factor was too large which can result in a Alarm.com sample being performed after account Authorization has expired. If sample occurs after Authorization has expired the ECONRESET error can result.

The variable factor was introduced in beta-6 and was why beta-4 did not produce errors.

DMBlakeley commented 3 years ago

Recommend releasing v1.7.2 with this change. V1.7.2 provides a number of key improvements for handling of cached devices which may address other issues. Will then provide new baseline for further troubleshooting.

chase9 commented 3 years ago

Good find on the timer being wrong, but I don't think this is the cause of the ECONNECT issues. At least, it's not the primary cause. Here's an issue describing the ECONNECT error before I implemented the random timer: https://github.com/node-alarm-dot-com/homebridge-node-alarm-dot-com/issues/66.

DMBlakeley commented 3 years ago

I probably reset my configuration as I unloaded beta.9 and did not use the plug-in for several days. Then when re-installing I also created a new secondary login for Alarm.com. This is when I found the error in the random timer and implemented locally in the beta.9 index.js file. Also added some additional logging to confirm that the login() function were working correctly. I am no longer seeing any errors from Alarm.com which was not the norm ~3 weeks ago. My system only consists of the panel, 6 contact sensors and 2 motion sensors.

Would be glad to try to help with debugging but find it appears that the source for beta.9 probably only exists on you development system. Bits and pieces of the beta builds are posted but there seem to be gaps. The NPM download is correct but this is the built output and not the source files. This is why I suggested that you release a v1.7.2 merging in all the pieces so that there is a new baseline to work with. You may have another suggestion on how to move forward.

rgd

chase9 commented 3 years ago

I believe the code you're looking for is on the beta branch in this repo as I push everything to Github.

I has been holding off on releasing a new version due to the ECONNECT error, but I agree at some point the pros are worth any cons. I'll look more tonight about making a release.

DMBlakeley commented 3 years ago

I have been able to build a version from your posts to GitHub. Have running without errors. There are a number of 'npm audit' warnings that I believe should probably be addressed before doing too much debugging.

DMBlakeley commented 3 years ago

@chase9, just an update. Have addressed the 'npm audit' issues. I am the author of the homebridge-awair2 plugin so made some changes to keep the code consistent for my own use.

I am not getting the ECONNECT errors with either my main or secondary logins, however, I am still getting the following error which is random in occurrence and takes several sampling cycles to clear:

[6/20/2021, 2:48:42 PM] [Security System] Error: GET https://www.alarm.com/web/api/systems/systems/2736084 failed: [object Object]
    at /usr/local/lib/node_modules/homebridge-node-alarm-dot-com/node_modules/node-alarm-dot-com/dist/index.js:464:15
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async Promise.all (index 0)

which I am trying to track down. Updated node-fetch to v3.0.0-beta.9 as a first step but still get error.

chase9 commented 3 years ago

Thanks for looking into this so closely! I'm pretty sure the error you're seeing is the ECONNECT error, just in a different form. I myself am seeing the ECONNECT error still, but I just switched to using a new login as you recommended. I'll report back what I see.

DMBlakeley commented 3 years ago

I've notice that when I do get this error it keeps repeating on each time loop but is resolved on the next account authentication. My current thinking is to catch the error and trigger a re-authentication. Thoughts on whether this would cause issues with Alarm.com?

Another item I am struggling with is that node-fetch is called out as a package at the top level (but does not appear to be used) as well as at the node-alarm-dot-com level. When node-fetch is called a the node-alarm-dot-com level my understanding is that the package at this level will be used and not the one at the higher level. Still learning ...

DMBlakeley commented 3 years ago

@chase9, I now believe that the above error is not a variant of the ECONNECT issue but appears to be issues with async/await calls.

In my account I have v1.7.3-beta.3 of Node-Alarm-Dot-Com. I used v1.7.3 to break the chain with the v1.7.2 betas. This version has been running well without errors.

There are a many small updates with the highlights being:

So, overall just a scrubbing of the code but no real changes in the overall operation.

There is 1 manual change. I updated package.json in the node_modules/node-alarm-dot-com module to point to node-fetch@v3.0.0-beta.9.

Right now I have the archive as private but could give you collaborator access to review if you wish. Let me know.

anthonyb82 commented 3 years ago

@DMBlakeley thanks for all the work you have done looking in to this. I’d be happy to beta test it if possible. Since the beta 9 update I no longer ever receive the ECONNRESET errors but do experience the “not found” ones you described.

DMBlakeley commented 3 years ago

@anthonyb82 appreciate the offer. I thought I had this secondary error licked but in reviewing the logs from last night I found a couple of occurrences. Need to do a bit more digging.

chase9 commented 3 years ago

I can't thank you enough for looking into this, @DMBlakeley. A task can quickly feel impossible without help. I'll look at the repo you invited me to and add the changes to the current beta branch.

If people not on the beta are experiencing these issues, I would be comfortable pushing this update out to stable.

DMBlakeley commented 3 years ago

Certainly was glad to dig in! Like the challenge to improve my HomeKit/homebridge setup.

Unfortunately, I still get the error sporadically which I cannot fully explain. Always happens with the refreshDevices() loop when making a get() call. I still believe that the problem has to do with the async/await hierarchy between functions. As I have been working through the hierarchy the problem seems to get less frequent.

One thought I had was to switch from node-fetch to axios for the get() and push() calls. I am using axios on my other plugin and have not had an issue. I am not getting good error handling with node-fetch and believe this would be improved with axios. Thoughts?

I would hold on pushing the beta stream but appreciate you taking a look. I am boing to be out of the loop for the next few days but should be able to pick back up again next week.

Regards, Doug

chase9 commented 3 years ago

I’m not at all opposed to replacing node-fetch as it seems we could benefit from a more robust http client.

Some background on this project is that it originally was forked by Mike Kormendy, and then taken on by me once Mike switched from an alarm.com http://alarm.com/ provider. There is a bit of technical debt, and switching to Typescript was my first attempt at solving that. Now that we’ve switched, I’m much more comfortable replacing parts of the plugin.

Chase

On Jun 25, 2021, at 6:55 PM, Doug B @.***> wrote:

Certainly was glad to dig in! Like the challenge to improve my HomeKit/homebridge setup.

Unfortunately, I still get the error sporadically which I cannot fully explain. Always happens with the refreshDevices() loop when making a get() call. I still believe that the problem has to do with the async/await hierarchy between functions. As I have been working through the hierarchy the problem seems to get less frequent.

One thought I had was to switch from node-fetch to axios for the get() and push() calls. I am using axios on my other plugin and have not had an issue. I am not getting good error handling with node-fetch and believe this would be improved with axios. Thoughts?

I would hold on pushing the beta stream but appreciate you taking a look. I am boing to be out of the loop for the next few days but should be able to pick back up again next week.

Regards, Doug

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/node-alarm-dot-com/homebridge-node-alarm-dot-com/pull/79#issuecomment-868888377, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN4B26B4WR3UX4VH3SOVZLTUUJPHANCNFSM46YNYFZQ.

DMBlakeley commented 3 years ago

Ok, will look into next. Thanks for the background on the plugin.

homebridge-awair2 was my first attempt at TypeScript when I ported homedridge-awair and was quite glad that I did. As I am still a bit of a newbie I am more comfortable with being explicit with async/await/promise rather than letting TypeScript sort out.

DMBlakeley commented 3 years ago

@chase9, I have now pushed beta.4 to my archive. I believe that I have now addressed the error that I was seeing as well as scrubbed async/await/Promise methodology to ensure that functions were completed in the right order. I did not transition from node-fetch to axios as I found it was not required and would have made significant changes to node-alarm-dot-com.

I wanted to get this pushed to you as it appears there are a number of other thoughts on additional functionality. Come the end of August I will be moving and no longer have an Alarm.com account. If there is any help I can provide up till then please let me know.

DMBlakeley commented 3 years ago

@chase9, got a bit too confident as I am still getting errors but believe I am now on the right track. Suggest not merging at this point.

DMBlakeley commented 3 years ago

Hi @chase9, I have submitted a PR to update the beta branch to beta.11. Believe that this addresses issues #75, #74, and #62.

I have done significant troubleshooting and cannot totally eliminate errors from Alarm.com. Decided to gracefully recover after one sampling cycle and this seems to work well.

I have also posted beta.7 of node-alarm-dot-com. This is a conversion of beta.6 to async/await format and have found no operational difference between using beta.6 or beta.7. Have not done a PR for beta.7 but can if you wish.

I am using homebridge-node-alarm-dot-com-beta.11 with node-alarm-dot-com-beta.7 and the combination has been run smoothly.

Will leave to your expertise to decide how to proceed.