Reduzierung der #defines, stattdessen Einstellungen in Web-UI

Wir haben jetzt sehr viele Compiler-Parameter (#defines), die zur Kompilierzeit gesetzt werden müssen und später nur durch einen erneuten Upload geändert werden können. Das ist ziemlich umständlich, schwer für Einsteiger und verhindert den Weg zu einem einheitlichen Build.

Ich habe dazu mal einen Entwurf gemacht, einige #defines zur Laufzeit im Webinterface einstellbar zu machen. So könnte es aussehen:

Wenn gewünscht ist eine andere Gruppierung in der Weboberfläche die geringste Arbeit.

Diese Einstellungen habe ich bereits umgesetzt:

	PLAY_MONO_SPEAKER
	USE_LAST_VOLUME_AFTER_REBOOT
	PAUSE_ON_MIN_VOLUME
	VOLUMECURVE
	SAVE_PLAYPOS_BEFORE_SHUTDOWN
	SAVE_PLAYPOS_WHEN_RFID_CHANGE
	PLAY_LAST_RFID_AFTER_REBOOT	

Diese Einstellungen fehlen noch:

	PAUSE_WHEN_RFID_REMOVED
	DONT_ACCEPT_SAME_RFID_TWICE
	HEADPHONE_ADJUST_ENABLE

Eigentlich ist es ganz einfach: Einstellungen werden im NVS gespeichert und über den bestehenden REST-API Endpunkt /settings mit dem Webinterface konfigurierbar gemacht. Die #defines werden dann im Code durch die NVS Einstellung ersetzt. Man könnte die übrigen #defines nach und nach portieren (alle Einstellungen auf einmal - daran bin ich gescheitert).

Eine NVS Backup/Restore Funktion für die Einstellungen wäre auch schnell & schlank umgesetzt, wir haben das schon für die RFID Zuweisungen gemacht.

Ziel für mich wäre auch die Änderungen minimal zu halten um neue Bugs zu vermeiden. Hier könnt Ihr reinschnuppern: Make some player settings customizable at runtime · biologist79/ESPuino@a3a4273 · GitHub

In einem weiteren Schritt könnte man auch die Hardware-Einstellungen über die Weboberfläche machen, evt. schon in Accesspoint.html den RFID-Leser, Anzahl Neopixel usw festlegen.

Wenn das durch ist haben wir einen einheitlichen Build!

Was haltet ihr generell von diesem Weg? Habt Ihr Vorschläge?

18 „Gefällt mir“

Sehr cool @tueddy!
Kriegen wir da vielleicht noch hinten ein (?)-Feld dran, in dem man kurz beschreiben kann, um was es geht?

Super, für mich sind vorallem REVERSE_ROTARY und NUM_INDICATOR_LEDS relevant, wollte ich beides sowieso umbauen, aber dann warte ich damit bis du soweit fertig bist.

EDIT:
Aktuell hast du die default Optionen fest im Code drin, die würde ich noch in defines umbauen mit #ifndefs drummherum (Analog wie es bei der natural sort option ist):

Naja das geht schon, habe es mal testweise für eine Einstellung umgesetzt:

grafik

Aber ist das hilfreich? Meine Meinung: Die meisten Optionen sind mehr oder weniger selbsterklärend und der Tooltip wird da nur wenig weiterhelfen.

@biologist Hast Du spezielle Einstellungen im Kopf oder sollen alle ein Hilfe-Tooltip erhalten? Oder meinst Du einen einzigen Hilfeschalter neben der „Optionen“ Überschrift die dann auf eine Hilfeseite verlinkt?

1 „Gefällt mir“

Das ist halt scheinbar nur selbsterklärend, weil du da quasi drinsteckst :slight_smile:. Es muss etwas ausführlicher beschrieben werden, dann macht das aus meiner Sicht absolut Sinn.

Ja das wäre auch ok - dann kann man es ausführlich beschrieben. Da würde ich dann aber eine Wiki-Seite draus machen, so dass das auch andere Leute anpassen können. Das nmüsste dann aber auch dt + en sein. fr tue ich mir persönlich nicht an, das war für mich in der Schule damals schon immer ein Graus :rofl:

Edit: Ich habe mal eine Wiki-Seite angelegt. Da kann man Sektionen machen und die kann man direkt verlinken. So kann man das Webinterface dann zusammenhängend beschreiben.

4 „Gefällt mir“

Das ist der Weg! :slight_smile: um dieses fantastische Projekt zugänglicher zu gestalten! Freut mich sehr!

Würde aber auch Empfehlen die einzelnen Optionen detaillierter zu beschreiben.

Vielleicht so: Create inline help tips for your site with a bit of CSS - Tutorialzine

Oder wie weiter oben bereits vorgestellt!

Ich denke ein einziger Hilfe-Button mit Verlinkung auf die Dokumentation ist ein guter Weg. Oder einen Hilfe-Button pro Kategorie.

Weil da kann man die Einstellung ausfürhlicher beschreiben & evt. auf Detail-Beiträge weiter verlinken. In einen Tooltip passt immer nur eine kürzere Beschreibung rein, siehe Bild oben. Ich werde es noch ein wenig ausarbeiten & dann hier vorstellen

5 „Gefällt mir“

@tueddy, kann man dich bei dem Thema irgendwie unterstützen? Ich hab das Topic schon wieder voll vergessen, aber das ist eigentlich eine viel höhere Prio als unser Geplänkel auf den anderen Kanälen :yum:


Für die Hilfe-Icons könnten auch Popovers gut funktionieren. Dann wäre schon mal mehr Info da, aber man kann entscheiden, ob man noch tiefer schauen will.
Und damit nicht alle Texte von Anfang an detailliert sein müssen, kann man einfach nur den Wiki-Artikel verlinken und drauf bauen, dass in der Zukunft wer was in die Übersetzungen schreibt :wink:

Im Dunkelmodus sähe das etwa so aus:

Und in Hell und mit Titel – wobei der eigentlich redundant ist – dann z.B. so:
image
Die Abstände passen natürlich noch überhaupt nicht, ist nur mal um das Popover zu zeigen.

Der Code davon ist ein HTML Button mit data Attributen …

<button class="btn btn-small btn-outline-secondary btn-sm rounded-pill"
  data-bs-toggle="popover" 
  data-bs-content="Die Box merkt sich die letzte Einstellung der Lautstärke für das nächste mal Einschalten. Mehr Info <a href='https://forum.espuino.de/topic'>hier</a>">
 &nbsp?&nbsp;
</button>

… und dann zum Initialisieren aller Popovers einmal dieses Snippet ausgeführt

document.querySelectorAll('[data-bs-toggle="popover"]')
  .forEach(popover => {
    new bootstrap.Popover(popover, {html: true})
  })
4 „Gefällt mir“

Also ich fände es vor allem wichtig, dass die Firmware zuerst umgebaut wird, damit die Einstellungen zur Laufzeit möglich sind und gespeichert werden.
Daraus kann dann die REST API abgeleitet werden. → Swagger
Und um das UI kann man/sich dann jemand separat kümmern.

@trainbird Danke das Du das Thema wieder angestoßen hast, ich finde es auch mit am Wichtigsten auf dem Weg zu einem einheitlichen Build!

Leider kann ich es derzeit nicht durchziehen da ich privat auch viel um die Ohren habe. Der Weg für die ersten 6-7 Einstellungen hatte ich ja demonstriert (Kann man natürlich auch anders machen aber es funktioniert so gut mit minimalen Codeänderungen).

@CaCO3 Es gibt dazu auch keinen großen Umbau der Firmware sondern nur das Laden/Speichern von Einstellungen mittels NVS. Es läuft dann Alles über den bestehenden REST-API Endpunkt /settings. Hier könnte man die einzelnen Settings natürlich besser dokumentieren.

Also wer möchte kann es anpacken. Dann hier aber bitte ohne Seitenthemen & straight-on → geradeaus

P.S: Den Popover Vorschlag finde ich gut: Bei Mouse-Over einen kurzen Hinweis + Klick auf die Wiki-Seite wo die jeweilige Einstellung dann detailiert erklärt wird.

1 „Gefällt mir“

Kann ich das irgendwo forken? Ich finde den tatsächlichen Branch nicht und ich möcht die Autorenschaft von deinem bisherigen Code überschreiben…

Das kannst Du gern machen, aber Du hast es vermutlich anders herum gemeint? :sunglasses: Das ist mir persönlich nicht wichtig, Hauptsache es geht weiter egal von wem…

Der Branch ist vorhanden aber nicht mehr aktuell. Inzwischen gibt es ja eine modernisierte Web-UI. Ich könnte das auf den neuesten Stand bringen, aber ich bin momentan zeitlich leider sehr eingespannt.

Oh, wo ist das „nicht“ hin? :laughing:
Okay, dann werd ich das demnächst einfach übernehmen :+1:

1 „Gefällt mir“

Ich hab gestern mal begonnen, das Thema wieder anzugehen und den PLAY_MONO_AUDIO umgesetzt. Leider funktioniert das nicht, also es wird nicht mono. Hab mich dann stundenlang geplagt um rauszufinden, ob das Problem in master stattfindet (da funktioniert das flag in den settings und es wird mono ausgegeben) oder ob es auf dev mit dem flag statt der Änderung funktioniert - tut es nicht.
Ich hab leider nicht rausgefunden wie man ohne logging und neu kompilieren easy debuggen kann (bzw ist mir die Zeit davon gelaufen) drum konnte ich da nicht weiter investigieren.

Kann das jemand bestätigen, dass zwar ‚neuer abspielmodus mono‘ geloggt wird, das dann aber nicht wirklich mono wird? Und hat dann vielleicht noch jemand eine Ahnung warum das sein könnte? :thinking:

Dann konzentrier ich mich drauf die anderen Flags umzusetzen, da komm ich besser mit dem Code zurecht :innocent:

3 „Gefällt mir“

So!

Der Stand von @tueddy ist jetzt mal im Branch GitHub - Trainbird/ESPuino at defines-to-web-ui eingecheckt.

Gerne und eingehend testen! Wie bereits beschrieben hab ich PLAY_MONO nicht zum laufen gebracht. Außerdem bin ich mir bei der VOLUMECURVE nicht ganz sicher, wie hörbar bei der Effekt bei meinem Setup ist/wäre…

Vorschau ohne Kompilieren:

Es finden sich auch allerhand Optimierungen (hauptsächlich) in der Web-Oberfläche (unter anderem beim Sprachen laden und bei den Abständen zwischen den Elementen, aber ich hab auch das defaults-Objekt ein wenig aufgeräumt :broom: :innocent: ) über die ich hie und da gestolpert bin.

Den Teil hab ich jetzt vor lauter „endlich läuft es“ völlig vergessen … das mach ich morgen noch schnell :thinking:

Weitere Todos sind PLAY_LAST_RFID_AFTER_REBOOT, PAUSE_WHEN_RFID_REMOVED, DONT_ACCEPT_SAME_RFID_TWICE und HEADPHONE_ADJUST_ENABLE.
Edit: Woooooow, die werden tricky :flushed: die sind ja alle überall verwendet, inklusive weitere davon abhängige Flags in settings.h und settings-custom.h :see_no_evil: Ich befürchte fast, die sind mir zu steil …

Und @freddy möchte noch REVERSE_ROTARY und NUM_INDICATOR_LEDS – wenn er es nicht selber machen möchte und je nachdem wie weit ich komme :joy:

PS: Mobile muss ich mir noch ansehen, das bricht sich noch wild durch die Gegend … :expressionless:

In Bezug auf die defaults der ersetzten Settings wurde ja drauf hingewiesen, dass es schöner wäre, die Flags aus der settings.h zu nehmen. Da bin ich völlig d’accord!
Eine Frage des Geschmacks (ich bin eher Web-Entwickler und hab mit C++ wenig zu tun) ergibt sich für mich da aber grade: Sollte ich die

#ifndef SOME_FLAG
    #define SOME_FLAG false
#endif

Routinen lieber im jeweiligen Codeschnippsel einfügen, wo das jeweilige SOME_FLAG dann verwendet wird einfügen, oder eine einheitliche Definitionsstelle suchen. Damit das einheitlich und ein einem Aufwaschen passiert.

Konkret steh ich schon beim PLAY_MONO vor der Entscheidung, es sonst an vier Stellen einfügen zu müssen, siehe Commit - was ich nicht so schön fände.

Ich kann es schwer einschätzen, wo so ein allgemeiner #ifndef-Block gut hinpassen würde - sowohl vom Design, als auch von der Code-Execution her… settings.h fällt ja schon mal flach, das würd beim Checkout alle individuell getroffenen Einstellungen zerschießen.
Meine Vermutung wäre mal main.cpp oder main.h gewesen? Zweiteres wäre auch noch recht leer, da würd es nicht so viel verschandeln.

Thoughts on that?


Ich mach derweil mal weiter bei den anderen Flags :wink:

Okidoks!
Wir haben endlich (wieder) einen working Prototype :partying_face:

Ich hab die Commits quasi eingedampft und einen neuen Branch gemacht. Es gibt (entgegen der damaligen Version) kein Update vom Language-Loading, das war mir zu anstrengend und so ist es erst mal sauberer.

Darauf :point_up_2: hab ich allerdings immer noch keine Antworten, bin immer noch kein CPP-Entwickler :innocent:

PS: Sorry für die vielen Commits, ich bekomm es einfach nicht geschi**en mit clang und den customized settings :joy:
PPS: Ich hab inzwischen alles fünfmal Reformattiert, aber Bluetooth.cpp wird bei mir immer so umgebrochen, dass es durchfällt …

1 „Gefällt mir“

@trainbird Danke das du dran bleibst, das wird sicher ein wichtiges Feature für’s nächste Release!

Das Problem der zur Entwurfszeit festgelegten #Defines bleibt & ich hatte es auch nicht geschafft sie zur Laufzeit als Bool auszuwerten.
Mir ist da aber noch eine Idee gekommen:

Am Beispiel Mono/Stereo, aus

#define PLAY_MONO_SPEAKER

wird

const bool PLAY_MONO_SPEAKER = true // oder false

Der Code würde sich an vielen Stellen vereinfachen von z.B.

#ifdef PLAY_MONO_SPEAKER
  gPlayProperties.currentPlayMono = true;
#else
  gPlayProperties.currentPlayMono = false;
#endif

zu

gPlayProperties.currentPlayMono = gPrefsSettings.getBool("playMono", PLAY_MONO_SPEAKER);

Damit kann zur Compilezeit festgelegt werden ob Mono/Stereo und zur Laufzeit dann in der Weboberfläche umgeschaltet werden.

Einziger Nachteil hier ist ein „breaking-change“, die settings.h müssen neu eingestellt werden sofern zur Compilezeit vom Anwender gewünscht. Kann dann später eh über die Weboberfläche umgeschaltet werden. Ich halte es jetzt für vertretbar weil wir eh gerade größere Änderungen gemacht haben wie z.B. Nahendes Supportende für ESP32 ohne PSRAM

Alternativ könnte man die Compilezeit Vorgabewerte komplett entfernen, hart voreinstellen, z.B. auf Stereo

gPlayProperties.currentPlayMono = gPrefsSettings.getBool("playMono", false);

und den ESPuino dann über die Web-UI an den eigenen Geschmack anpassen.

Gibt es noch andere Vorschläge?@biologist Welche Lösung würdest du bevorzugen?

Das würde ich tatsächlich so vorschlagen. Also komplett rausnehmen die Defines, es statisch sinnig vorbelegen und als Benutzer kann man es dann konfigurieren via Web-UI.
Und das treibt man im besten Falle so weit, dass man auch den RFID-Reader wählen kann, so dass man ein universal build anstreben kann. Aber der Weg wird echt super lang: Man kann ja Tod und Teufel konfigurieren in ESPuino.

Oh, das ist natürlich eine viel elegantere Lösung, stimmt!
Sollte im settings.h dann die Zeile einfach kommentarlos raus fallen oder soll ich sie mit Verweis auf die Weboberfläche auskommentieren?

Okay, ich lese da raus, dass quasi „irgend wann mal“ das gesamte settings.h am Ende in die Weboberfläche migrieren sollte?
Nicht, dass wir da schon in greifbarer Nähe wären :smiley: