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

Clarify syncCharger logic #12876

Merged
merged 16 commits into from
Mar 29, 2024
Merged

Clarify syncCharger logic #12876

merged 16 commits into from
Mar 29, 2024

Conversation

MarkusGH
Copy link
Contributor

No description provided.

Clarify syncCharger logic
Fix formatting, simplify
Fix formatting
core/loadpoint.go Outdated Show resolved Hide resolved
core/loadpoint.go Outdated Show resolved Hide resolved
MarkusGH and others added 6 commits March 11, 2024 20:49
Co-authored-by: Michael Heß <GrimmiMeloni@users.noreply.github.com>
Fix compile errors
Fix compile error
Fixed comments
Factorize, improve debug messages
Fix compile error
@MarkusGH MarkusGH marked this pull request as draft March 12, 2024 11:26
@MarkusGH
Copy link
Contributor Author

@GrimmiMeloni: Habe noch eine Verschönerung vorgenommen, sorry für die Last Minute Änderung. Jetzt ist aber Ruhe...

@MarkusGH MarkusGH marked this pull request as ready for review March 12, 2024 11:46
@andig
Copy link
Member

andig commented Mar 13, 2024

Sollen wir damit mal +1 Release warten um die Effekte auseinander zu halten?

@andig andig added the enhancement New feature or request label Mar 13, 2024
@MarkusGH
Copy link
Contributor Author

Für mich ok - hat ja keine Eile

@MarkusGH
Copy link
Contributor Author

@andig: Ich glaube wir können das so langsam übernehmen. Ich kriege es aber leider nicht hin die "Konflikte" aufzulösen. Habe diese gecheckt, es sind meines Erachtens gar keine echten Konflikte, der Code müsste kompatibel sein. Könntest Du das bitte zusammenführen?

@andig
Copy link
Member

andig commented Mar 26, 2024

Mhhm, das ist grad nicht ganz offensichtlich- da muss man sich mal beide Varianten nebeneinander legen :/

@MarkusGH
Copy link
Contributor Author

Hab ich schon. Das passt.

Include recent changes
@MarkusGH
Copy link
Contributor Author

@andig: War tatsächlich nicht offensichtlich, mir ist beim Überprüfen der Änderungen was durchgerutscht. Aber jetzt kannst Du tatsächlich alle Konflikte zugunsten patch-4 auflösen.

Added recent changes
Copy link
Member

@andig andig left a comment

Choose a reason for hiding this comment

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

Ich schaus grad nochmal an und es ist mir (wieder) nicht 100%ig klar :O

core/loadpoint.go Outdated Show resolved Hide resolved
core/loadpoint.go Show resolved Hide resolved
@andig andig merged commit 395da6d into evcc-io:master Mar 29, 2024
7 checks passed
@andig
Copy link
Member

andig commented Mar 31, 2024

@MarkusGH mir fällt gerade auf, dass in pvScalePhases auch noch Sync-Logik drin steckt die vielleicht auch eine bessere Heimat finden könnte?

// observed phase state inconsistency
// - https://github.com/evcc-io/evcc/issues/1572
// - https://github.com/evcc-io/evcc/issues/2230
// - https://github.com/evcc-io/evcc/issues/2613
measuredPhases := lp.getMeasuredPhases()
if phases > 0 && phases < measuredPhases {
	if lp.chargerUpdateCompleted() {
		lp.log.WARN.Printf("ignoring inconsistent phases: %dp < %dp observed active", phases, measuredPhases)
	}
	lp.resetMeasuredPhases()
}

@MarkusGH MarkusGH deleted the patch-4 branch April 1, 2024 16:00
@MarkusGH
Copy link
Contributor Author

MarkusGH commented Apr 1, 2024

@MarkusGH mir fällt gerade auf, dass in pvScalePhases auch noch Sync-Logik drin steckt die vielleicht auch eine bessere Heimat finden könnte?

ich weiß nicht, da wird ja nicht wirklich ein Zustand synchronisiert.

Aber einen Check auf phaseSwitchCompleted() sollte man da einbauen: #13253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants