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

fix: if connecting to discovergy fails (timeout, no internet) "0W" were reported and no error logged #1402

Closed

Conversation

yankee42
Copy link
Contributor

Recently I experienced strange spikes in the energy chart. EVU would suddenly report "0 W" for a short moment. I think this was because I had some trouble with my internet connection and since I am using Discovergy as EVU this sometimes caused HTTP calls to timeout.

The behavior is unfortunate because naturally PV-charging will never happen is such a case, because there is never any export for a longer period of time.

Also it is unfortunate that the error is not logged, making it difficult to figure out what is wrong in the first place.

This PR attempts to fix this.

Unfortunately I have no means to test whether the code actually works, so $someone will need to do that. Also I did not find anything about the openwbDebugLog call. No definition, not what the parameters mean, anything... So I just guessed how this is supposed to be used. Where is this coming from?

@benderl
Copy link
Collaborator

benderl commented May 26, 2021

openwbDebugLog is defined in helperFunctions.sh in root. As all modules are currently called from loadvars.sh, they will get access to this function automatically.

Please add a counter for the case that the connection failes for several times. Then the module should fall back to the old behaviour of reporting zero watts.

@benderl benderl added change requested This issue or pul request needs changes enhancement New feature or request labels May 26, 2021
@yankee42
Copy link
Contributor Author

openwbDebugLog is defined in helperFunctions.sh in root. As all modules are currently called from loadvars.sh, they will get access to this function automatically.

Ah, found it :-).

Please add a counter for the case that the connection failes for several times. Then the module should fall back to the old behaviour of reporting zero watts.

Where can the counter be saved? I guess as some file on the ram-disk...? Is there any convention for the path?

@benderl
Copy link
Collaborator

benderl commented May 27, 2021

Ramdisk is the preferred location for such files. Maybe something like discovergyerror.counter?

@yankee42
Copy link
Contributor Author

I worked on this, but then I thought about the consequences of that behavior. Why do you want to assume 0 W. I have to admit that if EVU values are gone for an extended period of time the "last known" value might not be of any help. Actually it might be counter productive. However assuming 0W seems even worse. The best solution would probably be to assume constant house consumption, but... That's probably difficult to implement (I don't know enough of the code yet to judge on that).

Let's say that we are PV charging with a negative value for "manueller Regelpunkt". Setting EVU to zero will cause charge power to climb to maximum. Using a positive value for "manueller Regelpunkt" will cause charge power to fall to minimum.

This could of course also happen with the "last known value" (in case the last known value was above or below regulation threshold), however this is much less likely.

So I think if EVU values are gone for an extended period of time this will be bad either way. So writing, reviewing and testing code that detects this "extended period" and then replaces a bad situation with another bad situation seems like a waste of time?

(Of course if you really want to I will write the code, just making sure that this actually makes any sense...)

@yankee42 yankee42 force-pushed the discovergy-dont-fail-with-zero branch 2 times, most recently from 4a6c614 to 135722e Compare May 30, 2021 09:36
@yankee42
Copy link
Contributor Author

yankee42 commented May 30, 2021

In the meantime I reworked the whole PR. I changed everything to Python. I think it makes to code a lot cleaner. It also makes the code more efficient (the old code spawned 8 processes (7xjq, 1xcurl). With the python script this can all be done in a single process.

However most importantly it is much easier to unit test. I am a little bit surprised that I found zero tests so far in the whole software. Thus I added a test, but it is unfortunately not connected to any automation to also run it. Are there plans to automatic tests?

It will also be easier to implement the "revert to 0W" behavior, should you decide that you want that. Still waiting for feedback there.

Anyhow: Differences between the old behavior and my python version are:

  • The old script always wrote a newline character at the end of the files it is writing in the ramdisk. I expect this not to be necessary, so I skipped that changed that. I also output newline characters now. I found lines like wattbezugint=$(printf "%.0f\n" $wattbezug), so I guess the linefeed is necessary after all.
  • The old script was always rounding down, while the new script uses rounding half-up. I think that is more accurate and makes more sense.

@yankee42 yankee42 force-pushed the discovergy-dont-fail-with-zero branch from 135722e to a3b0285 Compare May 31, 2021 07:39
@yankee42
Copy link
Contributor Author

Would be nice to get some feedback...

@yankee42 yankee42 force-pushed the discovergy-dont-fail-with-zero branch from a3b0285 to 29189b8 Compare September 29, 2021 14:33
@yankee42
Copy link
Contributor Author

I am somewhat sad, that this is not even getting a response at all. I see that @benderl in the meantime duplicated parts of my effort to port the script to python. I rebased my branch nevertheless. It still has some advantages:

  • The script is shorter
  • The script is structured in functions with meaningful names (e.g. the current version only has a single function with the name get_value (you will never guess what it does: It writes a value to a file).
  • It supports passwords with spaces (support for passwords with spaces was added in 8fd442d and removed in ae95104)
  • It has automated tests
  • In case of a failure to receive values from discovergy an error is logged

If there is something wrong with this PR I am open for suggestions. Actually if this kind of PR is not wanted I'll of course accept your decision and you can close it. However it is somewhat sad, that I put a lot of effort into this and not getting a reply for several months (but in the meantime someone has time to duplicate part of my efforts...)

@yankee42 yankee42 force-pushed the discovergy-dont-fail-with-zero branch from 29189b8 to ed360e7 Compare November 18, 2021 14:10
@yankee42
Copy link
Contributor Author

yankee42 commented Jan 5, 2022

Aufgrund diverser Änderungen in anderen PRs obsolet. Das Verhalten ist nun auf völlig andere Art und Weise ohnehin drin.

@yankee42 yankee42 closed this Jan 5, 2022
@yankee42 yankee42 deleted the discovergy-dont-fail-with-zero branch January 5, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change requested This issue or pul request needs changes enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants