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

homebridge-sma-home-manager #527

Closed
wimleers opened this issue Mar 26, 2023 · 66 comments
Closed

homebridge-sma-home-manager #527

wimleers opened this issue Mar 26, 2023 · 66 comments
Labels
verified use when a plugin meets the criteria - adds the verified badge text

Comments

@wimleers
Copy link

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

@wimleers wimleers added the pending the label given to a new verification/icon request label Mar 26, 2023
@github-actions
Copy link

✅ Pre-checks completed successfully.

@bwp91
Copy link
Contributor

bwp91 commented Mar 26, 2023

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
Copy link
Author

wimleers commented Mar 28, 2023

@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
Copy link
Contributor

bwp91 commented Apr 1, 2023

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

@bwp91 bwp91 added the awaiting-user-reply use after review has started - awaiting user to reply to a comment label Apr 1, 2023
@wimleers
Copy link
Author

wimleers commented Apr 9, 2023

Makes sense, will address that!

@bwp91
Copy link
Contributor

bwp91 commented Apr 12, 2023

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

@bwp91 bwp91 added awaiting-changes use after review has started - awaiting user to make changes to plugin and removed pending the label given to a new verification/icon request awaiting-user-reply use after review has started - awaiting user to reply to a comment labels Apr 12, 2023
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale used for stale issues label May 13, 2023
@wimleers
Copy link
Author

I will follow up on this in the coming week.

@github-actions github-actions bot removed the stale used for stale issues label May 16, 2023
wimleers added a commit to wimleers/homebridge-sma-home-manager that referenced this issue May 28, 2023
@wimleers
Copy link
Author

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 wimleers/homebridge-sma-home-manager@1b00ef6.

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

@wimleers
Copy link
Author

/check

@github-actions github-actions bot added pending the label given to a new verification/icon request and removed awaiting-changes use after review has started - awaiting user to make changes to plugin labels May 28, 2023
@github-actions
Copy link

✅ Pre-checks completed successfully.

@bwp91
Copy link
Contributor

bwp91 commented Jun 12, 2023

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.

@bwp91 bwp91 added awaiting-user-reply use after review has started - awaiting user to reply to a comment and removed pending the label given to a new verification/icon request labels Jun 12, 2023
@wimleers
Copy link
Author

Will do. Thanks for taking the time to elaborate!

wimleers added a commit to wimleers/homebridge-sma-home-manager that referenced this issue Jun 14, 2023
@wimleers
Copy link
Author

@wimleers
Copy link
Author

/check

@github-actions github-actions bot added the pending the label given to a new verification/icon request label Jun 14, 2023
@github-actions
Copy link

The following pre-checks failed:

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

Comment /check to run checks again.

@bwp91 bwp91 added pending the label given to a new verification/icon request and removed awaiting-user-reply use after review has started - awaiting user to reply to a comment labels Sep 4, 2023
@bwp91
Copy link
Contributor

bwp91 commented Sep 4, 2023

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
Copy link
Author

wimleers commented Sep 5, 2023

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: Battery Inverter ("Sunny Island") support wimleers/homebridge-sma-home-manager#5 — but without access to a device this is hard to implement
  3. there's also an issue for hybrid inverters: Support for hybrid inverters such as the Sunny Tripower Smart Energy wimleers/homebridge-sma-home-manager#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
Copy link
Contributor

bwp91 commented Sep 6, 2023

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.

@bwp91
Copy link
Contributor

bwp91 commented Sep 6, 2023

So I have installed the latest version of your plugin, unfortuntately all I seem to get this this:

Screenshot 2023-09-06 at 23 46 39

and checking the status of my child-bridges:

Screenshot 2023-09-06 at 23 48 37

At no point does the circular icon turn to green.

@bwp91 bwp91 added awaiting-user-reply use after review has started - awaiting user to reply to a comment and removed pending the label given to a new verification/icon request labels Sep 7, 2023
@wimleers
Copy link
Author

(Back from vacation.)

It doesn't crash. It doesn't find the necessary devices. Why would it make sense to have the circular icon to turn green, if none of the required devices are present?

The log entries are crystal clear, I think?

If I allow this to launch when no devices are discovered, that would indeed result in a nice "green circular icon". But it'd have one disastrous consequence in case of an ephemeral networking problem that'd prevent the devices from being discovered (e.g. multicasting broke due to router/switch settings changes): HomeKit would think the accessories have been removed, and would automatically also remove them from all automations.

(I know this because I had it happen early on during development. 😨)

I'd much rather have my Homebridge plugins tell me explicitly that the necessary devices are missing and refuse to boot than to potentially quietly mess up my carefully curated automations!

@wimleers
Copy link
Author

/check

@github-actions github-actions bot added the pending the label given to a new verification/icon request label Sep 17, 2023
@github-actions
Copy link

✅ Pre-checks completed successfully.

