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 deyeidc to latest #2187

Merged
merged 2 commits into from
Aug 21, 2023
Merged

Add deyeidc to latest #2187

merged 2 commits into from
Aug 21, 2023

Conversation

raschy
Copy link
Contributor

@raschy raschy commented Mar 21, 2023

Please add my adapter ioBroker.deyeidc to latest.

This pull request was created by https://www.iobroker.dev c0726ff.

@raschy
Copy link
Contributor Author

raschy commented Mar 21, 2023

Bluefox ist also invinited to npm

@ioBroker ioBroker deleted a comment from mcm1957 Mar 22, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Mar 22, 2023

Link to repo: https://github.com/raschy/ioBroker.deyeidc

adapter checker seems to be OK:
[W171] "common.title" is deprecated in io-package.jon
[W400] Cannot find "deyeidc" in latest repository

@GermanBluefox
Why does automatic checker report "No changed adapters found?"

@mcm1957
Copy link
Collaborator

mcm1957 commented Mar 22, 2023

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.

  • some error text is in german

    I would suggest to use english error messages or to support multilanguage error text, but not to use german only text as ioBroker is an international software.

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

Thanks for reading and evalutaing this suggestions.
McM1957

@Apollon77 Apollon77 added the Adapter-Review needed A developer from the ioBroker Team will review the adapter, will provide comments or require changes label Mar 22, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented May 18, 2023

RE-CHECK!

@GermanBluefox GermanBluefox 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 labels May 18, 2023
@mcm1957 mcm1957 removed the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label May 18, 2023
@ioBroker ioBroker deleted a comment from mcm1957 May 20, 2023
@raschy
Copy link
Contributor Author

raschy commented May 27, 2023

Please RE-CHECK the Release 0.0.7 from 27.05.2023

@ioBroker ioBroker deleted a comment from mcm1957 May 27, 2023
@raschy
Copy link
Contributor Author

raschy commented Jul 10, 2023

Please RE-CHECK the Release 0.0.9 from 10.07.2023

@github-actions github-actions bot deleted a comment from GermanBluefox Jul 10, 2023
@github-actions github-actions bot deleted a comment from mcm1957 Jul 10, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Jul 10, 2023

[W505] setTimeout found in "main.js", but no clearTimeout detected - false positive

@raschy
Copy link
Contributor Author

raschy commented Jul 18, 2023

Do I have to do anything else to get into the latest repository with the adapter?

@mcm1957
Copy link
Collaborator

mcm1957 commented Jul 18, 2023

Do I have to do anything else to get into the latest repository with the adapter?

No not st the moment.
Its just time to wait for official review by @Apollon77

thanks

@mcm1957 mcm1957 added (RE-)REVIEW pending (mcm1957) Changes requested by review have been applied, re-review could be done. and removed Adapter-Review needed A developer from the ioBroker Team will review the adapter, will provide comments or require changes labels Aug 5, 2023
@github-actions github-actions bot deleted a comment from mcm1957 Aug 8, 2023
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Automated adapter checker

ioBroker.deyeidc

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W400] Cannot find "deyeidc" in latest repository
  • 👀 [W505] setTimeout found in "main.js", but no clearTimeout detected

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

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 8, 2023

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

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.

  • Readme.md - text Developer Manual

    The header "Developer Manual" seems to be a left over.
    https://github.com/raschy/ioBroker.deyeidc#developer-manual

    But this is no blocking point for sure - it's onbly a cosmetic feedback.

  • onStateChange handler ignores ack flag

    The adapter must honor the ack flag when processing onStateChange events. Whenever the onStateChange handler is called for an own state (a state owned by the processing instance of the adapter) it must ignore this event if the ack flags is set. Only stateChanges with ack==false are allowed to be processed.

    When processing foreign states which are output of another adapter processing must only occure if the ack flag is true.

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!

Note: W505 setTimeout is a false positive.

@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 (mcm1957) Changes requested by review have been applied, re-review could be done. labels Aug 8, 2023
@raschy
Copy link
Contributor Author

raschy commented Aug 8, 2023

I would also like to thank someone for taking care of the adapter.
I can certainly change the title in the readme. But what should 'onStateChange handler ignores ack flag' tell me? At the moment I don't have a starting point where I need to make a change. Can I please get an explanation or a concrete action?

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 8, 2023

Current code is

	async onStateChange(id, state) {
		if (state) {
			// The state was changed
			//this.log.debug(`state ${id} has changed to ${state.val}`);
			if (id.indexOf('Power_Set') > 1) {
				await this.setPower(id, state);
			} else {
				await this.updateData(await this.computeData(id, state));
			}

You can simply change

    if (state) {

to

    if (state && !state.ack) {

Background:

Every state has an ack flag.
Whenever the adapter sets a value of an OWN state it also sets ack=true indicating the value is confirmed.
If some external source (user, script, other adapter) sets the value of an state it must set this value using ack=false indicating that that value is not yet confirmed by the target adapter. An adapter must only react if the ack-flag of an own state is false and ignore writes to own states with ack=true. (This is i.e. important if a state is written from external source but also by the adapter during updating values. Otherwise setting the value by adpatercode would trigger the adapter action and hence possibly result in some processing loop).

When listening to external states (States of other adapters) most likely the code should react on ack=true only.

@raschy
Copy link
Contributor Author

raschy commented Aug 8, 2023

Thank you for the quick answer and the detailed explanation.
In principle, that may be correct. In my adapter, all states are stored with ack:true. Since I check for my own state here, the check for if (state && !state.ack) will then never occur. To make this 'adapter compliant' I would have to check for if (state && state.ack). This would be feasible, but in my opinion it does not really help.

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 8, 2023

Thank you for the quick answer and the detailed explanation. In principle, that may be correct. In my adapter, all states are stored with ack:true. Since I check for my own state here, the check for if (state && !state.ack) will then never occur. To make this 'adapter compliant' I would have to check for if (state && state.ack). This would be feasible, but in my opinion it does not really help.

I do not really understand...
Who is writing to the subscribed states? Your own adapter or some external software (user, script, other adapter)?

Its ok if your OWN adapter writes with ack-flag true. But in this case I do not see why you would take the overhead of onStateChange processing?

External code should write with ack=false.

@raschy
Copy link
Contributor Author

raschy commented Aug 8, 2023

The data points that are subscribed to there are set by the adapter itself. I cannot judge how much overhead the onStateChange produces. I have not seen any other way to solve this.

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 8, 2023

As asking for another solution. I guess you could call "doProcessing" directly after a setState the same way as using an onSetState callback. So I do not see the problem to do it directly.

But I will leave this to @Apollon77 to decide whether its OK that a adapter communicates with itself using subscribed states.
For me its - except this point - OK.

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 8, 2023

@Apollon77
Please check / comment / merge

@mcm1957 mcm1957 added Adapter-Review needed A developer from the ioBroker Team will review the adapter, will provide comments or require changes and removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review labels Aug 8, 2023
@raschy
Copy link
Contributor Author

raschy commented Aug 8, 2023

I understand the concern about the correct, resource-saving way of working of an adapter. However, since the data points can be freely selected by the user and I do not know which data points are to be "used", I find it very difficult to find the other way at the moment. Nevertheless, I will pay special attention to this point in any further development.

@mcm1957 mcm1957 requested review from Apollon77 and removed request for Apollon77 August 8, 2023 18:13
@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 20, 2023

According to feedback at telegramm this logic is OK.
so lgtm ...

@mcm1957 mcm1957 added the lgtm Looks Good To Me label Aug 20, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 21, 2023

ok to release according to tg

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 21, 2023

@raschy

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.

@mcm1957 mcm1957 merged commit 8ac7978 into ioBroker:master Aug 21, 2023
9 checks passed
@raschy
Copy link
Contributor Author

raschy commented Aug 21, 2023

Thank you for including the adapter in the latest repository.
There is already a thread (https://forum.iobroker.net/topic/63899/neuer-adapter-cloudfreie-auslesung-von-deye-invertern) that I would like to continue.

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 21, 2023

Thats totally OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 lgtm Looks Good To Me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants