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

Support for multiple homebridge instances? #36

Open
dxdc opened this issue Mar 10, 2020 · 16 comments
Open

Support for multiple homebridge instances? #36

dxdc opened this issue Mar 10, 2020 · 16 comments
Assignees

Comments

@dxdc
Copy link
Contributor

dxdc commented Mar 10, 2020

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
Copy link
Owner

NorthernMan54 commented Mar 10, 2020 via email

@dxdc
Copy link
Contributor Author

dxdc commented Mar 11, 2020

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
Copy link
Contributor Author

dxdc commented Apr 20, 2021

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

@NorthernMan54
Copy link
Owner

@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
Copy link
Contributor Author

dxdc commented Apr 24, 2021

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.,

  • In prefs, give each instance a unique nickname (which will be shown as opposed to PIN)
  • Filter the list of devices/accessories by MAC address

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
Copy link
Owner

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
Copy link
Contributor Author

dxdc commented Apr 24, 2021

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
Copy link
Owner

NorthernMan54 commented Apr 24, 2021 via email

@dxdc
Copy link
Contributor Author

dxdc commented Apr 24, 2021

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
Copy link
Owner

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

@NorthernMan54
Copy link
Owner

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
Copy link
Contributor Author

dxdc commented Apr 24, 2021

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
Copy link
Owner

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

dxdc added a commit to dxdc/Hap-Node-Client that referenced this issue Apr 24, 2021
dxdc added a commit to dxdc/Hap-Node-Client that referenced this issue Apr 24, 2021
@dxdc
Copy link
Contributor Author

dxdc commented Apr 24, 2021

See what you think of this @NorthernMan54 :

NorthernMan54/Hap-Node-Client#49

NorthernMan54 pushed a commit to NorthernMan54/Hap-Node-Client that referenced this issue May 1, 2021
@NorthernMan54
Copy link
Owner

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

NorthernMan54/Hap-Node-Client@76dc925

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
Copy link
Contributor Author

dxdc commented May 1, 2021

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?

#92

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

No branches or pull requests

2 participants