@bwp91 bwp91 removed the awaiting-user-reply use after review has started - awaiting user to reply to a comment label Sep 18, 2023
@bwp91
Copy link
Contributor

bwp91 commented Sep 21, 2023

Hi @wimleers I appreciate your response.

For platform type plugins, the aim of the green light in the homebridge ui is that that plugin has successfully loaded and started. Not necessarily that it has found every accessory that it is expecting.

We are trying to avoid the log message of

plugin taking a long time to load and preventing homebridge from starting

Are you on the Homebridge discord group? Happy to have a 1:1 message about this

@wimleers
Copy link
Author

that that plugin has successfully loaded and started

But if the required hardware is absent or temporarily inaccessible over the network, how does it make sense to make the plugin conclude "success!" and omit the yet-to-be-connected-to accessory, which then results in all automations getting broken? 🙃

🤔 The only alternative I see to fulfill both needs (1. avoid messing up users' automations, 2. always starting the plugin) is to only try for ~30 seconds and if it's not found by then, to just expose the accessories anyway but make them do nothing.

I'll make that happen. 👍

@bwp91
Copy link
Contributor

bwp91 commented Sep 22, 2023

Before you start making these changes let me have a discussion with some other maintainers.

I do see your side of this too 😃

@bwp91
Copy link
Contributor

bwp91 commented Sep 28, 2023

Hi @wimleers

We believe that you are facing the accessory-resetting option because you are not making use of the homebridge cache accessory feature that platform-based plugins should use.

I will reply again with more info...

@bwp91
Copy link
Contributor

bwp91 commented Sep 30, 2023

I know I have mentioned the discord thing before, i would really like to help you get this plugin looking great, discord is basically like 'slack' but a homebridge community, and totally happy to have a 1:1 chat with you to explain about cached accessories.

I would find this much easier than trying to let it all out in one github post!!

A signup link is here https://discord.gg/kqNCe2D and once you have registered you can just find me and send me a DM my username is bwp91.

@wimleers
Copy link
Author

wimleers commented Oct 9, 2023

Why do this in real-time chat when none of us are likely to be online at the same time? And if we'd communicate async there, then how is it any better from discussing it here?

IOW: why not use this very GitHub issue? I'm used to seeing dozens or hundreds of comments on issues/tickets/PRs/MRs (I work on open source PHP software for my day job), because that results in linkable, searchable discussions that allow understanding why certain decisions were made years later.

That'd allow you to point to the discussion/guidance you'd provide here rather than having to do it all over again for somebody else.


We believe that you are facing the accessory-resetting option because you are not making use of the homebridge cache accessory feature that platform-based plugins should use.

No, I'm facing that because I followed the developer docs at https://github.com/homebridge/homebridge-examples#platform-plugins, which says:

Static platforms know which accessories they want to expose on start up. The set of accessories cannot change over the lifespan of the plugin.

That's a perfect fit for my use case! It is impossible for an additional PV inverter or energy manager to appear out of nowhere. IOW: there is no need for dynamic accessory discovery.

But you're now telling me that I should not expose an accessory that I do not know for certain will have a successful connection. AFAICT that implies that one should never use a static platform? If so, why is it in the official examples to this day?


At this point, it honestly feels rather arbitrary to not grant my plugin the "Verified" stamp. My plugin boots even when you do not have the necessary hardware, and it gives you actionable, precise feedback. Besides, the only reason it does not boot for you is because you don't have the hardware.

If I install an app on my smart phone that requires certain hardware to be connected, you also expect it to launch and inform you, right? Isn't this similar to that?

That's far better than most verified Homebridge plugins I've used. Just now, I was trying https://github.com/produdegr/homebridge-3em-energy-meter, which is also verified. This is the experience for that:
IMG_0938

(You can see that it connected, received a response, but is just endlessly failing at parsing the response.)

I've had similarly bad experiences with:


I'm definitely willing to make the changes mentioned in #527 (comment).

But refactoring from static to dynamic platform is wrong AFAICT. I tried to follow the prescribed best practices when I refactored the unmaintained & unreliable https://github.com/codyc1515/homebridge-sma-inverter project (but thankfully somebody built that! Otherwise getting started would've been much harder!) into something that works reliably.

The examples still list it, so surely it still is a recommended approach?

I'd really like that "Verified" badge to encourage more people to save energy in their home. I can't believe I've been trying this for >6 months at this point. 😞

As a fellow open source maintainer, I applaud your relentless attention to the end user experience! 😊 👏 But AFAICT this plugin is already more reliable than most plugins I've tried. With more precise guidance. With clearer docs. With actual responses from the maintainer. So … why is it not good enough?

@bwp91
Copy link
Contributor

bwp91 commented Oct 11, 2023

Why do this in real-time chat when none of us are likely to be online at the same time? And if we'd communicate async there, then how is it any better from discussing it here?
IOW: why not use this very GitHub issue? I'm used to seeing dozens or hundreds of comments on issues/tickets/PRs/MRs (I work on open source PHP software for my day job), because that results in linkable, searchable discussions that allow understanding why certain decisions were made years later.
That'd allow you to point to the discussion/guidance you'd provide here rather than having to do it all over again for somebody else.

Point taken I won't ask again!

No, I'm facing that because I followed the developer docs at https://github.com/homebridge/homebridge-examples#platform-plugins, which says:

Understood. We are trying to urge developers to always use dynamic-platform-type plugins. About a month ago I updated the homebridge wiki to just have links to the dynamic-platform type template as well as the camera-plugin template.
https://github.com/homebridge/homebridge/wiki
It's a gradual process. And whilst I took the links away from the homebridge-examples plugin templates, I didn't want to delete all the repos under this name right now.


Sorry it has been a lot of to-ing and fro-ing, but I really am taking into account now what you have said in your last reply, and this verification process in this case I totally understand has been very frustrating for you. This really was never my intention 😅


let me re-run the checks and have another ponder...

@bwp91
Copy link
Contributor

bwp91 commented Oct 12, 2023

/check

@github-actions
Copy link

✅ Pre-checks completed successfully.

@bwp91 bwp91 added the reviewed label Oct 12, 2023
@github-actions
Copy link

github-actions bot commented Oct 12, 2023

  • - The plugin must successfully install.
  • - The plugin must implement the Homebridge Plugin Settings GUI.
  • - The plugin must not start unless it is configured.
  • - The plugin must be of type dynamic platform. (N/A)
  • - The plugin must not offer the same nor less functionality than that of any existing verified plugin.
  • - The plugin must not execute post-install scripts that modify the user's system in any way.
  • - The plugin must not contain any analytics or calls that enable you to track the user.
  • - The plugin must not throw unhandled exceptions, the plugin must catch and log its own errors.
  • - The plugin must be published to npm and the source code available on GitHub.
  • - A GitHub release should be created for every new version of your plugin, with patch notes.
  • - The plugin must run on all Active LTS versions of Node.js, at the time of writing this is Node v18 and v20.
  • - The plugin must not require the user to run Homebridge in a TTY or with non-standard startup parameters, even for initial configuration.
  • - If the plugin needs to write files to disk (cache, keys, etc.), it must store them inside the Homebridge storage directory.

Everything Looks Good!

@bwp91 bwp91 added verified use when a plugin meets the criteria - adds the verified badge text and removed pending the label given to a new verification/icon request reviewed labels Oct 12, 2023
@github-actions
Copy link

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 ❤️ Donate button on its tile in the Homebridge UI. See https://github.com/homebridge/homebridge/wiki/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.

Don't forget to join the official Homebridge Discord server, where plugin developers can get tips and advice from other developers and the Homebridge project team in the #plugin-development channel!

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

@rofl0815
Copy link

Hi, i have the latest homebridge 1.6.1 with node.js version 18.16.0 running on a raspberry pi 4 with Buster.

I tried to install homebridge-sma-home-manager v1.1.8 (2023-09-03) and get the following error messages

...
make: Verzeichnis „/usr/local/lib/node_modules/homebridge-sma-home-manager/node_modules/@serialport/bindings/build“ wird betreten
  CXX(target) Release/obj.target/bindings/src/serialport.o
g++: error: unrecognized command line option ‘-std=gnu++17’
make: *** [bindings.target.mk:114: Release/obj.target/bindings/src/serialport.o] Fehler 1
make: Verzeichnis „/usr/local/lib/node_modules/homebridge-sma-home-manager/node_modules/@serialport/bindings/build“ wird verlassen
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:203:23)
gyp ERR! stack     at ChildProcess.emit (node:events:513:28)
gyp ERR! stack     at ChildProcess._handle.onexit (node:internal/child_process:291:12)
gyp ERR! System Linux 4.19.97-v7l+
gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /usr/local/lib/node_modules/homebridge-sma-home-manager/node_modules/@serialport/bindings
gyp ERR! node -v v18.16.0
gyp ERR! node-gyp -v v9.3.1
gyp ERR! not ok 
npm ERR! code 1
npm ERR! path /usr/local/lib/node_modules/homebridge-sma-home-manager/node_modules/@serialport/bindings
npm ERR! command failed
npm ERR! command sh -c prebuild-install --tag-prefix @serialport/bindings@ || node-gyp rebuild

npm ERR! A complete log of this run can be found in: /home/homebridge/.npm/_logs/2023-10-21T13_40_04_404Z-debug-0.log

Do i do something wrong? Thank you.

@bwp91 bwp91 closed this as completed Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verified use when a plugin meets the criteria - adds the verified badge text
Projects
None yet
Development

No branches or pull requests

3 participants