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 schedule for planner #16091

Draft
wants to merge 64 commits into
base: master
Choose a base branch
from

Conversation

Maschga
Copy link
Contributor

@Maschga Maschga commented Sep 13, 2024

Fix #5492

Hi,
dieser Draft soll den Support für das Planen für Wochentage hinzufügen.
Dafür fehlt noch folgendes: Ich kenne mich im evcc-System nicht so gut aus. Wenn etwas fehlt, dann bitte mir mitteilen.

  • Verhalten durchdenken, wenn Nutzer eine Zeit angibt, die nahe dem Sommer-Winter-Wechsel liegt
  • Übersetzungen ändern
  • Backend-Implementierung
  • Tests schreiben
  • Bug fixen: Uncaught (in promise) Maximum recursive updates exceeded in component <ChargingPlanRepetitiveSettingsEntries>. This means you have a reactive effect that is mutating its own dependencies and thus recursively triggering itself. Possible sources include component template, render function, updated hook or watcher source function.. Auslöser ist diese Zeile.
  • (Wie) in Preview Pläne visuell anzeigen?
  • Bug fixen: Siehe Video. Das Dropdown-Menu aktualisiert nur die Daten des dazugehörigen Select-Menus, wenn auf die Checkbox geklickt wird.
bug.mp4
  • Weiteres bitte mitteilen

Vielen Dank für das tolle Projekt!
~ Maschga

@andig
Copy link
Member

andig commented Sep 14, 2024

Was wäre denn hier die Idee fürs API?

@naltatis
Copy link
Member

Wording: Ich würd hier nicht repetetive verwenden. Da steckt von der Bedeutung was "ermüdendes", "monotones" drin. Lass uns repeating nehmen. Sowohl Richtung User als auch im Code.

@naltatis naltatis self-assigned this Sep 14, 2024
@naltatis naltatis self-requested a review September 14, 2024 22:12
@naltatis naltatis added enhancement New feature or request ux User experience/ interface labels Sep 14, 2024
@andig
Copy link
Member

andig commented Sep 15, 2024

Oder einfach Wochenplaner?

@Maschga
Copy link
Contributor Author

Maschga commented Sep 15, 2024

Wie soll ich es jetzt machen? 😅

@naltatis
Copy link
Member

Lass uns bei repeating bleiben.

},
methods: {
fetchRepetitivePlans: async function () {
let response = await api.get(`/loadpoints/${this.id}/plan/repetitive`);
Copy link
Member

Choose a reason for hiding this comment

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

Die Komponente sollte sich nicht selbst die Daten holen. Das Muster was wir standardmäßig verwenden ist, dass wir Schreiboperationen via REST machen die aktualisierten Daten aber über Websockets gepusht werden. Die landen dann im globalen State. Da dieses Feature nur für Fahrzeuge mit SoC verfügbar sein wird sollten wir den Ladeplan auch direkt am Fahrzeug pushen. Siehe hier:

Bildschirmfoto 2024-09-16 um 08 35 18

Damit ist er dann auch gleich über MQTT verfügbar und wir lösen das Problem Datensync zwischen Geräten. Also mehrere offene Browserfenster.

Aktuell veröffentlichen wir den "einmaligen Plan" auch noch nicht am Fahrzeug. Das müssen wir aber in diesem Zuge mit machen. Momentan kommen die aktuellen Plandaten die für die Statusanzeige in der UI und für das Ladeplanformular verwendet werden vom Ladepunkt. Mit der Einführung von mehreren Plänen würde ich am Ladepunkt nur noch die Daten des "als nächstes anstehenden Plans" veröffentlichen. Dass kann ein einmaliger sein, aber auch eine Ladung, die aufgrund einer Wiederholungsregel ansteht. Bislang mussten wir das nicht trennen. Jetzt ist das aber erforderlich.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beim Vehicle werden zwei Arrays mitgegeben: plans und repeatingPlans.
Beim Loadpoint werden die effectivePlanSoc und effectivePlanTime-Felder mitgeschickt.
Passt das so?

@@ -285,5 +296,35 @@ export default {
// TODO: handle fahrenheit
return this.fmtNumber(value, 1, "celsius");
},
getShortenedWeekdaysLabel: function (weekdays) {
Copy link
Member

Choose a reason for hiding this comment

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

Hier wären Unittests sicher gut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Maschga
Copy link
Contributor Author

Maschga commented Oct 14, 2024

Konkret würde das im Backend eigentlich "nur" bedeuten, dass überall da, wo aktuell der "statische Plan" verwendet wird eine Funktion aufgerufen werden muss.

Habt ihr für solche Funktionen eine extra Datei im Backend?

@Maschga
Copy link
Contributor Author

Maschga commented Oct 14, 2024

Ja die werden beide gepublisht. Aber wichtig ist ja vor allem, dass der Planner im Backend das Richtige mit den Plänen anstellt. Da muss die Logik ja ohnehin hin. Slots ausrechnen und Ladung starten macht ja nicht die UI.

Sorry, da habe ich deine Nachricht zu schnell gelesen und zu wenig darüber nachgedacht. 🙈

@Maschga
Copy link
Contributor Author

Maschga commented Oct 28, 2024

@naltatis Was hältst du davon bei der Vorschau den Titel immer bei "Plan Vorschau" zu lassen und den Übersetzungs-String "currentPlan" ("Aktiver Plan") zu entfernen?

@Maschga
Copy link
Contributor Author

Maschga commented Oct 28, 2024

Und es stellt sich die Frage, ob man in der Vorschau nur noch den nächsten aktiven Plan anzeigen sollte. Das würde im Frontend einiges vereinfachen und vielleicht auch dem Benutzer bei ganz vielen Plänen helfen, sich besser zurechtzufinden, welcher Plan als nächstes dran ist.

@Maschga
Copy link
Contributor Author

Maschga commented Oct 28, 2024

Man könnte auch bei den oberen Einstellungen (über der Vorschau) die Ränder der Dropdownmenus des nächsten Plans orange färben, damit der Benutzer besser den nächsten aktiven Plan erkennt.

// SetRepeatingPlans stores every repeating plan
SetRepeatingPlans([]api.RepeatingPlanStruct) error
// GetRepeatingPlansAsNormalPlans returns every repeating plan with a timestamp-key to sort
GetRepeatingPlansWithTimestamps(onlyActivePlans bool) []api.PlanStruct
Copy link
Member

Choose a reason for hiding this comment

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

Ich würde für diesen Daten-Zwischenstand keine neue öffentliche API am Fahrzeug machen. Das wird ja "nur" für die Berechnung des nächsten Zeitslots verwendet. Ich würd die Berechnung in der Methode verschieben, die rausfindet was die nächsten zu beachtenden Zeitpunkte sind. Also eher nen Aufruf, der in der Schleife bei vehiclePlanSoc() läuft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Du würdest also die Methode entfernen und den Code dieser Methode in die Methode vehiclePlanSoc verschieben?

@@ -43,6 +43,13 @@ type API interface {
// SetPlanSoc sets the charge plan time and soc
SetPlanSoc(time.Time, int) error

// GetRepeatingPlans returns every repeating plan
GetRepeatingPlans(onlyActivePlans bool) []api.RepeatingPlanStruct
Copy link
Member

Choose a reason for hiding this comment

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

Den onlyActivePlans Filter könnten wir auch rauslassen und erst in der Listenbearbeitung wo das für uns relevant ist prüfen. Ist aktuell ja nur die eine Stelle, wenn ich das richtig sehe.

core/vehicle/vehicle.go Outdated Show resolved Hide resolved
@naltatis
Copy link
Member

naltatis commented Nov 2, 2024

@Maschga bzgl. Vorschau und Kennzeichnung: Ja, aktuell wird das Diagramm immer angezeigt, auch wenn kein Ladeplan aktiv ist. Dann auf Basis der Werte des "Einmalplans". In diesem Fall steht da "Plan Vorschau". Ist der Plan aktiv ändert sich der Titel in "Aktiver Plan". Mit dieser Änderung müsste das mindestens "Nächster Aktiver Plan" heißen wenn es mehrere gibt.

@naltatis
Copy link
Member

naltatis commented Nov 2, 2024

Bzg. Kennzeichnung würde ich eher Richtung "Badge" Denken. Hinter dem Aktivierungshäkchen ist Platz den wir nutzen können. Hat den Vorteil, dass wir dann Text nutzen können. Das machts gerade für neue Nutzer einfacher zu verstehen als farbliche Umrandungen. Ggf. können wir auch Icon mit Tooltip nehmen. Ich hab hier mal ein schneller Mock.

Das ist noch nicht hübsch. Aber da kann ich mir noch mal GEdanken machen.

Bildschirmfoto 2024-11-02 um 16 06 41

@Maschga
Copy link
Contributor Author

Maschga commented Nov 2, 2024

Der Vorschlag mit dem Badge wird meiner Meinung nach platztechnisch schwierig, wenn man sich die Bilder hier anschaut.

@Maschga
Copy link
Contributor Author

Maschga commented Nov 2, 2024

Der Vorschlag mit dem Badge wird meiner Meinung nach platztechnisch schwierig, wenn man sich die Bilder hier anschaut.

Außerdem ist die Frage, ob man nicht zwei Badges braucht, einmal für den nächsten aktiven Plan und dann noch ein Badge für den nächsten Plan, der im Preview angezeigt wird (der kann ja aktiv oder auch inaktiv sein). Ansonsten besteht die Gefahr, dass der Benutzer meint, dass der Plan mit dem nächster-Badge im Preview angezeigt wird, obwohl das gar nicht stimmt.

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

Successfully merging this pull request may close these issues.

Planner: add schedule
4 participants