Add cloudless-homeconnect to latest

[mcm1957]

@eifel-tech

Replacement for #4213

[mcm1957]

reminder 2.11.2024

[simatec]

In the jsonConfig I miss some settings for the different screen resolutions.
Here is an example of what it should look like.

"xs": 12,
"sm": 12,
"md": 6,
"lg": 4,
"xl": 4

[eifel-tech]

@simatec Added the missed screen resolutions and described it at Work in progress section in readme file. This is my first adapter so I wonder if that is enough or do you need a new release now to recheck my code?

[mcm1957]

Thanks for fast response.

Please stand by. @simatec did a dedicated check in respect to responsive UI. I will do a check of code and general behavior as soon as I find some time for it. Maybe some more remarks will occure.

You may create a new release whenever you want, this will not block any checks. But ist not mandatory at this time.

[mcm1957]

Sorry, but due to travel / absence I will nozt able to do the review before ioBroker Meeting Solingen next weekend.

[mcm1957]

@eifel-tech

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

And sorry for the long response time.

I would like to give some feedback based on my personal oppinion. @Apollon77 might have additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him if you cannot follow my suggestions or want to discuss some special aspects.

• Readme.md - Disclaimer

  The disclaimer at README:md seems to be left from template text. If you do not feel a need to have it, consider removing it from README.md.

• Readme.md must be written in pure english

  As ioBroker is an international software supporting several languages the main Readme.md file must be written in english. Its ok to add additional Readme-.mds of course.

  Please remove the german version / all non english versions from Readme.md because Readme.md gets automatically translated and this will not work with multiple languages within Readme.md. You can add locatized Readme 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 (see https://github.com/iobroker-community-adapters/ioBroker.pvforecast/blob/main/io-package.json#L119-L122)

• sensitive information, especially passwords should be stored encrypted

  You configuration seems to contain passwords (or other sensitive information) which is not stored encrypted. As long as those information is not stored inside a table you can encrypt and secure this infomation simply by adding the following configuration to io-package.json. The iob runtime will encrypt / decrypt the data as required; no addition programming effort is required. Note that users will need to reenter this info one time after adding encryption, so drop a note at release notes.

  "encryptedNative": [
    "password",
    "data2"
  ],
  "protectedNative": [
    "password",
    "data2"
  ],
  

• directory 'js'

  Although its completly Ok to name directories as you like, I want to note, that all utility modules typically are placed into 'lib' directory. So I would suggest to move your files from 'js' to lib. This would make it mor consistent with typical adapter structure.

• name attributes must be english or multilingual

  "name" attributes for objects (states) must contain an english text (string) or an i18n multlingual object with all supported languages.

• invalid characters must be filtered from object ids

  Some characters are not allowed to be used as part of an object id. If an object id is not hardcoded or even depending on user input you should ensure that such an object id does not contain an invalid character. Iobroker provides the constant adapter.FORBIDDEN_CHARS, i.e. by using the following snippet:

function name2id(pName) {
    return (pName || '').replace(adapter.FORBIDDEN_CHARS, '_');
}

As far as I read the code the configuration is retrieved from the device and the ids for all states is created by this information. The adapter msut ensiure, that no invalid characters are passed in this way. So filter all invalid characters when creating object ids.

• objects of type state must not have children

  It look like you create some object of type state "dev_uid here:

  https://github.com/eifel-tech/ioBroker.cloudless-homeconnect/blob/d3342dfab5457573afcf277e8d0844bcc8309b02/main.js#L308

  And here you create an object of type state with id "dev_uid.optioname"

  https://github.com/eifel-tech/ioBroker.cloudless-homeconnect/blob/d3342dfab5457573afcf277e8d0844bcc8309b02/main.js#L289

This construct seems to create states as children of other states. This is not allowed. The parent of an state must be of type device, channel or folder. Objects of type state must not have children.

• log messages should / must be in english

  As ioBroker is an international software, please use only english wording for all logging messages - unless you translate logging depending on system language. Logging using the system langugae is ok of course - buts its your decision if you want toe invest the effort. German only logs are not desired. Thios refers to all text defined by you - error messages reurned from aservice (external text) can be kepogt as is of course.

• onStateChange handler ignores ack flag

  The adapter must honor the ack flag when processing onStateChange events. Whenever the onStateChange handler is called for an own state (a state owned by the processing instance of the adapter) it must ignore this event if the ack flags is set. Only stateChanges with ack==false are allowed to be processed.

  When processing foreign states which are output of another adapter processing must only occure if the ack flag is true.

• 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.

  Please avoid using general roles (state, value) whnever a dedicated role exists.

• Add timeout to axios calls

  All axios calls should have a timeout set. The default value for axios timeout is 0 which results in no timeout. So an unresponsive remote system could lead to an hanging adapter. Either set an appropiate timeout per axios call or set a default timeout globally for the axios instance.

• all timers / intervals must be canceld at onUnload handler

  During onUnload processing all timers and intervalls must be cancelled. a ckearIntervall for refreshInterval seems to be missing.

• check and adapt testing

  Please use standard iobroker test-and-release.yml workflow like
  https://github.com/ioBroker/ioBroker.example/blob/master/JavaScript/.github/workflows/test-and-release.yml

• linter setup

  Please use standard iobroker linter setup. This is located at @iobroker/eslint-config. See migration guide at https://github.com/ioBroker/ioBroker.eslint-config/blob/main/MIGRATION.md

Thanks for reading and evaluating this suggestions.
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!

reminder 10.12.2024

[eifel-tech]

@mcm1957
Thank you for your time to reviewing the code of my adapter.
I have implemented most of your comments but have a few questions now:

• objects of type state must not have children: I checked my implmentation but could not find, where the structure was not being followed. All state DPs will create under channel or device, also the DPs you linked to. This is allowed isn't it?
• check and adapt testing: I use the test things of the original yml (see here) and have only changed the deployment to npm on my own. I also don't need to create a new github version of your yml, I want to do the version management on my own. I think that is not wrong? That couldn't be a blocking note for latest! :-O
• linter setup: I tried to migrate but the configured settings are not my favorite and I was not able to reconfigure your defaults so that my IDE reacts like I want to. It costs a lot of time for a thing that only helps the look and feel of the IDE is the same also. So honestly I don't want to use your setup. Is this a blocking note for merging into latest?

Apart from that, a review can now be carried out again.

[github-actions]

Automated adapter checker

ioBroker.cloudless-homeconnect

-

👍 No errors found

• 👀 [W401] Cannot find "cloudless-homeconnect" in latest repository

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

[mcm1957]

** RE-REVIEW Workksheet only - please ignroe for now**

• Readme.md - Disclaimer

  The disclaimer at README:md seems to be left from template text. If you do not feel a need to have it, consider removing it from README.md.

• Readme.md must be written in pure english

  As ioBroker is an international software supporting several languages the main Readme.md file must be written in english. Its ok to add additional Readme-.mds of course.

  Please remove the german version / all non english versions from Readme.md because Readme.md gets automatically translated and this will not work with multiple languages within Readme.md. You can add locatized Readme 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 (see https://github.com/iobroker-community-adapters/ioBroker.pvforecast/blob/main/io-package.json#L119-L122)

• sensitive information, especially passwords should be stored encrypted

  You configuration seems to contain passwords (or other sensitive information) which is not stored encrypted. As long as those information is not stored inside a table you can encrypt and secure this infomation simply by adding the following configuration to io-package.json. The iob runtime will encrypt / decrypt the data as required; no addition programming effort is required. Note that users will need to reenter this info one time after adding encryption, so drop a note at release notes.

  "encryptedNative": [
    "password",
    "data2"
  ],
  "protectedNative": [
    "password",
    "data2"
  ],
  

• directory 'js'

  Although its completly Ok to name directories as you like, I want to note, that all utility modules typically are placed into 'lib' directory. So I would suggest to move your files from 'js' to lib. This would make it mor consistent with typical adapter structure.

• name attributes must be english or multilingual

  "name" attributes for objects (states) must contain an english text (string) or an i18n multlingual object with all supported languages.

• invalid characters must be filtered from object ids

  Some characters are not allowed to be used as part of an object id. If an object id is not hardcoded or even depending on user input you should ensure that such an object id does not contain an invalid character. Iobroker provides the constant adapter.FORBIDDEN_CHARS, i.e. by using the following snippet:

function name2id(pName) {
    return (pName || '').replace(adapter.FORBIDDEN_CHARS, '_');
}

As far as I read the code the configuration is retrieved from the device and the ids for all states is created by this information. The adapter msut ensiure, that no invalid characters are passed in this way. So filter all invalid characters when creating object ids.

• objects of type state must not have children

  It look like you create some object of type state "dev_uid here:

  https://github.com/eifel-tech/ioBroker.cloudless-homeconnect/blob/d3342dfab5457573afcf277e8d0844bcc8309b02/main.js#L308

  And here you create an object of type state with id "dev_uid.optioname"

  https://github.com/eifel-tech/ioBroker.cloudless-homeconnect/blob/d3342dfab5457573afcf277e8d0844bcc8309b02/main.js#L289

This construct seems to create states as children of other states. This is not allowed. The parent of an state must be of type device, channel or folder. Objects of type state must not have children.

• log messages should / must be in english

  As ioBroker is an international software, please use only english wording for all logging messages - unless you translate logging depending on system language. Logging using the system langugae is ok of course - buts its your decision if you want toe invest the effort. German only logs are not desired. Thios refers to all text defined by you - error messages reurned from aservice (external text) can be kepogt as is of course.

• onStateChange handler ignores ack flag

  The adapter must honor the ack flag when processing onStateChange events. Whenever the onStateChange handler is called for an own state (a state owned by the processing instance of the adapter) it must ignore this event if the ack flags is set. Only stateChanges with ack==false are allowed to be processed.

  When processing foreign states which are output of another adapter processing must only occure if the ack flag is true.

• 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.

  Please avoid using general roles (state, value) whnever a dedicated role exists.

• Add timeout to axios calls

  All axios calls should have a timeout set. The default value for axios timeout is 0 which results in no timeout. So an unresponsive remote system could lead to an hanging adapter. Either set an appropiate timeout per axios call or set a default timeout globally for the axios instance.

• all timers / intervals must be canceld at onUnload handler

  During onUnload processing all timers and intervalls must be cancelled. a ckearIntervall for refreshInterval seems to be missing.

• check and adapt testing

  Please use standard iobroker test-and-release.yml workflow like
  https://github.com/ioBroker/ioBroker.example/blob/master/JavaScript/.github/workflows/test-and-release.yml

• linter setup

--- feedback to be answered ---

• objects of type state must not have children: I checked my implmentation but could not find, where the structure was not being followed. All state DPs will create under channel or device, also the DPs you linked to. This is allowed isn't it?

• check and adapt testing: I use the test things of the original yml (see here) and have only changed the deployment to npm on my own. I also don't need to create a new github version of your yml, I want to do the version management on my own. I think that is not wrong? That couldn't be a blocking note for latest! :-O

• linter setup: I tried to migrate but the configured settings are not my favorite and I was not able to reconfigure your defaults so that my IDE reacts like I want to. It costs a lot of time for a thing that only helps the look and feel of the IDE is the same also. So honestly I don't want to use your setup. Is this a blocking note for merging into latest?

[mcm1957]

@eifel-tech
Thanks for processing the issues. There's only one issue which need further checks. See below:

ad lint:0
Its ok to use own linter setup. Using recommended eslint-config is nozt mandatory. Eventually you will recieve some recommandations from adapter checker in future but those will not be mandatory / level error too.

ad testing:
Creating github releases is a good practise but not required.
As long as standard iobroker tests are executed ( ioBroker/testing-action-check@v1, ioBroker/testing-action-adapter@v1) its ok. Additional test are welcome of course.
So testing is OK

ad object Tree:
Please post a view at the object tree (fully expanded) created by the running adapter (i.e. as a screen shot or as a json export) so that I can check if the state / child relations are ok. Thanks.

reminder 29.12.2024

[eifel-tech]

@mcm1957
Here you can check the object tree of the adapter.
I hope it's all ok now?

[mcm1957]

lgtm

[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, it' OK to continue using this topic too.

If you do not have write access to TESTER section, please drop a PN / Chat Note to HOMORAN at forum.