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

Add adapter sun2000 to latest #3219

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Add adapter sun2000 to latest #3219

merged 1 commit into from
Feb 2, 2024

Conversation

bolliy
Copy link
Contributor

@bolliy bolliy commented Jan 18, 2024

I think that my additional adapter for Sun2000 is justified. Because it can process up to 5 inverters at the same time.

br stephan

@github-actions github-actions bot added 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 *📬 a new comment has been added labels Jan 18, 2024
@github-actions github-actions bot deleted a comment from bolliy Jan 18, 2024
@bolliy
Copy link
Contributor Author

bolliy commented Jan 18, 2024

@mcm1957 mcm1957 changed the title sun2000 adapter Add adapter sun2000 to latest Jan 18, 2024
@mcm1957 mcm1957 added RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. and removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review *📬 a new comment has been added labels Jan 18, 2024
@mcm1957
Copy link
Collaborator

mcm1957 commented Jan 18, 2024

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

@mcm1957
Copy link
Collaborator

mcm1957 commented Jan 18, 2024

reminder 01.02.2024

bolliy added a commit to bolliy/ioBroker.sun2000 that referenced this pull request Jan 24, 2024
* [Add sun2000 to latest](ioBroker/ioBroker.repositories#3219)
* improve error handling (#34)
* add simple optimizer info
* Riemann sum of input power with energy loss for new state `dailySolarYield`
* try to recreate the `yield today` from the fusion portal
@mcm1957
Copy link
Collaborator

mcm1957 commented Feb 1, 2024

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.

  • info states

    Why do you duplicate config information into ion.xxx states? This is at least unusal. Consider this as a remark, I do not consider it blocking,

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

Thanks for reading and evaluating this suggestions.
McM1957

reminder 12.2.2024

@mcm1957 mcm1957 added must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review and removed RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. labels Feb 1, 2024
@bolliy
Copy link
Contributor Author

bolliy commented Feb 1, 2024

Thanks for checking my adapter!

"info state": I suspected that if the states were deleted manually, they would not be automatically recreated after a restart the adapter.

„unused onStateChange handler“: I have remove the handler and push the changes to github.

Do I have to create a new adapter npm version so that the adapter can be included in the ioBroker repro?

@github-actions github-actions bot added the *📬 a new comment has been added label Feb 1, 2024
bolliy added a commit to bolliy/ioBroker.sun2000 that referenced this pull request Feb 2, 2024
* Requirements from [Add sun2000 to latest](ioBroker/ioBroker.repositories#3219)
@github-actions github-actions bot deleted a comment from bolliy Feb 2, 2024
Copy link

github-actions bot commented Feb 2, 2024

Automated adapter checker

ioBroker.elgato-key-light

Downloads Number of Installations (latest) Number of Installations (stable)
NPM

  • ❗ [E114] No adapter are allowed in the repo without admin support (set "common.noConfig = true" and "common.adminUI.config = none" if adapter has no configuration)

ioBroker.sun2000

Downloads Number of Installations (latest)
NPM

👍 No errors found

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

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

@mcm1957 mcm1957 removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review *📬 a new comment has been added labels Feb 2, 2024
@mcm1957
Copy link
Collaborator

mcm1957 commented Feb 2, 2024

"info state": I suspected that if the states were deleted manually, they would not be automatically recreated after a restart the adapter.

My remark refers to the fact that those states exist at all. Most adapters do not duplicate their own configuration into states at all. So I suggested to remove those states. But as I already wrote this is not blocking.

Do I have to create a new adapter npm version so that the adapter can be included in the ioBroker repro?
no, code change will go with next release anyway.

@mcm1957 mcm1957 added the lgtm Looks Good To Me label Feb 2, 2024
@mcm1957 mcm1957 merged commit e07d8c2 into ioBroker:master Feb 2, 2024
24 checks passed
@mcm1957
Copy link
Collaborator

mcm1957 commented Feb 2, 2024

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.

@bolliy
Copy link
Contributor Author

bolliy commented Feb 2, 2024

Thank you Martin for your support. I am pleased about the quick response!

@github-actions github-actions bot added the *📬 a new comment has been added label Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
*📬 a new comment has been added auto-checked This PR was automatically checked for obvious criterias lgtm Looks Good To Me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants