homebridge / verified

Plugins Verified by Homebridge
https://homebridge.io/w/Verified-Plugins
GNU General Public License v3.0
366 stars 17 forks source link

homebridge-sma-home-manager #527

Closed wimleers closed 10 months ago

wimleers commented 1 year ago

Link To GitHub Repo

https://github.com/wimleers/homebridge-sma-home-manager

Link To NPM Package

https://www.npmjs.com/package/homebridge-sma-home-manager

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

A basic config of

        {
            "platform": "SMAHomeManager",
            "_bridge": {
                "username": "0E:E7:D4:44:A2:0A",
                "port": 46564
            }
        }

sends Homebridge into a crash loop:

[26/03/2023, 18:04:39] Registering platform 'homebridge-sma-home-manager.SMAHomeManager'
[26/03/2023, 18:04:39] [homebridge-sma-home-manager] Loaded homebridge-sma-home-manager v1.1.1 child bridge successfully
[26/03/2023, 18:04:39] Loaded 0 cached accessories from cachedAccessories.0EE7D444A20A.

/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285
                Object.keys(signalNames).forEach(id => {
                           ^
TypeError: Cannot read properties of undefined (reading 'offGrid')
    at /usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:286:35
    at Array.forEach (<anonymous>)
    at SMAHomeManager.accessories (/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285:28)
    at /usr/local/lib/node_modules/homebridge/src/bridgeService.ts:534:24
    at new Promise (<anonymous>)
    at BridgeService.loadPlatformAccessories (/usr/local/lib/node_modules/homebridge/src/bridgeService.ts:528:12)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:155:36)
[26/03/2023, 18:04:39] [homebridge-sma-home-manager] Child bridge process ended
[26/03/2023, 18:04:39] [homebridge-sma-home-manager] Process Ended. Code: 1, Signal: null
[26/03/2023, 18:04:46] [homebridge-sma-home-manager] Restarting Process...

Also does your plugin ever wait for the didFinishLaunching event? Im not sure if your plugin type needs to or not. But I wonder if this is the reason of the logging

[26/03/2023, 18:23:24] [homebridge-sma-home-manager] This plugin is taking long time to load and preventing Homebridge from starting. See https://homebridge.io/w/JtMGR for more info.

and the icon never turning to green?

Screenshot 2023-03-26 at 18 24 37

wimleers commented 1 year ago

@bwp91

  1. The support for homebridge-config-ui-x would never result in that sort of configuration as far as I can tell? πŸ˜…
  2. The plugin will never launch unless both an SMA solar inverter is discovered (using mDNS/DNS-SD) and a SMA Home Manager (using a Speedwire multicast subscription, where this device is broadcasting a datagram every second) are found. You likely do not have either device on your network, which is why the plugin takes so long to launch: it keeps trying indefinitely. I'm happy to change that to a limited period of time of course. But: A) the logging should make it pretty clear, B) it's then absolutely pointless to run this plugin.

(Over here it typically takes a few seconds to discover the devices, connect, and then launch.)

bwp91 commented 1 year ago

Hi @wimleers

Re (1) - there are still plenty of users who configure stuff directly with JSON, so we just need to make sure that any 'config issues' are caught properly by its plugin - to make sure that it does not send homebridge into a crash loop and therefore not stop any other plugin from functioning. homebridge-config-ui-x has made things so much easier for a lot of users, but please don't assume that every user will be using this.

Re (2) - okay πŸ‘ - no action needed from you, thanks for explaining

wimleers commented 1 year ago

Makes sense, will address that!

bwp91 commented 1 year ago

Great! Once ready, please reply on here with a comment just saying /check and we can try again πŸ‘

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wimleers commented 1 year ago

I will follow up on this in the coming week.

wimleers commented 1 year ago

I can't seem to find documentation on what the recommended pattern is for when the configuration is not valid, in either of these places:

So I went with throw new Error with a helpful message. See https://github.com/wimleers/homebridge-sma-home-manager/commit/1b00ef68eee2e94afbd691ebeabd5961016f18ba.

Released in https://github.com/wimleers/homebridge-sma-home-manager/releases/tag/1.1.4.

wimleers commented 1 year ago

/check

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

Hi @wimleers thanks for trying to sort this out.

The issue remains that you are throwing an error here. In my case I just installed your plugin via the homebridge-config-ui-x, and when the config screen appeared I just clicked save straight away (and then enabled child bridge). This results in a config of:

        {
            "signals": {
                "offGrid": true,
                "noSun": true,
                "highImport": false
            },
            "platform": "SMAHomeManager",
            "_bridge": {
                "username": "0E:16:95:1D:EA:89",
                "port": 40942
            }
        }

This is sending homebridge into a crash loop with this error:

[12/06/2023, 23:25:50] [homebridge-sma-home-manager] Restarting Process...
[12/06/2023, 23:25:52] [homebridge-sma-home-manager] Launched child bridge with PID 9802
[12/06/2023, 23:25:52] Registering platform 'homebridge-sma-home-manager.SMAHomeManager'
[12/06/2023, 23:25:52] [homebridge-sma-home-manager] Loaded homebridge-sma-home-manager v1.1.4 child bridge successfully

/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:45
        ['signals', 'surplusSignals'].forEach(requiredTopLevelKey => {
                               ^
Error: Invalid configuration: surplusSignals key is missing. See config.schema.json.
    at /usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:47:10
    at Array.forEach (<anonymous>)
    at new SMAHomeManager (/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:45:32)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:150:42)
[12/06/2023, 23:25:52] [homebridge-sma-home-manager] Child bridge process ended
[12/06/2023, 23:25:52] [homebridge-sma-home-manager] Process Ended. Code: 1, Signal: null
[12/06/2023, 23:25:59] [homebridge-sma-home-manager] Restarting Process...

I appreciate there are not any best practice guides with what to do in the case that a user has an invalid config.

What I personally do with my plugins is just to log a warning that the config is invalid and stop the plugin from loading any more.

For example in your code here:

https://github.com/wimleers/homebridge-sma-home-manager/blob/5a3bd375e33f42165fe62bb1b79b21bcd486e001/index.js#L47

I would simply use a return statement

log.warning('invalid config etc');
return;

so that the user (assuming they look at the logs) can see that there is an issue with the config, but by not throwing an error, this does not send homebridge into a crash loop and so does not affect other plugins.

wimleers commented 1 year ago

Will do. Thanks for taking the time to elaborate!

wimleers commented 1 year ago

Done: https://github.com/wimleers/homebridge-sma-home-manager/commit/8022bfe4361301cf318a3ea5bd1a259e74360542

Released: https://github.com/wimleers/homebridge-sma-home-manager/releases/tag/1.1.5

/check

wimleers commented 1 year ago

/check

github-actions[bot] commented 1 year ago

The following pre-checks failed:

:x: Failed to test plugin. :x: Command failed: docker build -t check .

Comment /check to run checks again.

wimleers commented 1 year ago

Ah, this time there's no bot involved? πŸ˜… πŸ™ˆ Sorry about that!

EDIT: looks like GitHub was just malfunctioning 🀷 None of the github-actions stuff appeared when I wrote this!

wimleers commented 1 year ago

/check

github-actions[bot] commented 1 year ago

The following pre-checks failed:

:x: Failed to test plugin. :x: Command failed: docker build -t check .

Comment /check to run checks again.

bwp91 commented 1 year ago

/check

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

You probably meant log.warn in your code instead of log.warning 😁

Still getting a (new logging) crash loop:

[16/06/2023, 20:33:59] [homebridge-sma-home-manager] Loaded homebridge-sma-home-manager v1.1.5 child bridge successfully

/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:45
        ['signals', 'surplusSignals'].forEach(requiredTopLevelKey => {
                               ^
TypeError: log.warning is not a function
    at /usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:47:8
    at Array.forEach (<anonymous>)
    at new SMAHomeManager (/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:45:32)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:150:42)
wimleers commented 1 year ago

LOL! I didn't test it this time, I just literally copy/pasted what you wrote at https://github.com/homebridge/verified/issues/527#issuecomment-1588197545 πŸ˜…

Will fix.

bwp91 commented 1 year ago

Ah sorry! 😁

wimleers commented 1 year ago

Done: https://github.com/wimleers/homebridge-sma-home-manager/releases/tag/1.1.6

wimleers commented 1 year ago

/check

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

Do you have some time to message-chat on the homebridge discord?

https://discord.gg/kqNCe2D

If you're already on there, please send me a dm, my username is bwp91!

wimleers commented 1 year ago

I’m not on Discord πŸ˜… Does email also work?

bwp91 commented 1 year ago

Email is good it's bwp91@icloud.com

bwp91 commented 1 year ago

Hi @wimleers

I think the issue here is the return statement isn't doing anything since it is in the callback of a foreach

    ['signals', 'surplusSignals'].forEach(requiredTopLevelKey => {
        if (!Object.keys(config).includes(requiredTopLevelKey)) {
            log.warn(`Invalid configuration: ${requiredTopLevelKey} key is missing. See config.schema.json.`);
            return;
        }
    });

basically, even if there is an invalid config, it is logging, returning inside this function, but then still continuing with the rest of the plugin initialisation.

Since there are only two 'keys' here, it may be easier just to

if (!Object.keys(config).includes('signals')) {
  log.warn(`Invalid configuration: signals key is missing. See config.schema.json.`);
  return;
}
if (!Object.keys(config).includes('surplusSignals')) {
  log.warn(`Invalid configuration: surplusSignals key is missing. See config.schema.json.`);
  return;
}

or a for loop, or a try/catch.

But at the moment that return; statement is redundant and even if a config item is missing, the logwarning is processed, but the rest of the plugin init continues

bwp91 commented 1 year ago

Hi @wimleers any update on this?

wimleers commented 1 year ago

Sorry, I was enjoying https://gentsefeesten.stad.gent/en at the time!

Will sort this out in the next few days πŸ‘

wimleers commented 1 year ago

returning inside this function, but then still continuing with the rest of the plugin initialisation.

OMG 🀦 πŸ™ˆ

Fixed: https://github.com/wimleers/homebridge-sma-home-manager/commit/9b914c6e6c3b9e1d349f8510302ad6288d040e80.

Released: https://github.com/wimleers/homebridge-sma-home-manager/releases/tag/1.1.7

wimleers commented 1 year ago

/check

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

Just wanted to pick up on one thing, this is nothing to do with verified or not!

Screenshot 2023-08-16 at 22 20 10

after installing plugin I see this screen, the recommendations link doesn't work, it opens a new window to

http://homebridge.local/#SMAHomeManager-recommendations

which basically just shows the home page of homebridge ui

bwp91 commented 1 year ago

With regards to verification, on a new install of this plugin, after clicking Save on screenshot above, setting up as a child bridge and restarting homebridge I am getting this constant restart:

[16/08/2023, 22:23:14] [homebridge-sma-home-manager] Invalid configuration: surplusSignals key is missing. See config.schema.json.

/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285
                const liveService = new Service.CustomPowerMonitor();
                      ^
TypeError: Service.CustomPowerMonitor is not a constructor
    at SMAHomeManager.accessories (/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285:23)
    at /usr/local/lib/node_modules/homebridge/src/bridgeService.ts:534:24
    at new Promise (<anonymous>)
    at BridgeService.loadPlatformAccessories (/usr/local/lib/node_modules/homebridge/src/bridgeService.ts:528:12)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:155:36)
[16/08/2023, 22:23:14] [homebridge-sma-home-manager] Child bridge process ended
[16/08/2023, 22:23:14] [homebridge-sma-home-manager] Process Ended. Code: 1, Signal: null
[16/08/2023, 22:23:21] [homebridge-sma-home-manager] Restarting Process...
[16/08/2023, 22:23:23] [homebridge-sma-home-manager] Launched child bridge with PID 21925
[16/08/2023, 22:23:23] Registering platform 'homebridge-sma-home-manager.SMAHomeManager'
[16/08/2023, 22:23:23] [homebridge-sma-home-manager] Loaded homebridge-sma-home-manager v1.1.7 child bridge successfully
[16/08/2023, 22:23:23] [homebridge-sma-home-manager] Invalid configuration: surplusSignals key is missing. See config.schema.json.

/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285
                const liveService = new Service.CustomPowerMonitor();
                      ^
TypeError: Service.CustomPowerMonitor is not a constructor
    at SMAHomeManager.accessories (/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285:23)
    at /usr/local/lib/node_modules/homebridge/src/bridgeService.ts:534:24
    at new Promise (<anonymous>)
    at BridgeService.loadPlatformAccessories (/usr/local/lib/node_modules/homebridge/src/bridgeService.ts:528:12)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:155:36)
[16/08/2023, 22:23:23] [homebridge-sma-home-manager] Child bridge process ended
[16/08/2023, 22:23:23] [homebridge-sma-home-manager] Process Ended. Code: 1, Signal: null
[16/08/2023, 22:23:30] [homebridge-sma-home-manager] Restarting Process...
[16/08/2023, 22:23:31] [homebridge-sma-home-manager] Launched child bridge with PID 21939
[16/08/2023, 22:23:31] Registering platform 'homebridge-sma-home-manager.SMAHomeManager'
[16/08/2023, 22:23:31] [homebridge-sma-home-manager] Loaded homebridge-sma-home-manager v1.1.7 child bridge successfully
[16/08/2023, 22:23:31] [homebridge-sma-home-manager] Invalid configuration: surplusSignals key is missing. See config.schema.json.

/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285
                const liveService = new Service.CustomPowerMonitor();
                      ^
TypeError: Service.CustomPowerMonitor is not a constructor
    at SMAHomeManager.accessories (/usr/local/lib/node_modules/homebridge-sma-home-manager/index.js:285:23)
    at /usr/local/lib/node_modules/homebridge/src/bridgeService.ts:534:24
    at new Promise (<anonymous>)
    at BridgeService.loadPlatformAccessories (/usr/local/lib/node_modules/homebridge/src/bridgeService.ts:528:12)
    at ChildBridgeFork.startBridge (/usr/local/lib/node_modules/homebridge/src/childBridgeFork.ts:155:36)
[16/08/2023, 22:23:31] [homebridge-sma-home-manager] Child bridge process ended
[16/08/2023, 22:23:31] [homebridge-sma-home-manager] Process Ended. Code: 1, Signal: null
wimleers commented 1 year ago

So it’s still not working when not configured correctly.

I’ll do a separate Homebridge install just to test that edge case. But I’m kinda baffled because I did exactly what you asked πŸ˜…πŸ˜ž

Sorry I’m taking up so much of your time. Definitely the opposite of my intent, because I too am an open source software maintainer, although in a very different software world.

bwp91 commented 1 year ago

@wimleers no problem at all sometimes unblocking one issue just leads to another, very used to this and all good πŸ‘πŸ»

bwp91 commented 1 year ago

@wimleers i was just having a look at the line of code that was causing the issue and im afraid when it comes to custom services and characteristics I am a bit clueless πŸ˜…

wimleers commented 1 year ago

Hah! I get that. It's a bit of a maze, but they are so very powerful! 😊 It works great in Eve, Home+, Controller, etc. Really unfortunate that the official Home app is so incredibly limited :(

bwp91 commented 1 year ago

custom characteristics are common with (verified) plugins, I personally have implemented these in my plugins that are verified. So it's not a show-stopper, in your case it just seems to be an issue with implementation.

Have you tried wiping your accessory cache and starting from new?

Custom characteristics don't normally, and should not, affect a standard homebridge user who only has the home app

wimleers commented 1 year ago

This weekend I'll get this done πŸ‘

Custom characteristics don't normally, and should not, affect a standard homebridge user who only has the home app

Agreed! All I meant to say was: the Home app does not allow setting up automations based on custom characteristics.

wimleers commented 1 year ago

The custom service/characteristic was a mere distraction. The validation warning caused an early return in the constructor, which caused the custom service to not be defined yet. Because despite the early return, HomeBridge will still call accessories().

I first added checks to have that return an empty list of accessories if the config was invalid.

But … there's a much, much simpler solution I should've adopted months ago: adding fallback logic to just allow no config whatsoever. I can't believe I didn't think of this months ago. 99% of the logic to deal with that was already in place! πŸ™ˆ

So the fix now is A) deleting the silly validation logic, B) adding || [] β€”Β really, that's it πŸ˜…

