Add new adapter ioBroker.rmb-bhkw to latest repository

[satchafunkilus]

No description provided.

[Apollon77]

Meeh, ok the resorted adapters break the detection :-)

Results from a manual check at https://adapter-check.iobroker.in/

• [E125] External icon not found under URL: https://raw.githubusercontent.com/satchafunkilus/ioBroker.rmb-bhkw/main/admin/rmb-bhkw.png
• [E201] Bluefox was not found in the collaborators on NPM!. Please execute in adapter directory: "npm owner add bluefox iobroker.rmb-bhkw"

[satchafunkilus]

@Apollon77

• Regarding the external icon: I have followed the folder structure by the adapter creation script and the external icon is located under ioBroker.rmb-bhkw/admin/rmb-bhkw.png. Is there any specific reason why this should be in an additional "main" subfolder?
• I have added Bluefox as owner, already some days ago. It is showing as invitation pending in npm.

[Apollon77]

Is there any specific reason why this should be in an additional "main" subfolder?

The URLs of the Github "raw content" includes the branch!!

[satchafunkilus]

@Apollon77 ok, since the branch is called master and not main, the icon is located under: https://raw.githubusercontent.com/satchafunkilus/ioBroker.rmb-bhkw/master/admin/rmb-bhkw.png. Do I need to rename the branch to main?

[Apollon77]

No, then lets consider it as false positive

[satchafunkilus]

No, then lets consider it as false positive

Ok, understood. Is there any other to-do then on my end?

[Apollon77]

No, I will do a review soon

[Garfonso]

@Apollon77 I'll try a review here.

[Apollon77]

Hi,

here some review comments:

• Must: An unmodified schedule adapter that queries data from external systems will produce a lot of issues because usually the default schedule stays unchanged for all users and so all installations out there will then contact the server at the exact same time create a "potentially huge" request spike. The best practice for now is at least to add a random delay to spread the calls within that minute - or even move the "minutes" on first start and so persist that (e.g. see https://github.com/ioBroker/ioBroker.yr/blob/master/main.js#L84-L90) ... if the adapter gets successfull then we ahve an big issue and the webpage owner contacting us ... we should prevent that from the beginning.
• Hint: You can add the required apt-packages in io-package like https://github.com/ioBroker/ioBroker.phantomjs/blob/master/io-package.json#L124-L128 and iobroker will try to install them on install of the adapter
• Please remove the onStateChange handler because not needed
• You should await https://github.com/satchafunkilus/ioBroker.rmb-bhkw/blob/master/main.js#L154, else you might stop too soon!
• I think https://github.com/satchafunkilus/ioBroker.rmb-bhkw/blob/master/main.js#L127 would pot. set a state without the object being initialized at least once?, right?

Thank you for checking and adjusting the comments

[Apollon77]

PS: regarding the main vs master ... https://github.com/ioBroker/ioBroker.repositories/pull/1779/files#diff-1384831e45cb5b5fc71b065c642c35169d0ee196221e07b4b81075a0d3b0aed8R1627 here you have it mixed up

[Garfonso]

@satchafunkilus
Hi,

some things I noticed in addition to @Apollon77:

• the icon and readme paths in your io-package.json are wrong and point to main-branch. Adjust them to master branch (might be connected to your PR here, see earlier comment).
• Your README is nice, but completely in German. Please add at least a short description in English (even if the adapter should only be of interest in Germany, people all over the world still need to decide if they need this adapter or not).

[satchafunkilus]

@Apollon77 @Garfonso thanks for your remarks, I have incorporated all your remarks and suggestions.

Regarding the schedule randomization: It appears to me that the approach suggested only works for randomizing an hourly poll of the data. As I need to poll the data every 5-15min, I added a random delay between 0-59 seconds, so not all polls are executed at the same time. I would assume that, taking into account the small number of devices of this type out there, this should be enough to prevent any major spikes on the servers.

[Apollon77]

Also a schedule like 1,6,11,16,... * * ... is a valid one ;-) but ok. I think it is still better then nothing, so ok for me ;-). Will check in depth later

[Apollon77]

Hi, you need to add one thi g if you now use such a delay option: You need to add a flag to your unload handler to remember that unload was called while delay was running ... and if then you need to stop your logic after the delay ... or you need to add a way that allows to cancel the delay in case of an unload.

