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
Child Bridge Support #2786
Child Bridge Support #2786
Conversation
…rnal bridge config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here 🚀 I think this will be nice addition to the 1.3.0 release 👍
Just had some smaller comments.
const filePath = path.resolve(this.baseDirectory, itemName); | ||
return fs.readJsonSync(filePath); | ||
} catch (e) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK getItemSync is currently used in loadCachedPlatformAccessoriesFromDisk()
.
What I currently don't like that errors in the StorageService are complexity silenced.
Additionally loadCachedPlatformAccessoriesFromDisk()
will just do nothing if getItemSync returns null
.
Am I missing anything that this "empty" catch makes sense?
If we encounter an IO error, we would first of all never the error message, and homebridge would just not load cached accessories silently.
I would argue for a solution where homebridge fails to init if the file can't be loaded (maybe including some logic to repair permissions, which are no 1. issue I think).
Same applies to the Promise based method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reasons this would throw would be if the file does not exist yet or if the file is corrupted (not valid json) - in either case we would want to write a new cache file.
We could check for the files existence before attempting to read it so we can log an error if the file is corrupted or we don't have permissions to read it - that just requires one more stat call.
I wouldn't prevent the bridge from starting if the JSON isn't valid - just re-write and move on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I would suggest to make that explicit then. So comment those individual scenarios in the catch clause (maybe adding a debug log statement of the logger?).
And for the corrupted file scenario, I would propose that we try as much as possible to recover (e.g. ensure permissions set) and outer wise overwrite as you suggest, though with logging a warning. So we can easily debug why cachedAccessories was reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes have been made - red error log messages will explain that the cached accessories either cannot be saved or cannot be loaded and the implications of this. It will not prevent Homebridge from loading.
Also updated the loadCachedPlatformAccessoriesFromDisk
to use the async methods, as the only places we use this is in async functions, and async is better so why not 😄
What a great Feature, Thx !! |
This would be a manual process, as it's not supported by the UI. Basically you need to:
You only have one chance to get this right. If you start the (child) bridge with the old |
Great ! (a bit scary though...) I will try to merge one instance when the final release 1.3 of Homebridge is published Thanks for your help! |
I believe you should be able copy the same cached accessories for each plugin. Then when the bridge loads up it will be remove the cache accessories that aren't needed. I can test this and let you know. |
HomeKit identifies accessories by uuid and bridge. So you can restore cached accessories to another bridge by copying/renaming |
So you could potentially change the |
It’s the |
♻️ Current situation
Currently all Homebridge platform and accessory plugins are exposed to a single bridge (with the exception to external accessories). This results in the bridge only responding as fast as the slowest plugin, and can cause the entire bridge to crash if a single platform or accessory throws an unhandled exception.
💡 Proposed solution
This feature allows any Homebridge platform or accessory to optionally run as it's own independent accessory, seperate from the main bridge, and in an isolated process. There are several reasons / benefits from doing this:
homebridge-ring
plugin).homebridge-hue
while it is trying to discover the Hue/Deconz bridge)This will work with all existing plugins without requiring then to perform any code changes.
Users will be able to use the Homebridge UI to enable the feature:
Users can also manually add the require config to each platform/accessory config block you want to be exposed as a seperate bridge:
The following can also optionally set:
pin
- optional - defaults to the same as the main bridgeport
- optional - defaults to a random unused port assigned by HAPNode.jsname
- optional - defaults to accessory/platform namemodel
- optional - defaults to same as main bridgemanufacturer
- optional - defaults to same as main bridgesetupID
- optional - default to unique setup id generated by HAPNode.js (so we can have different QR codes for each child bridge eventually)An IPC service has also been added to allow tools such as the Homebridge UI to restart individual child bridges. Currently this just uses Node.js parent/child process IPC, at some point in the future we may swap to using
node-ipc
to allow control from a non-parent process.Problem that is solved
See above.
Implications
This is an opt-in feature. Existing setups should not be impacted.
Testing
I have tested this with accessories, static and dynamic platform plugins.
Reviewer Nudging
@Supereg