There you go: https://github.com/wimleers/homebridge-sma-home-manager/releases/tag/1.1.8

wimleers commented 1 year ago

/check

github-actions[bot] commented 1 year ago

:white_check_mark: Pre-checks completed successfully.

bwp91 commented 1 year ago

deleting the silly validation logic im sure this was my 'silly validation logic' right?!

One question I have is why the long plugin name like homebridge-sma-home-manager, and why not something shorter like homebridge-sma?

I'm not familiar with the sma brand, but does your plugin generally support all their devices?

wimleers commented 1 year ago

but does your plugin generally support all their devices?

No, it doesn't.

I'd say >90% of SMA users are consumers with a solar (photovoltaic) inverter of theirs: https://www.sma.de/en/products/solar-inverters

The remaining 10% is probably divided between:

For all of the above, the SMA Home Manager (https://www.sma.de/en/products/monitoring-control/sunny-home-manager) is an optional add-on. But only with this do you get correct/complete production & consumption information. And only this broadcasts it in real time (one UDP packet per second, broadcast via IP multicast): Watt produced and consumed.

That's why I specifically named it "SMA Home Manager", because without it, this plugin is useless. It does also specifically talk to the solar inverter, but only for: live A and V (and cumulative production today, whether there is a fault, etc). But … this happens via ModBus, which does not support reading data once per second 😞 Even once per 10 seconds will cause problems! The original repo (https://github.com/codyc1515/homebridge-sma-inverter) from which I forked was barely maintained and very buggy, but did not require the SMA Home Manager: it supported only solar inverters.

So:

  1. this definitely does not work for all SMA devices: it requires an SMA Home Manager (which most don't have) and an SMA solar inverter
  2. there's an issue for adding support for battery inverters: https://github.com/wimleers/homebridge-sma-home-manager/issues/5 β€” but without access to a device this is hard to implement
  3. there's also an issue for hybrid inverters: https://github.com/wimleers/homebridge-sma-home-manager/issues/13 β€” same problem, but at least here the "production" part (rather than "battery storage" part) should work fine in principle: the same ModBus registers should be used, but … we already learned the hard way that that is not the case :/ So, access to hardware really is critical…
  4. but even if I'd implement 2 + 3 after I got access to hardware … this would still require an SMA Home Manager, because only then can you get a) a complete picture of the power flow, b) realtime updates

(I bet that was more information you were hoping for? πŸ˜… )

bwp91 commented 1 year ago

A yes or no would have been sufficient for me (!) but actually your extended response will be useful for a paper trail on here, so thank you for the detail, it's good for reference. Especially if there is ever a future sma plugin that wants to verify.

I am just personally on a bit of a mission at the moment to try and keep plugins with simple names where appropriate.