Skip to content

Add wiim to latest #4683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add wiim to latest #4683

wants to merge 1 commit into from

Conversation

KaiIOB
Copy link

@KaiIOB KaiIOB commented Apr 2, 2025

Please add my adapter ioBroker.wiim to latest.

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

@github-actions github-actions bot added the auto-checked This PR was automatically checked for obvious criterias label Apr 2, 2025
Copy link

github-actions bot commented Apr 2, 2025

Automated adapter checker

ioBroker.wiim

Downloads Number of Installations (latest) - Test and Release
NPM

👍 No errors found

  • 👀 [S062] Consider using "@alcalzone/release-script".
  • 👀 [W401] Cannot find "wiim" in latest repository

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

@mcm1957 mcm1957 added new at LATEST REVIEW pending (by mcm1957) obj-json missing The reuqsted dump obj objects in json format is missing labels Apr 2, 2025
Copy link

github-actions bot commented Apr 2, 2025

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:

Important:

To verify the object structure of this adapter during REVIEW please export the object structure of a working installation and attach the file to this PR. You find a guide how to export the object struture here: https://github.com/ioBroker/ioBroker.repochecker/blob/master/OBJECTDUMP.md

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
Copy link
Collaborator

mcm1957 commented Apr 2, 2025

reminder 9.4.2025

@ioBroker ioBroker deleted a comment from github-actions bot Apr 3, 2025
@github-actions github-actions bot added the *📬 a new comment has been added label Apr 3, 2025
@ioBroker ioBroker deleted a comment from github-actions bot Apr 3, 2025
@mcm1957 mcm1957 removed the *📬 a new comment has been added label Apr 4, 2025
@KaiIOB
Copy link
Author

KaiIOB commented Apr 6, 2025

Here comes the object structure:

wiim.0.json

@github-actions github-actions bot added the *📬 a new comment has been added label Apr 6, 2025
@mcm1957 mcm1957 removed *📬 a new comment has been added obj-json missing The reuqsted dump obj objects in json format is missing labels Apr 6, 2025
@mcm1957
Copy link
Collaborator

mcm1957 commented Apr 18, 2025

@KaiIOB

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 changelog

    Please rmove '<' from changelog headers. It sould read '###' to indicate H3 headers.

  • 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, '_');
    }
    

    You retriev the servername (first level object) from the bonjour esponce. This might conatin invalid characters and hence must be filtered. At least FORBIDDEN_CHARS maust be removed. Spaces should be removed in additon to. Optimal solution is to allow only characters [0-9A-za-z_-]. Dots must be handled seperatly as they are used as seperator between levels.

    Sepcials chars can cause problesm with several VIS implementations (i.e. space is know to break VISes).

  • Please minimize non-debug logging

    Please minimize logging to such info that a normal user will use. Feel free to log as much information as useful at DEBUG level. And please remove ornaments ect (like '******* xxx*****') and reduce to pure information. See https://github.com/KaiIOB/ioBroker.wiim/blob/eaaf2c23cd8017e9e45a4ff89495ac3bdc3337c1/wiim.js#L35

  • do not log errors as info

    Errors shoudl be logged as error or wanring (depending ob effect on opertion. If an error is normal and does not effect normal use, log it as debug. see ie. https://github.com/KaiIOB/ioBroker.wiim/blob/eaaf2c23cd8017e9e45a4ff89495ac3bdc3337c1/wiim.js#L86

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

  • all objects must be created

    You do not cretate intermediate objects. i.e. 'wintergarten' does not have a type assigned. Object hiorarchie must be
    'device' -> 'channel' -> 'state'. Device and / or channel might be omitted. chennels might have subchannels if required. device is NOT allowed below channel. States must not have any children. In addition 'folders' are allowed at every level.

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

    • role indoicator requires a state of type boolean
  • check r/w of states

    Are you sure that tracklength is a writeable state?

  • state subcriptions

    • you subscribe to a bigger number of states. This might lead to some noticeable filtering effort at js-controller. I would suggest to subscribe to all (own) states (this.subscribeStates('*')) and filter locally. This will move filtering load to your adapter and at least at system using more than one cpu core distribute the load better.
    • what du you expect by setting the option val:true, ack: true at subscribeStates?
  • remove unused admin files

    You adapter uses jsonConfig.json. So words.js seems to be outdated. I did not see any configuration for custom attributes or admin tab. So custom_m.html and tab_m.html seem to be obsolete too.

    wim.PNG is (most likely) not used t0o ans should be removed.

  • remove unused config paramaters from io-package.json

    IP_Address and Request-type are not used at configuration. So they should be removed from io-package.json. See https://github.com/KaiIOB/ioBroker.wiim/blob/eaaf2c23cd8017e9e45a4ff89495ac3bdc3337c1/io-package.json#L87

  • remove unused dependencies

    tcp-ping and xml2js are listed at dependencies but there is no require of them insoide code. So this dependencies seem to be obsolete. Please remove.

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

  • use setTimeout or delay instead of one-time intervals

    During startup you use ruther complicated setInterval solutions which seem to do only one thing - delay operation. Please eiterh use setTimout or (more likely to do the jiob there) use this.delay so delay operation as desired. In addtio it's at least unusal to loop until some state is reached (i.e. bonjourFinished). You can simply call the next routine at the point whre you set bonjourFinished - either directly or by using a setTimout.

  • general remark

    Some remarks:

    • The adapter seems to scan for devices only during startup. Whats the behavior when some device gets switched off while the adapter is running? The adpater should NOT liog errors over an over while the device is sitched off.

    • How does the adapter recognize a devide sitched off during adapter startup? The adapetr should - in my oppinion - scan from tie to time to detect new devices. It's not hande for users if the must restart the adapter whenever a new device is switched on. Consider running bounjour descovery at background .

Thanks for reading and evaluating this suggestions.
McM1957

** Please attach a new object dump whith corrected object creation and roles **

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 2.5.2025

@mcm1957 mcm1957 added must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review and removed REVIEW pending (by mcm1957) labels Apr 18, 2025
@mcm1957
Copy link
Collaborator

mcm1957 commented May 1, 2025

@KaiIOB
Did you have time to review the issues and fix them?

reminder 8.5.2025

@github-actions github-actions bot added 8.5.2025 and removed 2.5.2025 labels May 1, 2025
@mcm1957
Copy link
Collaborator

mcm1957 commented May 16, 2025

@KaiIOB
Did you have time to review the issues and fix them?

I do not have time to review before 26.5.2025 but please drop a note until then stating if an within what timeframe you plan to process the issues.

reminder 26.5.2025

@KaiIOB
Copy link
Author

KaiIOB commented May 16, 2025 via email

@github-actions github-actions bot added the *📬 a new comment has been added label May 16, 2025
@mcm1957 mcm1957 added the ON HOLD PR is set ON HOLD due to pending question or major issues. label May 25, 2025
@mcm1957 mcm1957 removed the *📬 a new comment has been added label Jun 4, 2025
@mcm1957 mcm1957 removed the 26.5.2025 label Jul 16, 2025
@github-actions github-actions bot added *📬 a new comment has been added 26.5.2025 labels Jul 16, 2025
@mcm1957 mcm1957 removed the *📬 a new comment has been added label Jul 16, 2025
@mcm1957
Copy link
Collaborator

mcm1957 commented Jul 16, 2025

reminder none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-checked This PR was automatically checked for obvious criterias must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review new at LATEST ON HOLD PR is set ON HOLD due to pending question or major issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants