rlindner / node-red-contrib-sma-webconnect

Node-RED node to query the web interface of SMA inverters
MIT License
13 stars 9 forks source link

Add support for "SUNNY BOY STORAGE 2.5" and custom configurations #7

Closed HoLo85 closed 4 years ago

HoLo85 commented 4 years ago

Node can now be configured with an input payload to set which values should be read.

rlindner commented 4 years ago

First of all, thanks a lot for taking the time to contribute your changes, really appreciate that!

I've reviewed your changes and came across some points which need to be addressed before we can integrate it.

1. Breaking Changes

The biggest issue here is that the introduced changes break every existing installation. Please adjust the implementation in a manner to ensure the node remains backwards compatible. See the suggestions below.

2. Configuration vs. Payload

You've used the payload passed to the nodes input to configure the node. Thats kind of a misuse, because a nodes input is intended to receive messages to manipulate on. Though in the context of this node the payload passed to the input isn't being evaluated, because it's just used as trigger to query the configured device. But the configuration is something which is static to the node and should be located in the nodes settings.

3. Flexibility vs. ease of use

I understand the approach you have taken to make the node as flexible as possible to query all kinds of values from different devices without having the need to adjust the nodes source. But I doubt that's the path the majority of the users will take. I don't want to require every user to inspect the web interface and find the correct ids and build a valid JSON configuration before being able to use the node. In addition to that it's not save to assume, every value you can query for by ID is of type number, in some cases they're e.g. of type string and so cannot be divided.

Suggestions:

To better understand if this is a viable solution for your use case it would be helpful if you could provide a complete example input and output you're using with your modified node.

HoLo85 commented 4 years ago

Ok, ich habe einige der Vorschläge umgesetzt damit der Node mit bestehenden Installationen kompatibel ist. Die Geräte können nun auch mittels Dropdown vorausgewählt werden, allerdings bevorzuge ich weiterhin den Ansatz den Node mit einem Input Payload zu konfigurieren. Ich verwende den Input Payload aktuell auch nur da ich bisher keine Möglichkeit gefunden habe den Editor in einen Custom Dialog zu integrieren.

Ok, I have implemented some of the suggestions so that the node stays compatible with existing installations. The devices can now also be preselected via dropdown, but I still prefer to configure the node with an input payload. I am currently using the Input Payload only because I have not yet found a way to use the integrated editor in a custom dialog.

rlindner commented 4 years ago

First of all, please understand I'd like to keep all the conversations here in english to keep the project including it's issues as accessible as possible.

Then sorry for the late response, I'd hoped to have some time to look into your issue you've mentioned in your last sentence, but unfortunately that didn't happen to be the case.

I'm still reluctant to use the payload to configure the node. The problem I have with that is, that I do not want to provide such a flexible interface to the node to the user. Once it's there, somebody will use it. So I have to support it, I can't remove it, nor can I change the payloads format without breaking any existing flows. I think I may be able to arrange with such a functionality only, if it's part of the nodes settings and hidden behind a clear notice of some kind of experimental/developer/debug mode.

HoLo85 commented 4 years ago

grafik This would be my suggestion for such a functionallity, i already implented it with the updated PR but made the description a bit clearer.

I understand that it is inconvenient to support such a function with the various devices offered by SMA. But the current implementation limits the readout to the predefined values only and I think that some users would like to read out more data from their devices.

gabrio79 commented 4 years ago

another idea could be the possibility to define timeout...

rlindner commented 4 years ago

@HoLo85 The solution with a third drop-down option would be OK for me. 👍

If the only reason for using the input payload is the integrated editor with JSON support and that's not possible in the node settings, then I would go with a plain text area in the node properties. I understand that's not so convenient, but I hope that's a tradeoff we can both live with. I suppose anyone providing his own configuration should be able to write valid JSON without the editor anyway. Maybe it would be helpful to link to an example config then as well?

Screenshot

If we can agree on that, I'd be happy to integrate your changes.

HoLo85 commented 4 years ago

To be honest i don't really like the idea, why shouldn't i used something that works and is already in Node Red. Also i don't really see a problem in using a input payload to configure the node. I think the hint should be good enough for users to know that they must provide a message if they want to use "Debug" mode. What i would do is adding a link to the gibt hub page with an example config.

HoLo85 commented 4 years ago

pr updtd

rlindner commented 4 years ago

I'm still not too happy about the "hijacking" of the input, but in the end I'm okay with it, as it's under the specific key "sma_config" now. Thanks again for your contribution! 🚀