Add uvr16xx-blnet to latest

weberk commented 2 weeks ago

Please add my adapter ioBroker.uvr16xx-blnet to latest.

This pull request was created by https://www.iobroker.dev c0726ff.

see https://github.com/ioBroker/AdapterRequests/issues/750

github-actions[bot] commented 2 weeks ago

ioBroker repository information about New at LATEST tagging

Thanks for spending your time and providing a new adapter for ioBroker.

Your adapter will get a manual review as soon as possible. Please stand by - this might last one or two weeks. Feel free to continue your work and create new releases. You do NOT need to close or update this PR in case of new releases.

In the meantime please check any feedback issues logged by automatic adapter checker and try to fix them. And please check the following information if not yet done:

You will find the results of the review and eventually issues / suggestions as a comment to this PR. So please keep this PR watched.

If you have any urgent questions feel free to ask. mcm1957

@simatec Please take a look in respect to responsive design. Thanks

mcm1957 commented 2 weeks ago

reminder 24.11.2024

simatec commented 1 week ago

jsonConfig looks good jsonCustom.json5 is not in use and should be deleted

Anlop commented 1 week ago

Habe den Adapter getestet, ich bekomme unter "Objekte" im iobroker die Werte der UVR 1611 gelesen.

weberk commented 1 week ago

If I just delete the file jsonCustom.json5 but Adapter Checker complains about: [E512] "admin/jsonCustom.json5" not found, but custom support is declared

Then I tried to set io-package.json> "supportCustoms": false,

but still https://www.iobroker.dev/adapter-check is complaining about 'custom support declared'.

Question: How to I switch custom support off? Answer: after pushing the changes to git the issue was gone.

Now the adapter does not longer use jsonCustom.json5

mcm1957 commented 1 week ago

Webbased checker is outdated. Please use commandline version or take a look at https://github.com/ioBroker/ioBroker.repositories/pull/4298#issuecomment-2497315538
https://github.com/weberk/ioBroker.uvr16xx-blnet/issues/6

see https://github.com/weberk/ioBroker.uvr16xx-blnet/issues/6

mcm1957 commented 6 days ago

@weberk

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

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 is incomplete

The README:md contains mainly to template used to create a new adapter. Please remove all developer information, those info is not of interest for users.

Please add a short description about the functionality of the adpter, add links the the manufacturer site (if applicable) and add a description of the configuration of the adapter. If you provide an external documentation - which should be located at doc/xx directory - please add links to at least the english documentation.
[ ] unusual file [.!775!.DS_Store]

The file .!755!.DS_Store looks a little bit unusual. Please check / remove or comment.
[ ] access node modules using node: syntax

You shoudl specify 'const net = require("node:net");' to clearly reuire the node module net and to avoid to fetch some module named 'net' from npmjs or a local installation.
[ ] duplicated if

