add sonnen-charger to latest

[ChrisWbb]

https://github.com/ChrisWbb/ioBroker.sonnen-charger

[mcm1957]

First of all - THANK YOU for the time and effort you spend to maintain this adapter.

I would like to give some (preliminary) feedback based on my personal oppinion. This is NOT an offical review and @Apollon77 might have several additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him (or wait for a response from him) if you cannot follow my suggestsions or before you spend major effort.

• Readme.md is very short

  If you have some spare time, please consider adding some more infos to readme, especially a clear description what device(s) this adapter is for. Add a link to the manufactorers website i.e. Also a short description what kind of information / manipulation is possible.

  If some documentation is useful for users add it to Readme.md or create a description below docs directory and ass a link to Readme.md.

• Field _id:chargerSettings.serialNumber seems to be duplicated at io-package.json
  https://github.com/ChrisWbb/ioBroker.sonnen-charger/blob/main/io-package.json#L168
  https://github.com/ChrisWbb/ioBroker.sonnen-charger/blob/main/io-package.json#L181

• Check onStateChange handling
  It looks like your onStateChange code does not respect the ack flag.
  Normally Adaptercode should only react on state changes issued with ack=false and ignore state changes with ack=true as long as it does not want to react on vales set by other adapters etc. Users should enter commands / set states with ack=false.
  See https://forum.iobroker.net/topic/61876/best%C3%A4tigt-acknowledged-flag-bedeutung-ein-mysterium?_=1680075857780 and eventually discuss this with @Apollon77

• consider using adapter.setTimeout / this.setTimeout instead of (standard) setTimeout

  The adapter package provides wrapper routines for native setTimeout, setInterval, clearTimeout and clearInterval. Using those routines ensures that timers are cancelled on onLoad. Additional checks on those limits might be performed, too. So consider replacing native setTimeout/clearTimeout by adapter.setTimeout/adapter.clearTimeout or this.setTimeout/this.clearTimeout. The same refers to setInterval/clearInterval.

Thanks for reading and evaluating these suggestions.
McM1957

[GioElCapitano98]

Hello,
i want to test the adapter for the sonnen charger too.
But i dont't find the adapter in iobroker.
where can i download the adapter and how can i install the sonnen charger adapter on the rpi 4 / iobroker ?

[mcm1957]

Adapter repository: https://github.com/ChrisWbb/ioBroker.sonnen-charger

Currently only available via github installation in expert mode.
Thios adapter is not yet release as stable in repositories.
If you need further Instructions please ask at forum (forum.iobroker.net) or contact developer via adapter repository.
This PR is not the place to discuss adapter usage, issues or feature requests.

[mcm1957]

@ChrisWbb
You entered the following url at sources-dist.json:
"icon": "https://raw.githubusercontent.com/ChrisWbb/ioBroker.sonnen-charger/master/admin/sonnen.png",

But the file is named sonnen-charger.png - at least now.

Please correct PR

[mcm1957]

"sonnen-charger": {
"meta": "https://raw.githubusercontent.com/ChrisWbb/ioBroker.sonnen-charger/master/io-package.json",
"icon": "https://raw.githubusercontent.com/ChrisWbb/ioBroker.sonnen-charger/master/admin/sonnen.png", <<< wrong filename
"type": "energy"
},

@ChrisWbb - please fix PR, filename is OK as the picture normally is named identical to adapter

[Apollon77]

@mcm1957 But the image naming is no requirement and also not really a best practice ... no need to enforce that. so This is ok fpr me

[mcm1957]

@mcm1957 But the image naming is no requirement and also not really a best practice ... no need to enforce that. so This is ok fpr me

@Apollon77
I noticed that the icon url points to a non existing file. So EITHER the PR / icon url OR the filename need to be corrected.
This causes the github checks to fail (see screenshot) - so I think this must be corrected. It not about the name itself - it might by xtzrcs6if453hjc.jpg too as long as the url point to this file.

Or is this statement incorrect too? In this case sorry for the statements.
McM1957

[Apollon77]

This statement is meaning that the UR is wrong completely ... and the file is not there.

SO yes it needs to be changed to https://raw.githubusercontent.com/ChrisWbb/ioBroker.sonnen-charger/master/admin/sonnen-charger.png ... but not because the file needs to be named like the adapter ... but because the file was not there because it was an copy/paste error :-)

[mcm1957]

I would like to give some additional (preliminary) feedback based on my personal oppinion. This is NOT an offical review and @Apollon77 might have several additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him (or wait for a response from him) if you cannot follow my suggestsions or before you spend major effort.

• reevaluate state roles

  Only the values specified here (https://github.com/ioBroker/ioBroker.docs/blob/master/docs/en/dev/stateroles.md) may be used as state roles. Do not invent new names as this might disturb the functionalyity of other adapters or vis.

  In addition the roles MUST match the read/write functionality. As an example a role value.* requires that the state is a readable state. Input states (write only) should have roles like level.*. Please read the description carefully. States with role 'button' should (must) have attribute 'common.read=false' set. A button ( "Taster" in german only triggers when you press but else has no state), so it should also be not readable. This is also like HomeMatic does it. A switch has clear states true or false and so can be read.

  As an example customMaxCurrent currently has role "indoicator" set. As this state is of type number and has a unit "A" assigned, this state seems to contain a value. So role value.current seesm to be appropiate. "numberOfPhases" is quite sure no indicator either. States with role indicator must have type boolen.

  Please review roles. Those are i.e. used by tapyedetector and vis ans should match standards.

• adapt testing to supported node releases

  Consider adding node 20 tests unless you already know incompatibilities.

  So the recommended testmatrix is [16.x, 18.x, 20.x],

Everything looks good to me.

Thanks for reading and evaluating this suggestions and sorry for the late / addition remarks. Please fix and/or comment I'll set the request to lgtm so that Apollon77 will continue asap.

McM1957

Please add a comment when you have reviewed and fixed the suggestionsor at least commented the suggestions and you think the adapter is ready for a re-review!

[mcm1957]

How shall we continue?
Please let us know if you plan zu fix the noted issues in the near future - or as an alterntive - like to discuss some / all of those points.

[ChrisWbb]

@mcm1957 : thanks for your review. I fixed your findings.

[github-actions]

Automated adapter checker

ioBroker.sonnen-charger

👍 No errors found

• 👀 [W202] Version of package.json (1.1.1) doesn't match latest version on NPM (1.1.0)
• 👀 [W400] Cannot find "sonnen-charger" in latest repository

Add comment "RE-CHECK!" to start check anew

[mcm1957]

@ChrisWbb

Looks goog now.

Please note that release 1.1.1 has not yet been published at npm

[mcm1957]

This adapter has been released to latest repository and should be visible within 24h maximum.

Please create a thread at https://forum.iobroker.net/category/91/tester titled like "Test Adapter " to collect some user feedback and provide a link to this topic when requesting addition to stable repository later.

Note: If an other testing topic already exists, ist OK to continue using this topic too.