Add heizoel24-mex to latest

[ltspicer]

heizoel24-mex added

[github-actions]

Automated adapter checker

ioBroker.heizoel24-mex

-

• ❗ [E115] No license found in io-package.json
• 👀 [W400] Cannot find "heizoel24-mex" in latest repository

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

[mcm1957]

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.

You will find the results of the review and evntually issues / suggestings as a communt to this PR. So please keep this PR watched.

If you have any urgent questions feel free to ask.

mcm1957

reminder 23.3.2024

[mcm1957]

Note:
[E115] No license found in io-package.json
can be ignored

[mcm1957]

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 should be written in pure english

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

• Readme.md contains multiple languages

  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 locatlized Readme (README-de.md or loclized pages in (docs/de and docs/en) like https://github.com/iobroker-community-adapters/ioBroker.pvforecast/tree/main/docs.

• 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"
  ],
  

• use standard logging mechanism

  Theres no need to provide a debug config option. ioBroker provides logging levels indo, warning, error, debug and if really needed silly. So please remove this.debug, remove option debug at config UI and use this.log.debug and/or this.log.silly whenever debug related data is to be logged.

• parameter checking / logging

  Paramters should be checkd early in code. So i.e. if the username is not empty but contains only spaces this should be detected earlier and result in an warning / error an terminatoion of the process. see https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L58
  and https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L41

• enhance debug information

  Information that data is received is fine, but I suggest to log the real data received (with debug level) as this could aid in analyzing a problem. see https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L201

• this.log.xxx is not async

  this.log.xxx does not require an await as the logger is no async function. Please remove awaits in front of this.log.xxx

• axios must be used with timeout

  The axios default timeout is 0 indicating no timeout. This might result in an blocked adapter if remote system dies not work as expected. Always add an appropiate timeout to axios calls either per call or as a default.

• log axios error information

  If axios raises an error the error information should be logged. It will be hard to solve a problem based on the hardcoded text 'ResultCode not 0. No session ID received!' or 'Error when fetching dashboard data';

• remove info.connection

  As this adapter is scheduled, info.connection does noz really make any sense. The adapter does not have a connection to a device. So I suggest to remove info and info.connection from io-package and from code.

• missing folder/channel object

  At line https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L82 you use an id 'Items.'+topic. I do not see a place where object Items is created. Every element of a state hirarchie must be created individually. You can do this by using steObjectNotExists or (if the object has a fixed name) by adding the objet to io-pacakge.json (see creating of info and info.connection).

• mqtt topics

  Are you sure that you want to publish mqtt topics to a hardcode rott without any unique identification? As far as I see this will cause conflicts if a user has two sensors and hence uses two (or more) instances of the adapter. Maybe you should include the sensor id into the topic hirachy.

• reevaluate write attribute

  You are creating states with write:true. This is most likely incorrect. write:true must be set only if the state is to be written by other adapters or the user (i.e. using java-script). And as this adapter cannot react on such state changes, no writes are useful / to be allowed anyway. see https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L89

• reevaluate state roles

  You are using types as roles. This is invalid. Type and role are totally different attributes.
  see i.e. https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L87

  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.

  As you know which data is provided (see topics array) you should be able to set the correct role for all states filled with data. I suggest to enhance the topics table to contain objects with Topicstring, Role and units - but feel free to choose another solution.

• use unit information

  As you know which data is provided (see topics array) you should be able to set the correct units for all states filled with data. I suggest to enhance the topics table to contain objects with Topicstring, Role and units - but feel free to choose another solution.

