homebridge / plugins

Information and resources for Homebridge plugins.
https://homebridge.io/w/Verified-Plugins
GNU General Public License v3.0
368 stars 17 forks source link

homebridge-tp-link-access-control #552

Closed jgrimard closed 1 year ago

jgrimard commented 1 year ago

Link To GitHub Repo

https://github.com/jgrimard/homebridge-tp-link-access-control

Link To NPM Package

https://www.npmjs.com/package/homebridge-tp-link-access-control

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

Hi @jgrimard

All looks generally good by the way but of course I need to point out the questions I have!

So I was playing around with different configs and was good to see the plugin disables itself if the required fields aren't set.

But then I went along with the suggested/default config of (with a child bridge):

        {
            "ipAddress": "192.168.1.1",
            "password": "admin",
            "devices": [
                {
                    "name": "Device Name",
                    "mac": "00-00-00-00-00-00"
                }
            ],
            "_bridge": {
                "username": "0E:XX:C1:19:XX:69",
                "port": 50651
            },
            "platform": "tp-link-access-control"
        }

and whilst this doesn't crash homebridge, I assume since I don't have the devices the plugin is looking for, I get these messages:

[03/07/2023, 22:17:09] [homebridge-tp-link-access-control] This plugin is taking long time to load and preventing Homebridge from starting. See https://homebridge.io/w/JtMGR for more info.
[03/07/2023, 22:17:29] [homebridge-tp-link-access-control] This plugin is taking long time to load and preventing Homebridge from starting. See https://homebridge.io/w/JtMGR for more info.
[03/07/2023, 22:17:49] [homebridge-tp-link-access-control] This plugin is taking long time to load and preventing Homebridge from starting. See https://homebridge.io/w/JtMGR for more info.
[03/07/2023, 22:18:09] [homebridge-tp-link-access-control] This plugin is taking long time to load and preventing Homebridge from starting. See https://homebridge.io/w/JtMGR for more info.
[03/07/2023, 22:18:29] [homebridge-tp-link-access-control] This plugin is taking long time to load and preventing Homebridge from starting. See https://homebridge.io/w/JtMGR for more info.
[03/07/2023, 22:18:49] [homebridge-tp-link-access-control] This plugin is taking long time to load and preventing Homebridge from starting. See https://homebridge.io/w/JtMGR for more info.
[03/07/2023, 22:18:59] [homebridge-tp-link-access-control] Initialization Error. Exiting.:  request to http://192.168.1.1/cgi-bin/luci/;stok=/login?form=keys failed, reason: connect ETIMEDOUT 192.168.1.1:80

So actually when I first started writing this comment I hadn't seen the last one about initialisation error. So good to see the plugin finally kinda gives up. But need a way of getting around the 'is taking long time to load and preventing Homebridge from starting` log warnings.

jgrimard commented 1 year ago

Hi @bwp91, You are right. I tested it with an invalid IP address that refused the connection but didn't test on an IP address that ignored the connection (or didn't exist). I'll get that fixed and let you know when it is ready for another check. Thanks.

bwp91 commented 1 year ago

Hi @jgrimard

Does your plugin make use of the didFinishLaunching homebridge event?

jgrimard commented 1 year ago

Hi @bwp91, No, I didn't think that event does anything when you set it up as a static platform. I thought that was only for dynamic platforms. I believe homebridge calls the accessories() function of a static platform once its ready. didFinishLaunching isn't used in the example code here: https://github.com/homebridge/homebridge-examples/blob/9b2b10f9c1a5768fc35c643002367bb5e2174ce3/static-platform-example-typescript/src/static-platform.ts

bwp91 commented 1 year ago

Ah I hadn't realised your plugin was a static platform, my apologies, I just looked at your config schema json file and saw

"pluginType": "platform",
jgrimard commented 1 year ago

@bwp91 No problem. It looks like that needs to be either accessory or platform. https://github.com/homebridge/homebridge/blob/6bdfc8123a027362650c464be31133fc7243bbdc/src/api.ts#L26-L29C2

jgrimard commented 1 year ago

@bwp91 Ok, we should be good to go. Let me know if you see any issues.

jgrimard commented 1 year ago

/check

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

jgrimard commented 1 year ago

/check

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

Great thanks @jgrimard im happy to verify this plugin now, so will run the github actions so you get all the nice stuff! I will then post a couple of recommendations that are just things from my opinion, up to you whether you want to look into them but not necessary for plugin verification. But thanks for sorting out the stuff above ^ 😀

github-actions[bot] commented 1 year ago

Everything Looks Good!

github-actions[bot] commented 1 year ago

Congratulations! Your plugin has been verified.

You can now add the Verified by Homebridge badge to your plugin's README:

verified-by-homebridge

[![verified-by-homebridge](https://badgen.net/badge/homebridge/verified/purple)](https://github.com/homebridge/homebridge/wiki/Verified-Plugins)

Your plugin is now also eligible to display a :heart: Donate button on its tile in the Homebridge UI. See https://github.com/oznu/homebridge-config-ui-x/wiki/Developers:-Donation-Links for instructions.

If for any reason in the future you can no longer maintain your plugin, please consider transferring it to our unmaintained plugins repo. We can take ownership until another willing developer comes along.

Thank you for your contribution to the Homebridge Community. https://homebridge.io

bwp91 commented 1 year ago
  1. Consider a name field in the config.schema.json so that a user can name the plugin how they would like:

https://github.com/bwp91/homebridge-govee/blob/486372504af857b539a7df8507e8f9eb4ca75f53/config.schema.json#L11-L15

This name appears in the homebridge log (for example Deebot here)

Screenshot 2023-07-10 at 23 04 13

as well as in the child bridge list

Screenshot 2023-07-10 at 23 04 44

bwp91 commented 1 year ago
  1. remember here that even though the warnings appear in the homebridge ui, a user can still save the config regardless of whether warnings are present or not

Screenshot 2023-07-10 at 23 05 39

If a user must use dashes instead of semi colon, then maybe the plugin could just convert this? whether a user uses : or -

jgrimard commented 1 year ago

Hi @bwp91 These are both great suggestions. I will add both of them. Thank you