500ms after your unload callback the database is gone and running your logic will give issues.

And please remove the german version from readme because readme get automatically translated and this will not work. You can add locatlized reme pages in (docs/de and docs/en) like https://github.com/iobroker-community-adapters/ioBroker.pvforecast/tree/main/docs and add it to io-package https://github.com/iobroker-community-adapters/ioBroker.pvforecast/blob/main/io-package.json#L119-L122

After this we should be ready

[Apollon77]

@satchafunkilus DO you need help for anything?

[satchafunkilus]

@satchafunkilus DO you need help for anything?

Thanks, I'm currently on a longer vacation without my PC, so unfortunately I can't work on the code at the moment. I tried implementing the thing you mentioned wiith regards to the flag for the delay before I left, but I wasn't really able to get it working in the short time. If you have any example for such a flag, that would help a lot. Thanks.

[Apollon77]

Sure, just set a variable :-)) e.g. https://github.com/Apollon77/ioBroker.meross/blob/master/main.js#L169

and then check in the other places "if (stopped) return" to simply stop logic :-)

[Apollon77]

@satchafunkilus Any update?

[satchafunkilus]

@satchafunkilus Any update?

I have added the check as you mentioned - however, I'm not 100% sure it's correct this way. Could you have a quick look?

Thanks

[Apollon77]

@satchafunkilus near to correct :-) When stpped is set true now thenstop prpcess is already in progress. replace https://github.com/satchafunkilus/ioBroker.rmb-bhkw/blob/master/main.js#L54 by just a "return;". SP just make sure to not continue ...

And BTW: maybe fill in better tags? https://github.com/satchafunkilus/ioBroker.rmb-bhkw/blob/master/io-package.json#L76

Last but not least: Please also check the comments of the automatic checker above

[Apollon77]

@satchafunkilus Anything we can support?

[Apollon77]

@satchafunkilus

[satchafunkilus]

@Apollon77 I have finally found some time to make the changes requested above. I have tested it, and it seems to be working fine. For some reason, the automated checks on github are now failing, though – the error message is:

pm ERR! Cannot read property '@iobroker/adapter-core' of undefined

Any clue what I can do here?

[Apollon77]

Ideally delete node-modules and also the package-lock.json and then execute "npm install". How is it then?

[satchafunkilus]

That I already tried yesterday, unfortunately it didn't help. But I realized that the node-modules folder is not uploaded to github anyway, so that cannot really be the issue.

[Apollon77]

Did you really also deleted the package-lock.json???? Because this one IS uploaded

[satchafunkilus]

Yes, both

[mcm1957]

https://t.me/ioBroker_development/26673 could be related to this.

Question:
Which npm version do you have installed?
Where exactly did you get the npm error? During tests?

[satchafunkilus]

Yes, that sounds like the same issue. I get it during the github actions tests after uploading a new version. https://github.com/satchafunkilus/ioBroker.rmb-bhkw/actions/runs/4213085290

npm version is 9.5.0

[xXBJXx]

Please remove the tests for Node 12, which are no longer supported as of ioBroker/Testing version 4.0.0.
Your version in package.json is 4.1.0

[xXBJXx]

@satchafunkilus
I made you a PR where I updated some files and took out the test for node 12 / 14.
Check if everything still works for you.

[satchafunkilus]

@satchafunkilus
I made you a PR where I updated some files and took out the test for node 12 / 14.
Check if everything still works for you.

Awesome, thanks. Much appreciated. Tests seem to have run through now without any issues.

[satchafunkilus]

RE-CHECK!

[GermanBluefox]

Automated adapter checker

ioBroker.rmb-bhkw

👍 No errors found

• 👀 [W400] Cannot find "rmb-bhkw" in latest repository

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

[satchafunkilus]

Any to-do left here from my end?

[Apollon77]

I will re-review soon

[mcm1957]

This PR MUST NOT try to update package-lock.json.
A PR requesting addition to latest must update exactly sources-dist.json only.

Please remove package-lock.jsosn from the PR or close this PR and create a new one. Feel free to use iobroker.dev support to create the PR with one or two clicks. When creating a ne PR, please reference this PR in it.

[satchafunkilus]

Please remove package-lock.jsosn from the PR or close this PR and create a new one. Feel free to use iobroker.dev support to create the PR with one or two clicks. When creating a ne PR, please reference this PR in it.

I have created a new PR #2298