• scheduled adapters should have random delays and do not access cloud services at fixed timestamps

  An unmodified schedule adapter that queries data from external systems will produce a lot of issues because usually the default schedule stays unchanged for all users and so all installations out there will then contact the server at the exact same time create a "potentially huge" request spike. The best practice for now is at least to add a random delay to spread the calls within that minute - or even move the "minutes" on first start and so persist that (e.g. see https://github.com/ioBroker/ioBroker.yr/blob/master/main.js#L84-L90) ... if the adapter gets successfull then we have a big issue and the webpage owner contacting us ... we seiould prevent that from the beginning.

  Avoid accessing cloud services at fixed timestamps (i.e. at every full hour, at 5 minutes after midnight , ...) This could lead to high load peaks at the cloud service as all ioBorker installations will use the service at almost the same time. Use random timestamps where possible.

  Detect the default schedule (0 */3 * * *) and change it to use randoim minutes with tha hour. See oilfox adapter for an example (do not user adapter.stop but use this.terminate.Oilfox is a very old adapter. see https://github.com/iobroker-community-adapters/ioBroker.oilfox/blob/deddf861e8bda00cbaffb2d2a5362f2ddaaeb092/main.js#L134 for details.

• this.client usage

  Why do you use this.client as this varibale is used only local inside onReady? Define the variable using let instead and initailize it to null.
  Why do you set this.cleint to 'dummy' at https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L55? this.client is a an object normally. So its not a good idea to set it to a string in some cases. When initializing client ot null, you can omit the omplket else path anyway.

• please check logging

  Why do you log broker address here? https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/085c56483085149cc4996cf6d07f928390f05a7d/main.js#L68. If mqtt is used ist has been checked some lines above, if mqtt is not used its not relevant in my oppinion. But this is only a suggestion.

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 25.3.2024

[ltspicer]

I have tried to resolve the open issues.
Please check on occasion and let me know.

[mcm1957]

reminder 17.3.2024

[mcm1957]

Thanks for adjusting your code. It looks much, much better now.

Some more suggestions

• use objects instead of synchroniced tables (suggestion only, not blocking)

  I would suggest to use a solution like

const topic1 = [
  { 
    "id" : "DataReceived", 
    "role" : "indicator", 
    "unit" : "", 
    "type" : "boolean" 
  },
  { 
    "id" : "SensorId", 
    "role": "value", 
    "unit" : "", 
    "type": "number" 
  } ];

and access it like

  topic1[ii].role 

This would keep the infomartion at one place using objects and avoid double checking whether the 5th element is really the 5th one ...

• check sensorId better (required change)

  parseInt is a function in JavaScript used to convert a string into an integer. It parses through the string until it reaches a character that isn't a numeral, returning the integer formed by the preceding numerals. So '12xxx' will return 12 and be considred valid - although later this will not work. Check that really only digits are present. This check should be done within code - a check at admin UI is not sufficient as the data could be changed by user directly at the config object too.

• ensure that all object levels are created (required change)

  At line https://github.com/ltspicer/ioBroker.heizoel24-mex/blob/449a926c4a096d211aa92b1e5f5ba325a633bbeb/main.js#L147 you create an object named. I do not see the place where the objects 'sensor_id' and 'sensor_id + ".Items."' are created. Every level of an object must be created seperately. Often the structure is ... Objects ob type state must not have any subelements. Whether you use device and channel depends on the type of your adapter. You may use type too.

  This has already be noted but I think you missed it. If you are unsure what to do, please ask.

• remove info object (required change)

  You already have removed info.connection. Theres no need to keep info object - unless you use it somehow and I did not recognize it. So I suggest to remove the complete ( :-) ) info object from io-packages.json too.

• this.client usage (strongly suggested but not blocking)

  Why do you use this.client as this varibale is used only local inside onReady? Define the variable using let instead and initailize it to null. this should only be used for attributes (variables) which are attached to the (adapter) object and either are required to be access from other routiens (compareable to global variables) or must keep their value accross multiple invocations of a routine. Normal variables used inside a routine only should be initialized using 'const' or 'let'. Do NOT use outdated var keyword.

Feel free to ask if you have any questions. But please note, that I am not really reachable starting tomorrow and lasting app 1 week.

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 25.3.2024

[ltspicer]

I have fixed the open issues.
Please have a look when you get a chance.
Thank you for your efforts.

[mcm1957]

looks ok neow

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