search

benzman81 / homebridge-http-webhooks

A http plugin with support of webhooks for Homebridge: https://github.com/nfarina/homebridge
GNU General Public License v3.0
178 stars 57 forks source link

Compatibility with Homebridge v2 #198

Open jsiegenthaler opened 1 week ago

jsiegenthaler commented 1 week ago

0.1.19-beta.1

From jsiegenthaler: Updated package.json to show compatibility with Homebridge v2 tested on homebdrige v2 with a motion sensor, all OK Updated dependencies: "http-auth": "^4.2.0", "node-persist": "^2.1.0", "request": "^2.88.2", "selfsigned": "^2.4.1" NOTE: a newer version of node-persist exists but all storage commands are async , requiring more code changes

jsiegenthaler commented 1 week ago

I'd like someone to review please

benzman81 commented 1 week ago

This is awesome. No code changea at all needed?

jsiegenthaler commented 1 week ago

Nope. But note I only tested the motion sensor. Strictly speaking, we should test every sensor type as I see you have a chunk of code per sensor. If you had some non-conformal code, it would through an error in Homebridge v2. But of course, if the code is done well, and you stick to the defined characteristic values, then no error will occur.

benzman81 commented 1 week ago

Ok, I will check how I can upgrade my docker homebridge to v2. This way some more accessories will be tested.

jsiegenthaler commented 1 week ago

I've created a full config with all supported accessories. Found this small issue, I'll fix it:

[24.09.2024, 06:30:57] Initializing platform accessory 'Fanv2-1'...
HAP-NodeJS WARNING: The accessory 'Fanv2-1' has an invalid 'Name' characteristic ('Fanv2-1'). Please use only alphanumeric, space, and apostrophe characters. Ensure it starts and ends with an alphabetic or numeric character, and avoid emojis. This may prevent the accessory from being added in the Home App or cause unresponsiveness.

Also fixed some names in the example config to be consistent with other examples

jsiegenthaler commented 1 week ago

Testing of each and ever sensor still needs to be done

benzman81 commented 1 week ago

Awesome. As soon as I can update my homebridge to v2 I will test it and merge it.

jsiegenthaler commented 1 week ago

Noted one issue last night: [24.09.2024, 22:57:33] [homebridge-http-webhooks] This plugin generated a warning from the characteristic 'Current Ambient Light Level': characteristic value expected valid finite number and received "NaN" (number). See https://homebridge.io/w/JtMGR for more info. This will need fixing in the code

Caused by [25.09.2024, 07:05:41] [homebridge-http-webhooks] Error: at CurrentAmbientLightLevel.characteristicWarning (C:\Users\jochen\AppData\Roaming\npm\node_modules\homebridge\node_modules\hap-nodejs\src\lib\Characteristic.ts:2785:105) at CurrentAmbientLightLevel.validateUserInput (C:\Users\jochen\AppData\Roaming\npm\node_modules\homebridge\node_modules\hap-nodejs\src\lib\Characteristic.ts:2673:14) at C:\Users\jochen\AppData\Roaming\npm\node_modules\homebridge\node_modules\hap-nodejs\src\lib\Characteristic.ts:2284:24 at C:\Users\jochen\AppData\Roaming\npm\node_modules\homebridge\node_modules\hap-nodejs\src\lib\util\once.ts:15:14 at HttpWebHookSensorAccessory.getState (C:\Users\jochen\Documents\GitHub\homebridge-http-webhooks\src\homekit\accessories\HttpWebHookSensorAccessory.js:155:5) at CurrentAmbientLightLevel.emit (node:events:519:28) at C:\Users\jochen\AppData\Roaming\npm\node_modules\homebridge\node_modules\hap-nodejs\src\lib\Characteristic.ts:2267:14 at new Promise (<anonymous>) at CurrentAmbientLightLevel.handleGetRequest (C:\Users\jochen\AppData\Roaming\npm\node_modules\homebridge\node_modules\hap-nodejs\src\lib\Characteristic.ts:2265:12) at CurrentAmbientLightLevel.toHAP (C:\Users\jochen\AppData\Roaming\npm\node_modules\homebridge\node_modules\hap-nodejs\src\lib\Characteristic.ts:2850:22)

I'll look at it tonight

jsiegenthaler commented 1 week ago

Saw a logging error and fixed it, and fixed some capitalisation errors in the logging in HttpWebHookThermostatAccessory.js, see commit log

jsiegenthaler commented 1 week ago

The issue "This plugin generated a warning from the characteristic 'Current Ambient Light Level'" is fixed. Root cause: the light sensor cannot have a minimum value of 0 for current light level. The minimum allowed is 0.0001. I added an IF statement to set state to 0.0001 when state is unknown

jsiegenthaler commented 1 week ago

@benzman81 I looked at some of the older PRs and if you want, we can incorporate some fixes into this PR. Example: #152 is a capitalisation inconsistency issue. I have an idea for a backwards compatible fix for this PR. I'd be happy to help. Let me know.