NorthernMan54 / node-red-contrib-homebridge-automation

Homebridge and Node-RED Integration
Apache License 2.0
107 stars 18 forks source link

Support for multiple homebridge instances? #36

Open dxdc opened 4 years ago

dxdc commented 4 years ago

I have 2 homebridge instances running and I've added both PIN#'s into hb-conf nodes. Just trying to understand the behavior since it's my first foray into having >1 hb instance.

I have 2 separate hb instances defined (different port #'s, userid/mac address, etc.) in config.json. I have added the PIN# of both instances as an hb-conf node.

  1. No matter which PIN# I choose in the dropdown hb-control, hb-status, etc. node, I see all the accessories combined from both instances.

  2. What is the purpose of entering the PIN# here, when running in insecure mode? From testing, any PIN# will work. (Can insecure mode be disabled?)

Running with Node-RED v1.0.4 btw.

NorthernMan54 commented 4 years ago

What the PIN does is allow access to change the state of an accessory. Without the PIN you will not be able to control or turn on/off something from NODE-RED.

On Mar 10, 2020, at 5:07 PM, dxdc notifications@github.com wrote:

I have 2 homebridge instances running and I've added both PIN#'s into hb-conf nodes. Just trying to understand the behavior since it's my first foray into having >1 hb instance.

I have 2 separate hb instances defined (different port #'s, userid/mac address, etc.) in config.json. I have added the PIN# of both instances as an hb-conf node.

No matter which PIN# I choose in the dropdown hb-control, hb-status, etc. node, I see all the accessories combined from both instances.

What is the purpose of entering the PIN# here, when running in insecure mode? From testing, any PIN# will work. (Can insecure mode be disabled?)

Running with Node-RED v1.0.4 btw.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NorthernMan54/node-red-contrib-homebridge-automation/issues/36?email_source=notifications&email_token=AEXEFGFJN65TIVECWUNEMSLRG2TYXA5CNFSM4LFIBAS2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IUAXTRA, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXEFGBJEITREVEROLJDQJDRG2TYXANCNFSM4LFIBASQ.

dxdc commented 4 years ago

Ah, thanks @NorthernMan54 ... so the merging of all accessories into a single list is intentional? Just wasn't clear why there would be 2 PIN# dropdowns when choosing hb-control, etc. if it has no apparent effect.

dxdc commented 3 years ago

@NorthernMan54 updated both branches here with a modified variant for this.

NorthernMan54 commented 3 years ago

@dxdc Tks very much for this, I have just published both changes. Fingers crossed that it is stable.

I did notice that after updating that all my nodes had a red triangle beside them, and I needed to 'update' the configuration node in order to resolve. After updating the configuration node, everything worked perfectly.

Thank you

dxdc commented 3 years ago

Thanks @NorthernMan54! Glad to finally find a more amenable solution that doesn't break other plugins that use it.

Yes I noticed the same thing about the red triangle. I don't know if there is a way to avoid this... but it didn't harm anything.

So far it has been stable for me, but was a little tricky to figure out the MAC address for the HB instances in the beginning. For example, if using node-red-contrib-homekit-bridged it's not so obvious what the MAC address is, I had to find it in the debug pane from a failure message.

Maybe it would be helpful to move to a single config node, whereby all the MAC addresses found would be shown, and a nickname/PIN# could be specified for each.

The way I wrote the code, the MAC address is stored by the config node and transmitted to the hap-client when registering. However, in the pins variable, it also stores the returned ip:host as a key, because (I think?) sometimes the call is made using just ip and host and not deviceID. Not familiar enough with how the functions are called.

I think it could be nice to also filter the list of HB items shown based on the HB instance the user has selected. This probably needs more work; e.g.,

Anyway, perhaps a suggested feature for later and/or toggle on/off option if people don't like it. Thanks for continuing to maintain this plugin! It adds an unreal dimension of automation and control that HK/Eve/etc. are simply incapable of.

NorthernMan54 commented 3 years ago

I’m using deviceid as the homebridge instance unique key, as in long running implementations the host and port of the instance can change. If you have a homebridge instance configured without a port, it will pick a random port on every start. Cameras and other external accessories also change port on restart.

dxdc commented 3 years ago

Interesting. If that's true, why in Hap-Node-Client, do you have functions like this:

HAPNodeJSClient.prototype.HAPstatus = function(ipAddress, port, body, callback) {
HAPNodeJSClient.prototype.HAPevent = function(ipAddress, port, body, callback) {

etc. It would be much nicer if they were being called by deviceID, it would be cleaner in that case, but maybe there are other reasons to do it this way. For ex., it looks like it will use this upon failure via reconnectTimer.

I built the PIN function to lookup by "ipAddress:port" because I didn't see another way to avoid these functions being called.

NorthernMan54 commented 3 years ago

Those are the original interfaces, and have been superseded in the clients with the deviceid versions. ( unless I missed one in homebridge-automation )

Sent from my iPad

On Apr 23, 2021, at 10:25 PM, dxdc @.***> wrote:

 Interesting. If that's true, why in Hap-Node-Client, do you have functions like this:

HAPNodeJSClient.prototype.HAPstatus = function(ipAddress, port, body, callback) { HAPNodeJSClient.prototype.HAPevent = function(ipAddress, port, body, callback) { etc. It would be much nicer if they were being called by deviceID, it would be cleaner in that case, but maybe there are other reasons to do it this way. For ex., it looks like it will use this upon failure.

I built the PIN function to lookup by "ipAddress:port" because I didn't see another way to avoid these functions being called.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

dxdc commented 3 years ago

I see. I think I know what the issue is now:

HAPNodeJSClient.prototype.HAPresourceByDeviceID = function(deviceID, body, callback) {
  // console.log('This-0', this);
  _mdnsLookup(deviceID, function(err, instance) {
    // console.log('This-1', this);
    if (err) {
      callback(err);
    } else {
      HAPNodeJSClient.prototype.HAPresource.call(this, instance.host, instance.port, body, function(err, response) {
        if (err) {
          _mdnsError(deviceID);
        }
        callback(err, response);
      });
    }
  }.bind(this));
};

The pin is not provided as a parameter to HAPresource, and there are several of such functions. When I modified this last time, you said it broke older API's, so I understand the concern.

There are a few solutions I can see:

  1. Run var pin = _findPinByKey(deviceID); after _mdnsLookup, and pass the PIN as a parameter to a modified version of these functions (e.g., HAPResourceNew).
  2. Update _mdnsLookup to refresh the PIN.

    function _mdnsLookup(deviceID, callback) {
    // debug('\nmdnsLookup start', serviceName);
    if (mdnsCache[deviceID]) {
    // debug('cached', mdnsCache[serviceName].url);
    callback(null, mdnsCache[deviceID]);
    } else {
    _populateCache(4, null, function() {
    if (mdnsCache[deviceID]) {
      // debug('refreshed', mdnsCache[deviceID]);
    
     // --- start new lines ---
     var pin = _findPinByKey(deviceID);
     var host = mdnsCache[deviceID].host + ':' + mdnsCache[deviceID].port;
     this.RegisterPin(host, pin); // this will update every time
     // --- end new lines ---
    
      callback(null, mdnsCache[deviceID]);
    } else {
      callback(new Error('ERROR: HB Instance not found', deviceID), null);
    }
    });
    }
    }
NorthernMan54 commented 3 years ago

That seems reasonable, let me integrate that fragment in the AM. It’s getting late here

NorthernMan54 commented 3 years ago

I did an audit of your change, and it looks like you covered all the external interfaces. I even went and tested the HAPresource interface ( used to get camera images ) and it works okay. Did you identify a use case that is failing ? HAPresource is only used by homebridge-automation and not homebridge-alexa

Screen Shot 2021-04-24 at 9 16 24 AM

dxdc commented 3 years ago

I did an audit of your change, and it looks like you covered all the external interfaces.

Thanks for checking. Yes, tried to make it robust because I wasn't sure which of the prototypes were needed.

The only thing is that - although it worked OK for you - it may just be because the default PIN is your default PIN :) You'd have to test it against a new HB instance with a different PIN (and presumably, at least 1 device set up to control) if you didn't already.

Did you identify a use case that is failing ?

Originally, I did, which is why I added this:

https://github.com/NorthernMan54/Hap-Node-Client/blob/6dca9ab249ccc202f31a3c51b968739aa6e095bc/HAPNodeJSClient.js#L457

And, if IP/ports are changing, adding something like this would be even better (to _mdnsLookup) as suggested above to refresh it. My IP's are static and the ports are defined so I don't see this as necessary on my setup, wasn't even aware that could change over time until you mentioned it.

   // --- start new lines ---
   var pin = _findPinByKey(deviceID);
   var host = mdnsCache[deviceID].host + ':' + mdnsCache[deviceID].port;
   this.RegisterPin(host, pin); // this will update every time
   // --- end new lines ---

Another idea that I had (in the morning light...) is to somehow pass the deviceID to these functions. E.g., perhaps the entire mdnsCache object is passed as an input. This way, the PIN lookup can work solely on the deviceID.

NorthernMan54 commented 3 years ago

Aligning the PIN with DeviceID does make the most sense. If you want to rethink it further I can merge another pull request.

dxdc commented 3 years ago

See what you think of this @NorthernMan54 :

https://github.com/NorthernMan54/Hap-Node-Client/pull/49

NorthernMan54 commented 3 years ago

Just published this as v0.0.85

During regression testing I did find an issue where all use cases where failing with "Homebridge not initialized". Did a bit of digging into it and found this was missing

https://github.com/NorthernMan54/Hap-Node-Client/commit/76dc925e1de889b99da35fd303d370ad03149d00

Do you have the ability to do regression testing in your development setup ? On my development machine I have node-red running the code directly from my development directory, by linking it from the ~/.node-red/node_modules/ directory.

ie

cd node_modules
ln -s ~/Code/node-red-contrib-homebridge-automation/ node-red-contrib-homebridge-automation

And I do something similar to the Hap-node-client directory inside ~/Code/node-red-contrib-homebridge-automation/node_modules

dxdc commented 3 years ago

During regression testing I did find an issue where all use cases where failing with "Homebridge not initialized". Did a bit of digging into it and found this was missing

Ah, thanks. Yeah I missed it in my earlier changes.

As far as testing goes, I just missed it in my changes. I also pushed a PR that could be a building block for you to run automated tests. I have it in my NR plugin: https://github.com/dxdc/node-red-contrib-join-wait

And, included 40+ tests to run. Could be a simpler solution for future?

https://github.com/NorthernMan54/node-red-contrib-homebridge-automation/pull/92