luc-ass / homebridge-evohome

Homebridge plugin for Honeywell Evohome
29 stars 16 forks source link

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

Closed coolweb closed 1 year ago

luc-ass commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

I'll let you know how it goes. Thanks again! ๐Ÿ‘

MonsterArtur1 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I am merging this into "persistent-accessories" and test it. Keep an eye on it. ๐Ÿ˜‰