stefankoelle/m5stack-dashboard
1
0
Fork 0
mirror of https://github.com/skoelle/m5stack-dashboard.git synced 2026-09-17 16:50:23 +00:00
Code Issues Packages Projects Releases Wiki Activity
Files
d0d4b29040911a777e084e10366adfa259c28f7b
m5stack-dashboard/CODE_REVIEW_PR1.md
T
stefankoelle 70d3bfba7c improvements
2026-08-02 08:22:54 +02:00

7.6 KiB
Raw Blame History

Code Review – PR 1: "Feature/improve UI"

Datum: 2026-08-02
Review-State: OPEN (nicht gemerged)
Commits im PR: 2 ("improve ui", "update build scripts")

1. ✅ Was gut ist

  • Neues include/date_utils.h: Saubere Extrahierung der Datumsformatierung in einen wiederverwendbaren Helper (DateUtils::formatShortDE). Sakamoto-Algorithmus für Wochentag-Berechnung ohne NTP ist korrekt implementiert. Eliminiert die vorherige Code-Duplizierung von formatEventTime in home_screen.cpp und calendar_detail_screen.cpp.
  • Gewitter-Icon: Neue drawThunderstorm / drawLightningBolt Funktion mit farbenfroher Akzentfarbe (ACCENT_STORM). Unterscheidet sich optisch klar von einfacher Bewölkung. Überload drawWeatherIcon(symbol, description, ...) mit Rückwärtskompatibilität via Default-Overload ist sauber gelöst.
  • Verbessertes Kalender-Icon: Ersetzt das vorherige checkbox-ähnliche Symbol durch ein echtes Kalenderblatt-Icon (Header-Balken, Heftklammern, Tagespunkt). Deutlicher UI-Qualitätsgewinn.
  • CI-Workflow-Fix: .github/workflows/build.yml generiert automatisch eine Wegwerf-secrets.h aus secrets.h.example für Builds. Bricht kein Build mehr in CI, weil secrets.h per .gitignore ausgeschlossen ist. Kommentar erklärt die Motivation gut.
  • UI-Feinabstimmung: Kalender-Detail und MVG-Screen erhalten mehr Zeilenabstand (rowH Konstante), Trennlinien und farbige Datumsangaben. Visuelle Hierarchie ist klarer.
  • drawCloud color-Parameter: Neuer Default-Parameter (Theme::TEXT_DIM) ermöglicht farbige Wolken für das Gewitter-Icon, ohne die bestehenden Aufrufer zu brechen.
  • .gitignore Cleanup: Eintrag für .venv-platformio/ neu organisiert, Kommentar hinzugefügt.
  • Manuelle Retries: Durch forceRefresh=true bei Button-Druck (via goToScreen) wird automatisch ein Retry ausgelöst – entspricht SPEC.

2. ⚠️ Was verbessert werden sollte

  • Kommentar-Entfernung zu radikal: Viele wertvolle Design- und Dokumentationskommentare wurden gelöscht (z.B. in http_client.h, weather_api.h, screen_base.h, home_screen.h). Diese Kommentare erklären warum der Code so funktioniert, nicht nur was er tut. Die SPEC.md bleibt zwar als Referenz, aber inline-Dokumentation auf Ebene der Header wäre wertvoll. Insbesondere die Erklärung des Retry-Mechanismus in http_client.h fehlt jetzt.

    • Dateien betroffen: src/api/http_client.h, src/api/weather_api.h, src/api/calendar_api.h, src/api/departures_api.h, src/screens/screen_base.h, src/screens/home_screen.h
    • Fix: Kurze Doc-Strings / Design-Kommentare wiederherstellen oder als /** ... */ Dokumentation ergänzen.

  • wifiConnected Flag nie verwendet (src/main.cpp:16): Das Flag wird in connectWifi() gesetzt, aber nirgends gelesen. Wenn WiFi ausfällt, gibt es keine Reconnect-Logik. In loop() wird der Verbindungsstatus nicht geprüft, bevor API- Calls gemacht werden – das wird durch den WiFi.status()-Check in httpGet abgefangen, aber ein dedizierter Reconnect-Versuch oder eine Fehlermeldung wäre robuster.

    • Fix: wifiConnected entweder nutzen (z.B. für UI-Statusanzeige) oder entfernen. Optional: Hintergrund-Reconnect in loop() implementieren.

  • Keine JSON-Key-Validierung: In weather_api.cpp, calendar_api.cpp, departures_api.cpp werden doc["events"], doc["forecast"], doc["departures"] direkt iteriert, ohne zu prüfen, ob der Schlüssel existiert oder ob doc erfolgreich deserialisiert wurde (nur auf err geprüft, aber nicht auf doc.containsKey(...)). Bei einer unerwarteten API-Antwort (z.B. Fehler-JSON statt Daten-JSON) würde dies zu einem leeren Array oder undefiniertem Verhalten führen.

    • Dateien betroffen: src/api/weather_api.cpp, src/api/calendar_api.cpp, src/api/departures_api.cpp
    • Fix: Vor Iteration if (!doc.containsKey("events")) { data.valid = false; return data; } prüfen.

  • Fehler-Anzeige-Duplizierung: Der Retry-Icon + "Keine Verbindung"-Block ist identisch in weather_detail_screen.cpp, calendar_detail_screen.cpp und mvg_screen.cpp (3× dupliziert).

    • Dateien betroffen: src/screens/weather_detail_screen.cpp:26-31, src/screens/calendar_detail_screen.cpp:39-44, src/screens/mvg_screen.cpp:39-44
    • Fix: Gemeinsame Hilfsfunktion drawErrorState(int x, int y, const char* label) in eine shared Header-Datei (z.B. src/screens/screen_utils.h) extrahieren. home_screen.cpp hat bereits eine lokale drawErrorState – diese sollte nach oben hinausgehoben werden.

  • Heißer Crash bei drawWeatherIcon mit leerem Symbol: drawWeatherIcon ruft symbol.charAt(0) und symbol.charAt(1) auf. Obwoohl isNight und condition mit Length-Checks abgesichert sind, wird das default-Case im Switch bei unbekannten Symbolen auf drawCloud geleitet – das ist eine implizite Annahme, die nicht dokumentiert ist.

    • Datei betroffen: src/icons/icons.h:53 (PR-Version)

  • mvg_screen.cpp Zeilenbegrenzung hardcoded: y > 212 Hardcoded-Wert ist nicht mit Theme::SCREEN_H verknüpft. Bei Anzeigegrößen-Wechsel müsste dies manuell angepasst werden.

    • Datei betroffen: src/screens/mvg_screen.cpp (PR-Version)

3. 🛑 Was blockierend ist (muss gefixt werden)

  • Kein Test-Framework vorhanden: Es gibt keinerlei Unit-Tests, keine Mock-Infrastruktur, keine Assertions. Insbesondere für date_utils.h (kritische Datumsberechnung) und text_utils.h (UTF-8-Transliteration) wären automatisierte Tests wertvoll. Ohne Tests ist nicht nachweisbar, dass z.B. der Sakamoto-Algorithmus korrekt funktioniert oder dass sanitizeGermanText alle Umlautfälle abdeckt.

    • Betroffene Dateien: include/date_utils.h (neu), include/text_utils.h
    • Blockade: Unit-Tests für DateUtils::formatShortDE und DateUtils::computeWeekdayMonBased hinzufügen (inkl. bekannte Test-Datumswerte). Test-Framework wählen (z.B. ArduinoUnity via PlatformIO, oder host-seitig mit Catch2 + Mock für String).

  • http_client.cpp – http.begin() Return-Wert ignoriert: http.begin(url) gibt bool zurück (true = success). Der Rückgabewert wird ignoriert. Bei einer ungültigen URL oder fehlschlagender Verbindungsinitialisierung würde http.GET() dann undefiniertes Verhalten zeigen.

    • Datei betroffen: src/api/http_client.cpp:16
    • Fix: if (!http.begin(url)) { result.success = false; http.end(); return result; } hinzufügen.

  • http_client.cpp – Stack-Größe bei DynamicJsonDocument: 8–16 KB DynamicJsonDocument auf dem Stack plus die gesamte HTTP-Antwort im String body → potenzieller Stack-Overflow bei großen API-Antworten. Der String wächst dynamisch im Heap, aber res.body nach http.getString() ist bereits vollständig im RAM geladen. Bei sehr großen Antworten (z.B. 10 Kalendertermine mit langen Description-Feldern) könnte der verfügbare RAM auf dem ESP32 knapp werden.

    • Datei betroffen: src/api/weather_api.cpp:15, src/api/calendar_api.cpp:15, src/api/departures_api.cpp:15
    • Nicht blockierend, da 8–16 KB üblich sind, aber serializeJson-Stream-Ansatz in einem Follow-up-PR erwägen.

Checkliste für Follow-up

  1. Unit-Tests schreiben für date_utils.h und text_utils.h
  2. http.begin() Return-Wert in http_client.cpp prüfen
  3. JSON-Key-Validierung in allen drei API-Clients hinzufügen
  4. Duplizierte Error-Anzeige in eine shared Utility-Funktion extrahieren
  5. Entfernte Design-Kommentare in Header-Dateien als Doc-Strings wiederherstellen
  6. wifiConnected Flag entweder nutzen oder entfernen
  7. Hardcoded y > 212 in mvg_screen.cpp durch Theme::SCREEN_H-Ableitung ersetzen
  8. Optional: Reconnection-Logik für WiFi-Ausfall in loop() implementieren
Reference in New Issue View Git Blame Copy Permalink