Add refoss to Latest

[chuanlingZhao]

Add a new adapter to the latest repository

[mcm1957]

reminder 19.12.2024

[github-actions]

Automated adapter checker

ioBroker.refoss

-

👍 No errors found

• 👀 [W401] Cannot find "refoss" in latest repository

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

[chuanlingZhao]

Follow-up on Code Review Progress

Dear ioBroker Team,

I hope this message finds you well. I am writing to follow up on the code submission from our software team for review. Could you kindly provide an update on the status of the review process and let us know if any further information or actions are required from our side?

We appreciate your time and effort in reviewing the code, and we look forward to your feedback.

Thank you for your assistance!

[mcm1957]

Sorry for the delay, but I did not have time to do a review until now.

[chuanlingZhao]

It's all right. I completely understand that you've been really swamped. Thank you so much for making the time to do it now. If it's convenient for you, could you give me an estimate of how long it will take to finalize the review?

[mcm1957]

@chuanlingZhao

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.

• package.json contains contributor bluefox

  Did bluefox really contribute to this adapter? If not, please remove.

• why do you create a user folder 'scripts'?

  see https://github.com/Refoss/ioBroker.refoss/blob/007b1b3fb82893172478156ebf777b687c657f4e/main.js#L35
  Please indicate where you are using this storage or remove creation.

• Why do you subscribe to file changes?

  see https://github.com/Refoss/ioBroker.refoss/blob/007b1b3fb82893172478156ebf777b687c657f4e/main.js#L36
  I did not see any onFileChange handler. So this subscription does not seem to fulfill any purpose. Please remove or indicate its usage.

• Why do you subscribe to states?

  https://github.com/Refoss/ioBroker.refoss/blob/007b1b3fb82893172478156ebf777b687c657f4e/main.js#L36
  As there is not usefule onStateChange code doing any useful porpose, please remove subscription or explain the reason for it.

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

• some labels seem to have no translation into all languages

  It seems that some labels are not translated into all supported languages, i.e. ...
  Please consider adding missing translation into i18n/xx/transltions.js. Adding to english translatioons.json (i18n/en/translations.json) should be enough as running npm run translate will update otehr languages.

• Erors should be logged

  Please evaluate whether ist really agood idea to ide errors from users, i.e. here: https://github.com/Refoss/ioBroker.refoss/blob/007b1b3fb82893172478156ebf777b687c657f4e/lib/protocol/udp4.js#L53. User will not run with debug log enabled, so they wiull not get informed if an error occures but wonder why the adapter is non functional.

• binding to fixed ports

  Is there any reason you bind the udp server to a fixed port? See https://github.com/Refoss/ioBroker.refoss/blob/007b1b3fb82893172478156ebf777b687c657f4e/lib/protocol/udp4.js#L74. This port could be occupied by another application already.

• adminUI - parameter port

  Please add some helpText or tooltip describing the usage of paramater 'port' to jsonConfig.json.
  Where is this port used in code?

