add iopooleco to latest

[mule1972]

Add iopooleco Adapter

[mcm1957]

please fix urls at PR

https://raw.githubusercontent.com/ioBroker/ioBroker.iopooleco/main/io-package.json

repo owner is NOT iobroker but (most likely) mule1974

[mcm1957]

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

I would like to give some (preliminary) feedback based on my personal oppinion. This is NOT an offical review and @Apollon77 might have several additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him (or wait for a response from him) if you cannot follow my suggestsions or before you spend major effort.

• Readme.md should contain a link to the manufacturer and/or device

  The README.md of any adapter related to device or m ultiple devices of a manufacturer should contain a link the manufacturers website and/or to a description of the device. This is to anable new users to easily check wether they associate the adapter with the correct device(s). (See https://github.com/ioBroker/ioBroker.repositories#requirements-for-adapter-to-get-added-to-the-latest-repository paragraph 4.)

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

  The paramater 'apikey' sound like something which should be protected. Please check.

• sensitive information should not be logged into logfile

  To avoid publishing sensitive information when forwarding logs, those information should never be written into the logs but either omitted or at least partly masked so that the information cannot be misused. This relates at least to usernames, passwords, uniqui (user assigned) keys and tokens. In addition personal information like name, address, gegraphical coordinates should not be logged too. Refers to 'apikey'.

• avoid not required logging

  Logging at info, wraning or error level should only be performed if the information is really useful for the user.
  'this.log.info(check if exists pooldevice: ${id.id})' is most likely a sort of debug information. Please log such info using level 'debug'

• 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 retrieve ids from the response of the website. Please check those ids and filter out invalid characters. Characters matching FORBIDDEN_CHARS must be filtered. As a good praxis for new adapter filetr out all characters other than a-zA-Z0-9,_ adn replace them with _ .

• [ ]processing of last timestamp
  Why do you retrive the state latestMeasure.measuredAt and process other attributes only if it changes? If you configure appropiate poll rates, it should not be a problem to update values even if they did not change,

• folders must be created

  You create a state '.latestMeasure.temperature' but I do not see a plce where you create the folder '.latestMeasure'. see https://github.com/mule1972/ioBroker.iopooleco/blob/f32d3be7befbf20a393ea6359fbad45a19bb0e02/main.js#L95

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

• unused variables

  Some variables seem to be unsused

    	this.pollInterval = 0;
    	this.pollTimeout = null;
  

• latestMeasure.isValid

  It looks like isValid is never set to false as states are only update if latestMeasure.isValid is trur. Please check.

• scheduled adapters should have random delays

  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.

• do not use cron for adapter which start very often

  You cron cponfig schedules the adapter to start every minute. If this is really desired / required (which seesm to be inappropiate) consider to move to a deamon mode. Overhead starting an adapter so often is too big. Schedules adapters should not start more then every 5 to 10 minutes.

• check and limit intervals used to access cloud services

  Limit all intervals used to access cloud services to a reasonable value. If users configure an inapprobiate access rate this could lead to suspending the service by the provider for all ioBroker users. Polling every minute seems to be much to often. None of th epool paramaters will change significant witrhin such a short time

• do not access cloud services at fixed timestamps

  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.

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!

[mule1972]

• Readme.md:
  has been updated with link to manufacturer web page

• sensitive information:
  API key is now being encrypted and protected

• avoid not required logging:
  checked and updated

• invalid characters should be filtered from object ids:
  fixed

• processing of last timestamp:
  I would like to ensure that states only get updated if new measurement data is being received. For e.g. pools are usual closed during winter and meter devices like ECO from iopool are therefore set into a winter mode which means there is no more measurement data for months. Other scenarios are failure or maintenance of the meter device. For those scenarios I think it would not be a good idea to always update the states on regular basis without any new measurement data.

• folders must be created:
  fixed

• reevaluate state roles:
  checked it but did not find what you would like to change me. Therefore please point me to an example of what is wrong or should be changed.

• unused variables:
  fixed

• latestMeasure.isValid:
  Only valid measurements should be saved. Therefore latestMeasure.isValid has been removed from iobroker data set.

• scheduled adapters should have random delays:
  fixed

• check and limit intervals used to access cloud services:
  Official API guidelines from iopool defines min request frequence to 1 second (Public API usage guidelines iopool/community#8) but I now changed the interval to every 15 minutes (usual measure frequence of the devices from iopool) and synced the scheduler with the measuredAT timestamp in order to get most current measurement data

• do not access cloud services at fixed timestamps:
  see comments before

[github-actions]

Automated adapter checker

ioBroker.iopooleco

👍 No errors found

ioBroker.iopooleco

👍 No errors found

• 👀 [W171] "common.title" is deprecated in io-package.json
• 👀 [W400] Cannot find "iopooleco" in latest repository

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

[mcm1957]

• Please review loggings once more (non blocking)
  I do not consider it blocking, but logging i.e. start end end of every request is very likely filling the log without real information.
  see i.e. https://github.com/mule1972/ioBroker.iopooleco/blob/0e6158f68ce83ffaaa64098c97e9e30464708d7c/main.js#L35
  Logging at start of a daemon adapter would be acceptable - a scheduled adapter should only log whts really improtant as it would log with every start.

[mcm1957]

@mule1972
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, ist OK to continue using this topic too.