Please check this if construct and the following if statement(s) https://github.com/weberk/ioBroker.uvr16xx-blnet/blob/9573f86827c17b8ccba0ee73cdddc35ad4c2413c/main.js#L81
[ ] invalid characters should 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, '_');
}
```
Looks like you are creating staeId based on information returnd frome xterna system. Please check this information does not contain invalid characters.
[ ] unused onStateChange handler

You configred an onStateChange handler but it looks like this handler does not do any important tasks. Please remove the handler if the adapter does not react on state changes.
[ ] creation of device/channel/folder objects missing

You create states named 'output.xxx'. But I did not see a where you create the object named 'output'. You must create every level of the object tree eiuther programmatically using setObjectNOtExists or extendObject or by adding the object to io-package.json native area (like info and info.connection).
[ ] create objects only once

Do not call setObjectNotExists whenever a state value is written. This causes uneccessary load to the system. States must be created once only. Call setObjectNotExists only once after start of the adapter and remember that the state has been created i.e. by storing the info at an object. Its ok to do this initialization at every adapter start.
[ ] 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) whenever a dedicated role exists.

As an example (but this is not a complete list) states with role 'indictaor' must be of type boolean.
[ ] 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 on load. Additional checks on thomse 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.
[ ] check and limit configurable timeouts / intervals

Node setTimeout/setInterval routines have a maximum allowed value of 2,147,483,647 ms. Using delays larger than 2,147,483,647 ms (about 24.8 days) result in the timeout being executed immediately. So all (user configurable) values passed to setTimeout / setInterval should be checked in code and limited. Checking/limiting in code is required as config data could be changed directly or by some other adapter too, so limiting at ui level might not be sufficient.
[ ] evaluate to use this.setTimeout instead of this.setInterval

When using setInterval you must ensure, that any cycle is completed before the next cycle is started. This can become complicated if the external device does not respond within a specified time. If multiple processing cycles are started in paralle this could sum up and consume ore an more resources finally crashing the complet ioBroker instance. So in most cases it would be better to start a new cycle at the end of the processing loop using this.setTimeout instead of using this.setInterval.
[ ] admin ui inconsistency

Please specify if you are using jsonConfig.json admin UI or want to use a react based admin UI.

You specify common.adminUI.config : json at io-package.json. And you provide a jsonConfig.json file. So it looks very like you want to use jsonConfig to provide admin ui. Your jsonConfig.json does not contain any custom componen reference. So I would assume, that you do NOT want to use react.

So at keast the follwing steps shuld be done:
- admin/src, admin/build, admin/.eslintrc.json seem to be obsolete and should be removed
- @iobroker/adapter-react and other react devDependencies should be removed
- build commands do not make any sense and should be removed from package.json (compare with plain js adapter from examples https://github.com/ioBroker/ioBroker.example/tree/master/JavaScript)
- disable build at workflow test-and-release.yml
- adapt config of releasescript (remove build step)
Not sure if this includes every step. You should not have selected REACT and CUSTOM when creating teh adapter unless you want to programm with react ( see .create-adapter.json : "adminUi": "react", "adminFeatures": ["custom"],)
[ ] 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 11.12.2024

github-actions[bot] commented 4 days ago

Automated adapter checker

ioBroker.uvr16xx-blnet

Number of Installations (latest) -

[ ] :heavy_exclamation_mark: [E504] setInterval found in "main.js", but no clearInterval detected
[ ] :eyes: [W401] Cannot find "uvr16xx-blnet" in latest repository
[ ] :eyes: [W505] setTimeout found in "main.js", but no clearTimeout detected

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

weberk commented 4 days ago

@mcm1957 , @Apollon77

Thinking on renaming the adapter from "uvr16xx-blnet" to "uvrxx-blnet". What implications whould this have on npm, the iobroker repo, ...? Would allow to support more climate controls in the future than devices starting with UVR16.. Please advise.

Ready to comment and asking for a recheck.

[x] Readme.md is incomplete Remove all developer information. Added a short description about the functionality of the adpter, added links to the manufacturer site and added a description of the configuration of the adapter. Link to an external documentation - additionally is located at doc/xx directory.
[x] unusual file [.!775!.DS_Store] The file .!755!.DS_Store was delete and added to .gitignore.
[x] access node modules using node: syntax added 'const net = require("node:net");' to avoid to fetch some module named 'net' from npmjs or a local installation.
[x] duplicated if removed
[x] invalid characters should be filtered from object ids Used name2id(pName) for all node names.
[x] unused onStateChange handler Removed the handler since the adapter does not react on state changes.
[x] creation of device/channel/folder objects missing Every level of the object tree is now created programmatically using setObjectNOtExists.
[x] create objects only once setObjectNotExists is now only called on adapter instance start.
[x] reevaluate state roles Roles and types of the nodes have new best matching values.
[x] consider using adapter.setTimeout / this.setTimeout instead of (standard) setTimeout Used the adapter package provides wrapper routines for native setTimeout, setInterval. Suppose I do not have to care about clearTimeout and clearInterval myself, because AdapterClass is caring.
[x] check and limit configurable timeouts / intervals Additional checks on polling interval limits was added in config and programatically.
[x] evaluate to use this.setTimeout instead of this.setInterval Removed usage of setInterval. Starting a new cycle at the end of the processing loop using this.setTimeout instead of using this.setInterval.
[x] admin ui inconsistency Removed all react stuff in filesystem, build, script, dependencies and lint.
[x] linter setup Already using standard iobroker linter setup which was setup by the adapter generator. Migration avoided.

mcm1957 commented 4 days ago

As this adapter is not yet listed at any repository feel free to rename it. The reason for a rename seems very valid.

Of course rename must be done at all places (Repository, entries within io-package.json, package.json ... I do not know whether npm allow a rename - please check npm docu

Maybe it would be easier to create a new adapter with the new name and copy main.js (and otehr files you have modified / created to the new aadpert. If finished you can set the old repository to archived and npm to deprecatd (or unpublish the package).

weberk commented 3 days ago

Managed to rename it in place. Needed to publish it as a new package to NPM Will also issue a new PR to add it to this repository. Have to wait on bluefox accespting the invite. New location is ioBroker.ta-blnet This PR can be closed.

ioBroker / ioBroker.repositories