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.

[mcm1957]

@chuanlingZhao

Thanks for fixing and feedback.

The following issues seem to be open still:

• Why do you subscribe to states ?

  https://github.com/Refoss/ioBroker.refoss/blob/13d582e2918d575c3afe36e114a0515821620e86/main.js#L34
  As there is no onStateChange code doing any useful porpose (or I missed it), please remove subscription or explain the reason for it.

• binding to fixed ports

  OK, you do not specify why you want to use a fixed port. But thats finally you decision. Maybe refoss devices are not capeable to use different port. Anyway be warned that any port could be occupied by another application already.

  But due to the fact using a fixed port multiple Instances of this adapter cannot be installed. They would all try to use the same port which will for sure not work. So you must limit your adapter to one instance per host. You can use attribute common.singletonHost for this purpose: see https://github.com/ioBroker/ioBroker.js-controller/blob/1e5ae5ec4b6b1d75e2b14ffe7dbaa28dc25d8a34/schemas/io-package.json#L1415

• some labels seem to have no translation into all languages

  As your Adapter does not seem to have any configuration options (jsonConfig is now empty, adminUI:conig is set to none) anymore Please delete the i18n tree completly as no text used for adminUI exists at all.

  Only as an explanation:

  Currently no translation key / text pairs are present at i18n/en/translations.json. This file must be populated by you. 'npm run translate' will then do the translation jib but this script does not know which test you use. It reads i18n/en/translations.json and creates all other language files.

• As adpter does not have any config, jsonConfig is outdated

  As your adapter states that it does not have any config (https://github.com/Refoss/ioBroker.refoss/blob/13d582e2918d575c3afe36e114a0515821620e86/io-package.json#L96) jsonConfig.json is useless and can be removed.

• Std workflows do not run

  You have an error inside test-and.release.yml. Github actions reports:

  [Invalid workflow file: .github/workflows/test-and-release.yml#L48]
(https://github.com/Refoss/ioBroker.refoss/actions/runs/13192765131/workflow)

You have an error in your yaml syntax on line 48

see PR provided.

• Please activate linter and use eslint 9

  Your current setup is not ready for eslint. 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. Of course you may setup your own rules if really preferred.

• You call onlineSatusCheck every 15s. There you query all devices known by the adapter Instance using getAllDeviceIds which usues getAllDevices. This call must access the state database and hance this causes unnecessry overhead as new devices can only be added by some action within your adapter. 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 aadpter but you should cache this data locally and use the cached information while the adapter is running.

  you wrote:
  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.

  It's OK to ping whenever you feel the need to do so. Thats completly your decision. The remark refers to the usage of this.getDevices. You should avoid to query the ioBroker state database when it's nozt needed. You can easily cache the list of known devices and use this list to loop over all devices without any access to the ioBroker database. As state above its completly OK to use this.getDevices once during startup of the adapter - but for the recurrent loops please use a local cache / local array. And please avoid querying state values too. As these values are readonly and hence can be set by your adaptercode only, it would be easy to cache the values too if you need to detect changes. The goal is to reduce the read access to the iobroker state/object database whenever it is possible without much effort.

• 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 this.setStateChangedAsync if you really do not want to update the state with the some value again.

  https://github.com/Refoss/ioBroker.refoss/blob/13d582e2918d575c3afe36e114a0515821620e86/main.js#L118

  Most code of this function seems to set a state value only if it has changed. If this reading is ok you could simply use function setStateChangedAsync which does exactly this operation. In any case do not read state valaues over and over but cache them locally at your code if required. See https://github.com/ioBroker/ioBroker.js-controller/blob/1e5ae5ec4b6b1d75e2b14ffe7dbaa28dc25d8a34/packages/adapter/src/lib/adapter/adapter.ts#L374

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

  I could not detect the place where you ensure that no invalid characters are received. At
  https://github.com/Refoss/ioBroker.refoss/blob/13d582e2918d575c3afe36e114a0515821620e86/lib/protocol/base.js#L132 you construct the deviceId based on deviceInfo.devName which you recived via udp from an external source. Filtering / replacement of illegal characters must be done within your code somehow. Please add or advise if I missed the filtering step.

• State / Object Ids containing spaces

  Please remove white spaces from objectIds (StateIds) and i.e. replace them with "_". Although whitespaces are valid due to historical reasons its known that several otehr adapters, especially visualization adapters have troubles processing them. So it's good practice and strongly recommended to avoid them. As your adapter is new, please remove spaces from state / object Ids.

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

  • value.powerFactor is no valid role. As no matching role exists, please use value

• checking of received messages

  I might be wrong, but it looks like your code does not validate the received message at https://github.com/Refoss/ioBroker.refoss/blob/13d582e2918d575c3afe36e114a0515821620e86/lib/protocol/udp4.js#L60 in any way. So if any message is received which is not properly formatted as json your adapter will crash. Please check this (capture exception from parsing) and if not yet done handle the case. Recieving invalid data must not crash the adapter nor must this cause invalid objects to be created. So devName must be checked to contain only valid characters and uuid should be checked to be properly formatted as an uuid too. You may not rely on receiving ony valkid messages. The json could be corrupt and if its a valid json some elements might be missing or contain invalid data too.

---

Finally I'll anser / reply to your infos:

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

OK - see remarks above (delet i18n and jsonConfig.json)

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

OK - see above. Please set singletonHost

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

OK - object tree is created correctly. Please adapt Ids containing spaces and add filtering. Deatils see above.

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

See my repsonses above. Access to ioBroker Database should still be reduced. Ping operation itself is ok.

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.

OK - thanks for explanation.

Object structure of this adapter
refoss.0.json

OK - Thanks for providing dump.

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!

Please attach a new object dump after fixing the object related remarks.

reminder 27.2.2025

[chuanlingZhao]

Hello @mcm1957,
Regarding the issue of detecting connected devices by sending UDP packets, we have decided to send UDP broadcasts 3 times when starting the adapter and also add periodic UDP broadcasts during the adapter's runtime. We would like to ask for your opinion on what would be an appropriate time interval for this periodic broadcast.

[mcm1957]

Hello @mcm1957, Regarding the issue of detecting connected devices by sending UDP packets, we have decided to send UDP broadcasts 3 times when starting the adapter and also add periodic UDP broadcasts during the adapter's runtime. We would like to ask for your opinion on what would be an appropriate time interval for this periodic broadcast.

Completly OK

Please see my re-review lisr above. I've just finalized it.

[chuanlingZhao]

I have made all the modifications based on your feedback. Please review them again.