New Adapter iobroker.emporia

[Chris-656]

New adapter iobroker.emporia

Checklist
[ ] All requirements to bring a new adapter into Stable repository are fulfilled (see https://github.com/ioBroker/ioBroker.repositories#requirements-for-adapter-to-get-added-to-the-stable-repository)
[ ] Please add a link to Forum testing thread, if existing.

What will happen after creating this request?

• We will do a short additional review of your adapter
• After this we add the adapter to the stable repository

Thank you for your new adapter in ioBroker!

[Chris-656]

new name

[Chris-656]

New adapter iobroker.emporia

[mcm1957]

Its at least unusual to add an Adapter to latest and stable at the same time. Please see README.md describing the normal sequence.

I would suggerst to remove addition to stable und create a second PR after this adapter is reviewed and added to latest and at leaset some users have tested it using latest repo.

But feel free to wait for any response from @Apollon77 - he will finally decide this topic

[mcm1957]

Adapter repo: https://github.com/Chris-656/ioBroker.emporia

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

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

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

• Please remove addition to stable repository until your adapter has passed some time at latest repository.

Thanks for reading and evalutaing this suggestions.
McM1957

[Chris-656]

See questions below in your text regards Chris

…

________________________________ 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<https://github.com/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. * consider using adapter.setTimeout / this.setTimeout instead of (standard) setTimeout :Done 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 and limited. Not clear to me. In index_m.html there is the limitation implemented <input type="number" class="value" id="refresh" min="5" max="1000" /> Is this not sufficient ? * 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. Ok understand. Try to implement a time out for the state active * Please remove addition to stable repository until your adapter has passed some time at latest repository. Done Thanks for reading and evalutaing this suggestions. McM1957 regards Chris

[mcm1957]

• check and limit configurable timeouts / intervals
  :
  Not clear to me. In index_m.html there is the limitation implemented

OK, I did not check index_m.html. Limiting could be OK there. But users could change the value within objects too (manually). So personally I prefer to check the limits insode code too. But its your decision whether a double check is appropiate. I do not think, this would be blocking at Apollons check.

• check and limit intervals used to access cloud services

          Ok understand. Try to implement a time out for the state active
  

No I think my comment was misleading. I do not think that a timeout is required. The important point is to limit polls from cloud to a maximum rate. The user should not be allowed to poll i.e. every second while a poll every minute would be more than sufficient (i.e. when polling whether data od currency exchange rates, ...) As notes above you limit to 5 (seconds I assume) so this limit ist low but might be ok. The problem which should be addressed is, that we already had situations where the cloud provider access from iobroker was blocking due to inappropiate high load caused by few but much to heavy polling.

Thanks for reading and reacting on the suggestions.

McM

[mcm1957]

Please remove node 14 from test-and-release.yml and (optionally) add node 20 to testing matrix.
Node 14 is no longer supported and tests will fail since 22.5.2023.

Thanks
McM1957

[mcm1957]

I would like to give some additional (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.

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

  role 'name' does not exist, please use an existing role, i.e. 'text'

• adapt testing to supported node releases

  As node 14 is end of life and will no longer be supported by js-controller 5.x, consider removing node 14.x tests from matrix id still present.
  Tests for node 16 is mandatory unless you require node 18 and higher.
  Tests for node 18 is mandatory as many users already use node 18 and node 16 will be eol and deprected Q3/2023.
  Consider adding node 20 tests unless you already know incompatibilities.

  So the recommended testmatrix is [16.x, 18.x, 20.x],

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!

[mcm1957]

How shall we continue?
Please let us know if you plan zu fix the noted issues in the near future - or as an alterntive - like to discuss some / all of those points.

[github-actions]

Automated adapter checker

ioBroker.emporia

👍 No errors found

• 👀 [W156] Adapter should support admin 5 UI (jsonConfig) if you do not use a React based UI
• 👀 [W202] Version of package.json (0.0.12) doesn't match latest version on NPM (0.0.10)
• 👀 [W400] Cannot find "emporia" in latest repository

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

[Chris-656]

New adapter iobroker.emporia

[Chris-656]

@Chris-656

How shall we continue?

Please let us know if you plan zu fix the noted issues in the near future - or as an alterntive - like to discuss some / all of those points.

Looks like only adaption of node release (16 to 20, remove 14) was missing.

Please add any comment if you still maintain this adapter and want it listed at repositories If we do not receive any comment until mid of November this PR might be closed.

I changed the role names and added V 20.x in the tests. How we can now proceed ?

[mcm1957]

@Chris-656

Why did you add the adapter to stable (again) ?
Please remove changes to sources-dist-stable.json.
Adapter must first be available at latest bvore addition to stable con be considered.

Please add a comment when done!

[mcm1957]

@Chris-656
I did not detect you removal if stable as you did not comment.
I'll continuer asap
No further action required by you

[mcm1957]

RE-CHECK!

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