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

Ford: async soc refresh #1001

Merged
merged 9 commits into from
May 13, 2021
Merged

Ford: async soc refresh #1001

merged 9 commits into from
May 13, 2021

Conversation

andig
Copy link
Member

@andig andig commented May 11, 2021

replace #1000

/cc @MrJayC

err = fmt.Errorf("refresh failed: status %d", res.Status)
// if status attribute in JSON response is 200, update is complete, otherwise server is still
// waiting for vehicle and the request needs to be repeated
if err == nil {
Copy link
Sponsor Contributor

@MrJayC MrJayC May 11, 2021

Choose a reason for hiding this comment

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

v.refreshId muss zurückgesetzt werden wenn err != nil

Copy link
Member Author

Choose a reason for hiding this comment

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

Das ist ne gute Frage. Könnte bei der nächsten Iteration ja klappen (Netzwerkproblem)? Aktuell würde das weiter passieren bis nachgelagert ErrTimeout kommt. Was meinst Du?

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Guter Punkt, dann muss aber die Bedingung für den Timeout geändert werden (Zeile 206).
if res, err = v.refreshResult(); err != nil && time.Since(v.refreshTime) > fordRefreshTimeout {

Copy link
Member Author

Choose a reason for hiding this comment

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

willst Du's Dir nochmal greifen?

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

ich kann in dein Repo nicht pushen - für die eine Zeile rentiert sich nicht einen neuen Pull-Request zu öffnen (oder es gibt einen Trick, den ich nicht kenne)

Copy link
Member Author

@andig andig May 13, 2021

Choose a reason for hiding this comment

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

Eigentlich einfach

	if err != nil && time.Since(v.refreshTime) > fordRefreshTimeout {
		v.refreshId = ""
		if errors.Is(err, api.ErrMustRetry) {
			err = api.ErrTimeout
		}
	}

oder?

Copy link
Member Author

Choose a reason for hiding this comment

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

auch nciht, aber jetzt hab ich das problem verstanden, mom

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, neuer Versuch- denke es ist einfacher alle Logik an einem Platz zu haben. Jetzt gibts keine Kopfschmerzen mehr ;)

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

So ist es perfekt 👌
Ich würde sagen „das ist es“ (auf den PR bezogen). Ich zieh die letzte Version auf jeden Fall nochmal auf mein Produktivsystem zum Test, erwarte aber keine Fehler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Top, dann rein damit!

@MrJayC
Copy link
Sponsor Contributor

MrJayC commented May 11, 2021

2 Kommentare habe ich dagelassen, sieht sonst sehr gut aus! Ich werde morgen mal zum Testen einen Ladevorgang durchführen.

@andig andig mentioned this pull request May 13, 2021
@andig andig merged commit 79b67a3 into master May 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the mrjayc/ford-async branch May 13, 2021 20:13
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

2 participants