• object ids

  Object ids must only contain a limited number of characters. It looks like the states created by the adapter are defined within refossem06 (and the assizioated base file. If I read the code correct, the object key is used as stateId. This is not correct as stateId must not contain i.e a dot or a space. Spaces should be avoided as other parts of ioBroker, especial VISes cannot handle them correctly. Best praxis is to mit to [A-Za-z0-9-_] Dots have a special purpose as seperators within hirarchie. Here you must ensure that all intermediate objects are created too. I did not see a place where you create the objects A1, ... (most likely of type channel or folder)

  If objectIds are based on external sources (i.e. sent by an device) the received data must be filtered againts the constant FORBIDDEN_CHARS - see nect point

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

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

  At least the role value.pf does not exist. In this case 'value' would be best matching

• Please attach object dump

  To verify the Object structure of this adapter please export the sobject structure of a working instllation and attch athe file. You find a guide how to export the object struture here: https://github.com/ioBroker/ioBroker.repochecker/blob/master/OBJECTDUMP.md

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

• check and adapt testing

  Standard iobroker testing is required but seems to be missing. Please add and setup ./github/workflows/test-and-release.yml

  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

  If you need a PR for this, please let me know.

• adapt testing to supported node releases

  Tests for node 18 are mandatory unless engines at package.io reuires node 20.
  Tests for node 20 are mandatory as this is the recommended node version.
  Tests for node 22 are mandatory unless you already know incompatibilities which cannot be fixed immidiatly. In this case please create a issue stating the problem and fix as soon as possible.

  So the recommended testmatrix is [18.x, 20.x, 22.x] depending on engines requirments setting at package.json

  Tests must be performed at all supported platforms (linux, windows, mac) unless special hardware or software restrictions prohibit this.

• linter setup

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

• github dependabot support seems to be missing

  It looks like you did not yet setup dependabot support; at least .github/dependaboit.yml is missing.

  Please consider setting up dependabot support (and to ease life the workflow auto-merge.yml). Dependabot will ensure that your adapter uses the most recent versions of external packages and avoids problem due to outdted base libraries. Dependabit provides PRs at you repository to update the dependencies. Depending on you setting, especially on auto-merge.yml configuration no or only minor revisions will be outmatically merged into you main branch. All major revisions msiut (ans should) be reviewed by you.

Some more general remarks for discussion

• usage of @apollon/iobroker-tools

  I'm not sure wether using this pacakge is a good solution. I'm not aware that it is designed as a general tools package and it looks like an overhead at this time. The package is based on callbacks. You do not use the callbacks - so it not clear withou further investigation if timing is guaranteed. Normally teh up to date functions provided by js-controller / adapter-core which use primises and are ready to be used with async await environment might be better choice. I'll aks @Apollon77 for feedback to this question. He might answer directly too.

• I'm not quite sure how you detect new devices. I assume that this is based on data received via udp. But you also sent some udp packet when starting the adapter. But this is only performed twice after adapterstart. How do you detect devices not active during adapter start?

• You call onlineSatusCheck every 15s. There you quere all devices known by the adapter Instance using getAllDeviceIds which usues getAllDevices. This causes unnecessry overhead as new devices can only be added by some action within your adapetr. So you should keep the list of active devices within your code and not request a scan of objects every 15s. It's completly ok to scan objects / states once after starting up the adpter but you should case this data locally and use the cached information while the adapter is running.

• Handling of device onlinestatus could be improved too. You do not need to ever read the onlineStatus of a device using getState. You can simply cache this information internally too. And you can eiterh set the state every 15s during poll or use setStateChanges if you really do not want to update the state with the some value again.

• Does it make any sense to have more than one instance of this adapter running? Would this work at all? If not, plese set flag singletonHost at io-package,json to block installation of multiple instances.

• You are mixing communication based on http and udp. It looks like you poll the target device using routine httpState. But this routine is only called (or better firt time started) from 'initObjects'. But if this is correct then 'deleteOldObjects' would be called previously. But you may not delete all objects whenever the adapter starts even if they are recreated afterward as this would destroy any costom setup users have applied (i.e. history). States should only be deleted if user explizitly request this, i.e. be deleting some configuration. As you do not have any device confoiguration this cannot be the case. States must not be deleted if a device is not reachable during startup i.e. as it might be offline for some maintance only. Please explain the behavior during startup. Id think delting states is no option for this adapter at all.

Well maybe some more issues to be discussed arise later. I'll suggest you check the listed once above for the moment. Please let us know if you have any questions.

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!

[chuanlingZhao]

Hello @mcm1957,
Thank you very much for reviewing my adapter and for your prompt response. We have received your message and are currently carefully reviewing and addressing the issues mentioned above.

[mcm1957]

reminder 29.1.2025

[mcm1957]

@chuanlingZhao

To verify the Object structure of this adapter during RE-REVIEW please export the object structure of a working installation and attach the file in addtion to fixing or commenting the issues mentioned above. You find a guide how to export the object struture here: https://github.com/ioBroker/ioBroker.repochecker/blob/master/OBJECTDUMP.md

Thanks

reminder 10.2.2025

[chuanlingZhao]

Hello @mcm1957,
Thank you for your detailed feedback and questions. Based on the issues you reported, we have made a revision and updated the code accordingly. However, there are still some questions that we need to discuss with you.

• Item six in the list: I used the command you provided, npm run translate, but the content remains unchanged.

• Item eight in the list: We have internally discussed the use of port 9989 and will notify users in the documentation.

• Item ten in the list: We didn't use the object ID containing a dot as the state ID. In the createObjects function, an object ID like A1.Current was split, and the split part A1 was used to create a channel. https://github.com/Refoss/ioBroker.refoss/blob/main/lib/protocol/base.js#L46

The remaining questions:

• The onlineSatusCheck function is used to monitor the status of all known devices. It performs a TCP ping every 15 seconds to determine whether the devices are offline. Every 15 seconds, onlineSatusCheck updates the online status of the devices. Before each data request, it checks the online status of the devices, and only continues to send the request if the device can be pinged. I'm not quite sure if this will cause unnecessary overhead here. https://github.com/Refoss/ioBroker.refoss/blob/main/main.js#L162

• The combination of HTTP and UDP communication is employed to meet different requirements. UDP is utilized for device discovery, while HTTP is used for subsequent data queries. The httpState function is designed to poll data from the target device via the HTTP protocol. Before invoking the httpState function, specifically when the adapter starts, a call to deleteOldObjects is made to clean up old or unused objects. Since there is no configuration interface available, users are unable to actively initiate an explicit request to clear old objects. Therefore, to be compatible with user - defined settings (such as historical data), the deleteOldObjects function will no longer be called for object deletion, and users are required to perform the deletion manually.

• Object structure of this adapter
  refoss.0.json

The above are my questions. Please conduct a re - review. I'm looking forward to your reply.