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 warning residualpower in combination with battery #9126

Merged
merged 10 commits into from
Sep 18, 2023

Conversation

VolkerK62
Copy link
Contributor

da auch bei negativen Werten eine Warnung kommt ist der alleinige Hnweis "is missing" missverständlich

da auch bei negativen Werten eine Warnung kommt ist der alleinige Hnweis "is missing" missverständlich
@StefanSchoof
Copy link
Contributor

Hilft hier wie bei Intervall unter 30s auch ein Link zu den Docs die den Hintergrund erklären?

@VolkerK62
Copy link
Contributor Author

gute Idee, bau ich noch ein

@naltatis
Copy link
Member

naltatis commented Aug 5, 2023

Was spricht hier eigentlich dagegen im Fall von nicht gesetzten residualPower einfach nen Default von 100 zu verwenden und nicht zu loggen?

//cc @premultiply

@StefanSchoof
Copy link
Contributor

Immer default 100 oder nur wenn eine Batterie im System enthalten ist?

@VolkerK62
Copy link
Contributor Author

MMn nur bei Batterie. Es wird ja schon geprüft und statt der Warnung wird es auf 100 gesetzt.

@VolkerK62
Copy link
Contributor Author

ich hab mich mal (blauäugig) daran gewagt ;)

@StefanSchoof
Copy link
Contributor

Ich denke, dass es zu Verwirrungen führt, wenn ohne Meldung ein gesetzter residual Power ignoriert wird. Würde vorschlagen, setzten und trotzdem eine Log Nachricht.

@StefanSchoof
Copy link
Contributor

StefanSchoof commented Aug 6, 2023

Vielleicht auch bei 0 mit Batterie auf 100 setzen und bei negativen Werten eine Warnung.

Dann gibt es im Default ein sinnvolles Verhalten und wenn jemand eine merkwürdiges Spezialanforderung hat, kann es es trotzdem auf Minus Werte setzen und die Warnung ignorieren.

@VolkerK62
Copy link
Contributor Author

Ok, ein Hinweis auf den "Eingriff" wäre nicht schlecht.
Aber, wie soll unterschieden werden, ob ein "falscher" Wert ( <=0 ) absichtlich gesetzt wurde?

core/site.go Outdated Show resolved Hide resolved
VolkerK62 and others added 2 commits August 6, 2023 16:55
so viel Aufwand, für einen Parameter ;)

Co-authored-by: StefanSchoof <4662023+StefanSchoof@users.noreply.github.com>
der Batterie Check war doppelt
core/site.go Outdated
@@ -182,7 +182,14 @@ func NewSiteFromConfig(
}

if len(site.batteryMeters) > 0 && site.ResidualPower <= 0 {
site.log.WARN.Println("battery configured but residualPower is missing (add residualPower: 100 to site)")
if site.ResidualPower == 0 {
site.log.WARN.Println("ResidualPower is set to 100, because battery is configured and residualPower is missing or = 0.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diese Logmeldung können wir uns mMn. dann sparen. Der normale Nutzer sollte gar nichts über residualPower wissen müssen und mit den Default gut fahren. Wenn er da doch was dran tweaken möchte findet er den Parameter und seiner Bedeutung über die Doku.

Einziger Punkte wäre, wenn er ihn explizit auf 0 setzt um ein bestimmtes Verhalten zu bekommen und wir seinen Wert dann überschreiben. Wir können hier ja leider nicht zwischen 0 und nicht gesetzt unterscheiden. Halte das aber für einen Edge-Case der auch eher in die Doku sollte.

Copy link
Contributor Author

@VolkerK62 VolkerK62 Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm ... da bin ich jetzt hin- und hergerissen.
Einerseits gebe ich dir recht

der normale Nutzer sollte gar nichts über residualPower wissen müssen

andererseits finde ich es gut, wenn über "Systemeingriffe" informiert wird.
Gibt es auch log.INFO?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wir können hier ja leider nicht zwischen 0 und nicht gesetzt unterscheiden.

@naltatis die Doku von Viper behauptet wir können das.

Make it easy to tell the difference between when a user has provided a command line or config file which is the same as the default.

Liefert isSet() ggf. false wenn ein Default gezogen wird?

@andig
Copy link
Member

andig commented Aug 17, 2023

Keine neuen Erkenntnisse- schließen?

@VolkerK62
Copy link
Contributor Author

Ich würde zumindest die Korrektur des Wortlauts der Warnung beibehalten wollen. 4c77fc9
In der weiteren Diskussion ging es ja darum, den Wert "automatisch" zu setzen.

@GrimmiMeloni
Copy link
Collaborator

GrimmiMeloni commented Aug 18, 2023

@VolkerK62 ich habe heute morgen mal rumgespielt. Es ist möglich zu erkennen ob ein Wert 0 als Default gesetzt wurde.
In other fliegt die Konfiguration rum. Eine Präsenz des Keys heißt, daß der Anwender den Wert (auch 0) explizit gesetzt hat.

	if _, ok := other["residualpower"]; !ok {
		site.log.WARN.Println("residualPower not set")
	} else {
		site.log.WARN.Println("residualPower IS set")
	}

Hilft Dir das für die angestrebte Automatik?

@VolkerK62
Copy link
Contributor Author

sorry, da muss ich passen. Kein Ahnung vom Code und das sind mir zu viele "if".

Ich würde es erstmal bei der Änderung der Warnung belassen wollen.
Da die Einstellungen (und configure?) ins UI wandern sollen, könnte man ja in dem Zusammenhang dann einen entsprechenden default 100 einbauen.

@VolkerK62
Copy link
Contributor Author

@andig ich wäre hier fertig.
Evtl kann man einen default in configure einbauen.

@github-actions github-actions bot added the stale Outdated and ready to close label Sep 13, 2023
@github-actions github-actions bot closed this Sep 18, 2023
@naltatis naltatis reopened this Sep 18, 2023
@naltatis naltatis merged commit 63e73cd into evcc-io:master Sep 18, 2023
@naltatis naltatis removed the stale Outdated and ready to close label Sep 18, 2023
naltatis pushed a commit that referenced this pull request Sep 19, 2023
* fix warning residualpower in combination with battery

da auch bei negativen Werten eine Warnung kommt ist der alleinige Hnweis "is missing" missverständlich

* add link to doku

* type

* set ResidualPower to 100 instead of warning

* wip

* wip

* wip

* 100 if missing, warning if negativ

so viel Aufwand, für einen Parameter ;)

Co-authored-by: StefanSchoof <4662023+StefanSchoof@users.noreply.github.com>

* wip

der Batterie Check war doppelt

* nur erweiterte Warnung

---------

Co-authored-by: StefanSchoof <4662023+StefanSchoof@users.noreply.github.com>
@VolkerK62 VolkerK62 deleted the patch-20 branch June 22, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants