Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added adapter for WARP chargers to sources-dist.json #1751

Merged
merged 1 commit into from
May 19, 2022

Conversation

pottio
Copy link
Contributor

@pottio pottio commented Mar 30, 2022

@GermanBluefox GermanBluefox added the auto-checked This PR was automatically checked for obvious criterias label Mar 30, 2022
@ioBroker ioBroker deleted a comment from Apollon77 Mar 30, 2022
@Apollon77 Apollon77 added the Adapter-Review needed A developer from the ioBroker Team will review the adapter, will provide comments or require changes label Apr 5, 2022
@pottio
Copy link
Contributor Author

pottio commented Apr 28, 2022

Ich habe bereits einige positive Rückmeldungen erhalten, sowohl für WARP 1, als auch WARP 2 Charger. Was fehlt noch, damit der PR gemerged werden kann?

@Apollon77
Copy link
Collaborator

Der Adapter-Review durch mich fehlt noch.

@Apollon77
Copy link
Collaborator

Hi, here some Review feedback:

  • (Must) Please do not use setObject for state objects! e.g. https://github.com/pottio/ioBroker.warp/blob/c6a19cd1aead22549e638144b6f60de91ddb380f/src/warp/warp-service.ts#L285 ... better option is setObjectNotExistAsync or extendObjects (yes extend also works when object do not exist before)
  • Question/Hint: How many states that allow actions you usually have? also: how many single subscribes you do? Starting with 100 or such it is cheaper in sum to do a "*" subscribe and filter inconing data on receive :-)
  • Info: The password encryption can be made more simple with the new options like "encryptedNative" and such , but works. But you should set protectedNative for "Passwordd" field to prevent access to the encrypted password from other adapters
  • Info: The "news" entry for 0.0.2 in io-package seems completely broken.
  • Info: In unload/terminate ... in fact no need to unsubscribe because this will happen automatically ... maybe tear doown your service first ... https://github.com/pottio/ioBroker.warp/blob/c6a19cd1aead22549e638144b6f60de91ddb380f/src/warp/warp-service.ts#L56
  • Info: Do you think the idea with "create objects within first 60s after start" is really the best approch? you can also simply remember which devices you initialized already or such ... more flexible and especially old systems could have issues in load situations with 60s :-)

Please adjust the first point (and maybe more if you like) and publish a new version and poke again here. Thank you.

@Apollon77 Apollon77 added question Something needs to be done or answered by the adapter developer must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review and removed Adapter-Review needed A developer from the ioBroker Team will review the adapter, will provide comments or require changes auto-checked This PR was automatically checked for obvious criterias labels May 16, 2022
@GermanBluefox GermanBluefox added the auto-checked This PR was automatically checked for obvious criterias label May 17, 2022
@ioBroker ioBroker deleted a comment from pottio May 17, 2022
@Apollon77
Copy link
Collaborator

@pottio Ready for re-review?

@pottio
Copy link
Contributor Author

pottio commented May 17, 2022

Hi, thanks for your feedback.

  • (Must) Please do not use setObject for state objects! e.g. https://github.com/pottio/ioBroker.warp/blob/c6a19cd1aead22549e638144b6f60de91ddb380f/src/warp/warp-service.ts#L285 ... better option is setObjectNotExistAsync or extendObjects (yes extend also works when object do not exist before)
    > Changed object creation/update to extendObjectAsync(...)
  • Question/Hint: How many states that allow actions you usually have? also: how many single subscribes you do? Starting with 100 or such it is cheaper in sum to do a "*" subscribe and filter inconing data on receive :-)
    > In total less than 20 states depending which WARP charger is used
  • Info: The password encryption can be made more simple with the new options like "encryptedNative" and such , but works. But you should set protectedNative for "Passwordd" field to prevent access to the encrypted password from other adapters
    > Used "encryptedNative" instead of unnecessary implementation within adapter/admin page. Also protected "user" and "password" via "protectedNative" feature
  • Info: The "news" entry for 0.0.2 in io-package seems completely broken.
    > Fixed
  • Info: In unload/terminate ... in fact no need to unsubscribe because this will happen automatically ... maybe tear doown your service first ... https://github.com/pottio/ioBroker.warp/blob/c6a19cd1aead22549e638144b6f60de91ddb380f/src/warp/warp-service.ts#L56
    > Removed unnecessary state unsubscribe implementations
  • Info: Do you think the idea with "create objects within first 60s after start" is really the best approch? you can also simply remember which devices you initialized already or such ... more flexible and especially old systems could have issues in load situations with 60s :-)
    > Created a new improvement issue for that topic (Improve object/device initialization pottio/ioBroker.warp#32)

Please review again. Thanks!

@Apollon77
Copy link
Collaborator

Hi,

near to complete :-)

  • with the ecnrypt change please add a common.globalDependencies with admin >= 4.0.9 to io-package.
    Please add that and release update to npm and we are redy to merge! Thanky ou for your support!

Ingo

@pottio
Copy link
Contributor Author

pottio commented May 18, 2022

Hi, I added the common.globalDependencies with admin >= 4.0.9 to io-package and deployed a new version :)

@GermanBluefox
Copy link
Contributor

Automated adapter checker

ioBroker.warp

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W400] Cannot find "warp" in latest repository

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

@ioBroker ioBroker deleted a comment from pottio May 18, 2022
@Apollon77
Copy link
Collaborator

Thank you, and welcome to the repo :-)

@Apollon77 Apollon77 merged commit f2471d4 into ioBroker:master May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-checked This PR was automatically checked for obvious criterias must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review question Something needs to be done or answered by the adapter developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tinkerforge WARP 2 Wallbox
3 participants