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

fix #61 do not return empty array of accessories #129

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

coolweb
Copy link

@coolweb coolweb commented Oct 26, 2022

No description provided.

@coolweb coolweb changed the title do not return empty array of accessories, try to fix #61 fix #61 do not return empty array of accessories Oct 26, 2022
@luc-ass
Copy link
Owner

luc-ass commented Oct 26, 2022

Thank you for your PR! It's awesome to see some work on this. 🚀

Have you tested this? What happens when the login fails but the server returns after a few minutes? Could you elaborate a little?

I remember adding those callbacks to make the Plug-in non-blocking. Must be a few years ago...

@coolweb
Copy link
Author

coolweb commented Oct 26, 2022

I didn't test it, but if the plugin is not configured as a child bridge it'll block homebridge, but as a child bridge it'll not block homebridge. This is the remark of @ebaauw.

@luc-ass
Copy link
Owner

luc-ass commented Oct 26, 2022

Got it. The empty callback was needed to make the plugin non-blocking. This was proposed by the homebridge-team to have the plugin verified.

I'll do the following:

  • Test your PR against server failure/recovery to see whether it returns to normal by itself
  • Add a LARGE warning, that this plugin may only be run as a child bridge to prevent blocking
  • Research whether we could return an error instead of an empty array

I'll let you know how it goes. Thanks again! 👏

@MonsterArtur1
Copy link

There are other plugins, like WIZ bulb that didn't block homeridge and also shows devices as offline when unavailable.
Maybe there is a hint? Maybe we need to cache accessory id when unavailable?
Sorry for being not helpful with code :(

@luc-ass
Copy link
Owner

luc-ass commented Oct 26, 2022

@MonsterArtur1 No worries. Every part of the discussion helps. 👍 The WIZ-Plugin was written according to the new Typescript guidelines. This automatically deals with the problem. It's just that I am missing the resources to rewrite the plugin at the moment. 🥲

@coolweb I verified that changing callback([]) to callback(err) (without empty array) preserves the aid/iid of the accessories, but still removes them from HomeKit. They reappear after adding the correct credentials, but I couldn't see whether this still breaks automations... testing... 🧪

I have contacted donavanbecker in the original issue, let's see if he has some input 💡

@coolweb
Copy link
Author

coolweb commented Oct 26, 2022

@luc-ass after some research, it appears that the plugin should start after the event didFinishLaunching, in the doc
there is some logic to keep devices from cache:

api.on('didFinishLaunching', () => {
  const uuid = api.hap.uuid.generate('SOMETHING UNIQUE');
      
  // check the accessory was not restored from cache
  if (!this.accessories.find(accessory => accessory.UUID === uuid)) {
    // create a new accessory
    const accessory = new this.api.platformAccessory('DISPLAY NAME', uuid);
    // register the accessory
    api.registerPlatformAccessories('PLUGIN_NAME', 'PLATFORM_NAME', [accessory]);
  }
});

@luc-ass
Copy link
Owner

luc-ass commented Oct 26, 2022

That's for "dynamic platforms" (loading accessories after homebridge started). This plugin is a static platform. It loads the accessories at runtime. One more thing that would be solved by a rewrite 😆

Never the less this is a good example on how to read/load accessories from cache. Testing... 🧪

@luc-ass
Copy link
Owner

luc-ass commented Nov 29, 2022

Just a quick follow up, your pull request has not been forgotten. I'll include this in a future version, but I will probably include a switch to turn this off.

@luc-ass luc-ass changed the base branch from master to persistent-accessories December 22, 2022 18:18
@luc-ass
Copy link
Owner

luc-ass commented Dec 22, 2022

I am merging this into "persistent-accessories" and test it. Keep an eye on it. 😉

@luc-ass luc-ass merged commit 5bc1c8c into luc-ass:persistent-accessories Dec 22, 2022
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 this pull request may close these issues.

4 participants