Automatische Code Formatierung

Und ich hab’s nie verstanden, warum man das nicht einrückt.

1 „Gefällt mir“

Ich kenne es auch so, dass Makros nicht wie der Programmfluss formatiert werden (mit einigen Ausnahmen, wie zB FOREACH Makros).

Es gibt uralte Compiler (von vor C89), die benötigen, dass bei einer Präprozessor-Direktive das ‚#‘ an Erster Stelle in einer Zeile steht (moderne Compiler verlangen nur, dass nur Leerzeichen davor stehen).

Ein anderer Grund ist, dass der Präprozessor vor dem Compiler durchläuft und eigentlich nicht Teil vom Programmfluss ist. Ich kenne diese Arten für die Formatierung:

a)

#ifdef WIN32
    #include <a.h>
#else
    #include <b.h>
#endif

void x() {
    #if DEBUG
    do_something();
    #endif
}

b)

#ifdef WIN32
#include <a.h>
#else
#include <b.h>
#endif

void x() {
#if DEBUG
    do_something();
#endif
}

c) (den mag ich am wenigsten, da die Einfärbung ab und zu fehl schlägt)

#ifdef WIN32
#   include <a.h>
#else
#   include <b.h>
#endif

void x() {
#   if DEBUG
    do_something();
#   endif
}

Ich denke auch, dass wir die meisten großen #if Blöcke umändern können, dass sie „normalen“ Code erzeugen (Buttons.cpp ist solch ein Kandidat). Damit würde das Problem mit der Formatierung auf jeden Fall lösen.

3 „Gefällt mir“

So, ich habe mich mal mit Button.cpp beschäftigt (der nach meinem Geschmack das meiste Copy & Paste und Präprozessor Direktiven hat). Ist relativ einfach zu vereinfachen:

Die Branch für alle Mutigen. Es lässt sich compilieren und läuft :laughing:: GitHub - laszloh/ESPuino at devel/code_cleanup

Größenvergleich zwischen den Varianten:

Alte Buttons:

    Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
    RAM:   [==        ]  21.6% (used 70736 bytes from 327680 bytes)
    Flash: [======    ]  58.9% (used 2276609 bytes from 3866624 bytes)

Neue Buttons:

    Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
    RAM:   [==        ]  21.6% (used 70720 bytes from 327680 bytes)
    Flash: [======    ]  58.9% (used 2276269 bytes from 3866624 bytes)
4 „Gefällt mir“

Das sehe ich genau so.
Ich würde mich freuen, wenn es eine Entscheidung für ein Tool geben würde, damit zumindest optische Anforderungen, wie ein Leerzeichen oder kein Leerzeichen vor oder nach einer Klammer, Einrückungen etc. einheitlich sind.

Aber gerade die Formatierung der Prä-Prozessor Anweisungen wie #if und dergleichen wirft eigentlich ein ganz neues Thema auf. Hier sollte es auch Richtlinien geben zu welchem Zweck diese angewendet bzw. nicht angewendet werden sollten. Mir ist klar, dass das, so wie es ist, gewachsen ist und immer den neuen Wünschen und Anforderungen entsprechend erweitert wurde mit dem Ziel, eine passende Firmware zu erstellen. Aber auch offene Wünsche, wie z.B. fertige Builds, fordern diesbezügliche Überlegungen.

Für eine Entscheidungshilfe, würde ich folgenden Ansatz anbieten:
Meine Betrachtungsweise bezieht sich auf die Anwendung die erstellt wird.
Diese soll möglichst auf unterschiedlichsten Hosts (Mikroprozessor Plattformen, Developer Boards, etc.) lauffähig sein. Wenn das gegeben ist, dann sollte diese dann ohne Neucompilierung maximal bezüglich Hardwarkomponenteneinsatz und Featurewahl konfigurierbar sein. Damit lässt sich ein Build breitbandig einsetzen.

Daraus ergeben sich schon viele Entscheidungshilfen zu PräProzessor ja/nein

Klares Ja zu Präprozessor:

  • Bedingungen, die Sourceteile nicht compilieren, da diese nicht auf allen Hosts lauffähig sind
    z.B: Plattform: ESP32 oder ESP32_XY (da bei 2. z.B. Register angesprochen werden, die beim 1. nicht vorhanden sind) (verwandt mit nächstem Punkt)
  • Einbindung von libraries die unterschiedliche Funktionalitäten zu unterschiedlichen Performances bieten: Hier per #if definieren was beim aktuellen Build Priortität hat: Mehr Features oder HighPerformance

Klares Nein zu Präprozessor:

  • Konfiguration von gewünschten Features
  • Ersetzen von Laufzeitprogrammablauf (z.B: Ermittlung des Wertes einer Konfigurationskonstanten etc.)

Großer Vorteil: Änderung von Featurwünschen ohne Neucompilierung der Firmware und dem damit verbundenen zwingenden Update.

Wann gibt es ein JAIN?
Prinzipiell sind für die maximale Flexibilität zur Laufzeit PräProzessor Directiven möglichst zu vermeiden.
Wir bewegen uns aber doch noch im Bereich nicht unendlicher Ressourcen (vorallem Speicher).
Sobald nicht mehr alle Features gleichzeit aktivierbar sind bzw. überhaupt speichermäßig nicht mehr unterbringbar sind, kommt man um eine Vorauswahl nicht mehr herum.

Irgendwie komme ich zum Schluss, solange wir es uns ressourcenmäßig leisten können, #if entfernen und durch „normale“ if Abfragen ersetzen.
Das bedingt dann eine config Klasse, die ihre Werte persistent (NVS) pflegt.

Damit verliert das Thema der PräProzessor Formatierung stark an Bedeutung.
Die verschachtelten #if Anweisungen sollten sich weitgehend in ganz einfache Anweisungen am Begin der Zeile auflösen.

Was meint ihr?

2 „Gefällt mir“

Ja, leisten können wahrscheinlich schon was die Ressourcen der Hardware angeht (Speicher). Und deine Aufteilung wo der Präprozessor gebraucht wird und wo nicht passt m. E. auch soweit.

Aber: Das bedeutet einiges an tiefgreifenden Änderungen und einiges an Programmieraufwand, weil auch Webinterface, etc. betroffen sind. Als langfristiges Ziel ist das sicher gut, aber ich denke wir sollten schrittweise vorgehen.

Selbst ohne dass man Settings vom Präprozessor in NVS zur Laufzeit verlagert kann man die #ifdef ... recht einfach loswerden. Zum Teil so wie oben von @laszloh am Beispiel der Buttons gezeigt oder es gibt da auch noch einen netten Makro-Trick, der bei anderen Projekten wie z. B. Linux, U-Boot oder Zephyr eingesetzt wird um ein definiertes oder undefiniertes Präprozessor-Symbol in ein Boolean-Literal (0 oder 1) zu konvertieren.

Damit kann man dann z. B. einfach if IS_ENABLED(PORT_EXPANDER_ENABLE) statt #ifdef PORT_EXPANDER_ENABLE schreiben. Der Compiler sieht dann nur noch entweder if (1) oder if (0) und optimiert entsprechend.

Wen interessiert wie das im Hintergrund genau funktioniert kann sich z. B. zephyr/include/zephyr/sys/util_internal.h at 76078ea63ce77fa32260d82e913115569731f849 · zephyrproject-rtos/zephyr · GitHub anschauen.

3 „Gefällt mir“

Ich stimme fschrempf zu. Deine vorgeschalgenen Änderungen würden eine tiefgreifende Änderung in dem Programmcode bedeuten. Plus muss die Konfiguration wahrscheinlich zu einem großen Teil im RAM gehalten werden. Das bringt einge Herausforderungen mit sich, das optimal für den RAM-Verbrauch auszuführen (und dabei so flexibel wie möglich zu bleiben).

Ein Beispiel wie wir es nicht lösen sollten (was aber gleichzeitig natürlich die größte Flexibilität mit sich bringt) könnt ihr in squeezelite-esp32 anschauen. Da wird die Konfiguration als JSON Objekt in den externen RAM geladen. Das bringt aber mit sich, dass wir zwingend einen WROVER haben müssen. Sonst reicht der RAM & Flash nicht aus.

Aktuell geht die SW auch sehr gut mit WROOM (also ESP32’s ohen externen RAM) und das will ich ehrlich auch behalten.Da haben wir aktuell um die 60kB Heap für die MP3 Dekodierung. Bluetooth Source funktioniert zB nicht, da der Heap dann auf 5k - 25k zusammenbricht (siehe Neustartschleife nach gescheitertem Anschluss eines Bluetoothkopfhörers). Damit ist es dann nicht mehr verwendbar, da für die MP3 Dekodierung leider gilt: „Mehr ist immer Besser und nur noch mehr ist noch besser“


Ich habe die Makros gesucht (und bin halb verzweifelt dran). Jetzt weiß ich wo ich abkupfern muss :laughing: ! Danke!!

Des weiteren kann mit c++17 constexpr if verwendet werden indem wir schreiben if constexpr(IS_ENABLED(XYZ)) (order wie in der LED PR if constexpr(NUM_LEDS ==1).

Das sind Ausdrücke die zur Compile-Zeit ausgewertet werden und entsprechend dem Ergenbis Pfade genommen werden. Der andere Pfad wird aus dem resultierenden Binary gelöscht.
Bei unserer Verwendung ist das equivalent zu dem #if, aber mit dem riesen Vorteil, dass man zu dem Zeitpunkt einen c++17 Compiler am „Steuerrad“ hat. D.h. er findet auch Tipp- und Syntaxfehler in dem „nicht genommenem“ Zweig und es könne auch komplexe Ausdrücke und std-Funktionen wie zb if constexpr (std::is_pointer_v<T>) verwendet werden.

EIn zusätzlicher Vorteil von constexpr if’s ist, wenn der Ausdruck nicht zur Compile-Zeit evaluiert werden kann (weil man zB den Inhalt einer nicht const variable abfragt), kriegt man das als Fehler vom Compiler um die Ohren geschmissen. Das heißt, man kann darauf vertrauen, dass dann auch nur 1 Pfad ohne eine if-Abfrage schlussendlich drin sein wird.

Hier ist eine sehr gute Diskussion drüber: https://www.reddit.com/r/cpp_questions/comments/x6didv/is_using_if_constexpr_equal_to_using_preprocessor/

3 „Gefällt mir“

Es ging mir darum, das Thema anzusprechen und insbesondere ggf. bei neuem Code in die gewünschte Richtung zu gehen.
Wie sieht es nun mit den vorgestellten Tools zur Codeformatierung aus?
Soll das kommen und wenn ja welches.
Clang und uncrustify würden mir gefallen.

Niko, nicht falsch verstehen, deine Idee ist gut. Bei einem Projekt mit mehr RAM & Flash würde ich auch definitiv in die Richtung gehen, dass so viel wie möglich Konfiguriert werden kann (am Besten in einfach zu ändernden JSON Files).
Das Problem ist halt hier wirklich das Audio Decoding (plus my :heart: dass das mit WROOM geht). Und ja, einiges mehr per Config modifizierbar zu machen ist sicherlich eine gute Idee und etwas was wir als Ziel haben sollten (ich schaue dich an cmdShort & cmdLong in Buttons.cpp :slight_smile:). Nur halt nicht alles, vor allem da aktuell teilweise einzelne Funktionen sich in die Quere kommen, bzw kein Sinn machen dynamisch geändert werden zu können ← zB Anzahl an LEDs in dem Ring oder die Anzahl an Tasten.

Aber ja, back to topic :smiley:


Ich plädiere für Clang. Einfach wegen seiner Verfügbarkeit (basically jeder der VSCode & Platformio verwendet hat es schon installiert). Damit halt wirklich keiner eine Ausrede, wieso er eine unformatierte PR eröffnen sollte und es gibt auch Github Actions, die eine PR automatisch gegen eine Config checken.

Wir müssen halt für die nächste Zeit noch mit dem „schlecht“ formatierten Präprozessor Direktiven leben, bis wir die Kompliziertesten aus dem Code bekommen.

4 „Gefällt mir“

Danke für die Erklärung. Außer dass if constexpr eben eine Prüfung mit sich bringt ob der Ausdruck wirklich konstant ist hat es aber keinen Vorteil gegenüber einem normalen if, oder?

Wenn ich if constexpr(NUM_LEDS == 1) verwende resultiert das doch binär in den selben Instruktionen, wie wenn ich if (NUM_LEDS == 1) schreibe, oder? Beides mal optimiert der Compiler so, dass nur ein Pfad und keine Verzweigung übrig bleibt.

Oder geht es im Wesentlichen darum den Compiler zu der Optimierung zu zwingen, während ohne constexpr nicht garantiert ist, dass die Verzweigung wegoptimiert wird (z. B. mit deaktivierter Optimierung -O0)?

Genau, if constexpr zwingt den Compiler zur Optimierung der Verzweigung (und den Programmierer dazu den Ausdruck auch konstant zu machen). Ansonsten hat der Compiler, wie du sagst, je nach -Ox die Freiheit Pfade drin zu lassen oder wegzuoptimieren.

Bei den NUM_LEDS == 1 wird das bei uns jedes Mal den gleichen Code erzeugen (da das am Ende nur ein Vergleich 12 == 1 ist).

1 „Gefällt mir“

Ich finde auch, clang ist eine gute Auswahl, da weit verbreitet.

Noch OT zur Runtime-config: ich meine es gab da mal den Traum ein Universal-image bereitzustellen. Dort müsste die hardware-abstraction - also so etwas wie Anzahl LEDs, pin-Belegung, etc. - komplett konfigurierbar sein. Vielleicht muss man sich dafür vom WROOM verabschieden, und Nutzer von WROOM müssen halt selbst compilen. Die Schwierigkeit dabei wäre wiederum ordentlich zwischen compile-time und runtime config zu abstrahieren.

Warum muss man sich dafür vom Wroom verabschieden? Man kann doch mit Pointern arbeiten und die Instanzierung macht man nur wenn notwendig.

Also es wäre definitiv unschön, wroom auszuklammern. Das wird auf jeden Fall von mehreren Dutzend Personen verwendet.

Mein Kommentar bezog sich darauf, dass der RAM für die MP3-Dekodierung nicht mehr reicht, wenn zusätzlich die Konfigurationsvariablen im Ram gehalten werden.
Ob das notwendigerweise so ist, sei dahingestellt.

Und ich bezog mich auch nur darauf den WROOM von der dynamischen Kondiguration auszuschließen. WROOMs müssten dann wie bisher konfiguriert werden. Aber das schweift schon wieder sehr ab.

Hab ich da etwas übersehen? Von vieviel Bytes an RAM Bedarf gehst du aus?

So nachdem ich hier ein wenig still herum geworden ist, habe ich mal eine Branch für Clang erstellt:

Hier könnt ihr anschauen, wie der Code nach der Formatierung aussehen würde.

Folgende inhaltliche Änderungen sind auch mit drin:

  • Clang format check bei push & PR
  • PIO build run bei push und PR

Der PIO build ist mit Caches und parallel (jedes Target wird einzeln gebaut). Damit brauchen wir im Schnitt 5 Minuten um alle zu bauen (CPU Zeit gleich, ca 20 Minuten → ~3 min / Target). Die Idee habe ich von Tasmota übernommen :slight_smile: .

Ergebnis schaut bei PRs dann so aus:

Kommentare und Vorschläge sind willkommen (ihr könnt auch gerne mit Test-PR’s gegen die Branche die PR-Checks ausprobieren). Wenn es so passt, mache ich eine PR für die Änderung auf.

2 „Gefällt mir“

Super! Danke für die Mühe! Das sieht sehr gut aus. Ich würde einen PR dafür auf jeden Fall befürworten.

Noch ein paar Dinge, die mir spontan aufgefallen sind:

name: Build all boards
on: 
  workflow_dispatch:
  pull_request:
    branches: dev
    paths:
      - '**.c'
      - '**.cpp'
      - '**.h'
      - '**.hpp'
      - '**.py'
      - '**.ini'
      - '.github/workflows/test-builds.yml'
  push:
    branches: dev
    paths:
      - '**.c'
      - '**.cpp'
      - '**.h'
      - '**.hpp'
      - '**.py'
      - '**.ini'
      - '.github/workflows/test-builds.yml'

Wenn ich das richtig sehe wird hier nur beim Push auf dev und bei PRs gegen dev getriggert. Wäre es nicht sinnvoll auf allen Branches zu triggern (wie bisher)? Dann läuft der Build auch z. B. bei einem Feature-Branch (ohne PR) und auf dem master.

Und bei den Trigger-Pfaden: Wäre es nicht sinnvoller statt allen Dateiendungen, die entsprechenden Verzeichnisse anzugeben, also src/*, html/*, etc.

    strategy:
      fail-fast: true

Das fail-fast bedeutet ja, dass beim ersten fehlgeschlagenen Job abgebrochen wird. Oft will man ja aber doch wissen, welche Jobs fehlschlagen und welche auch nicht. Daher hätte ich das eher deaktiviert.

1 „Gefällt mir“

Den Branch master sah zu testen sah ich als nicht wichtig an, da der Workflow ja sein sollte PR → dev → master. Damit sollte alles was in den Master einfließt schon sauber sein. Aber an Feature branches habe ich nicht gedacht. Ich schaue ob es ein Exclude branch oder so gibt. Sonst nehme ich es einfach raus.

Die Trigger-Pfade passe ich an, die hatte ich einfach von Tasomta copy&pasted ohne groß zu hinterfragen. Muss auch gestehen, ich bin kein Meister in den Github Actions :laughing:.

Hm, ja macht beiden Sinn. Ich hatte die action als go-nogo Test für die PR konzipiert, d.h. mit der Aussage, wenn eines ausfällt brauchen wir nicht weiter zu schauen. Auch unterscheiden sich die Targets bei uns fast nur im Pinout, somit ist es sehr wahrscheinlich dass nach dem Ersten auch die anderen ausfallen werden.

Für die Erleichterung der Fehlersuche macht es aber Sinn auch die anderen duchrlaufen zu lassen. Verbrauchst halt im schlechtesten Fall extra Rechenzeit (was wir als public Repo nicht bezahlen müssen, zum Glück :innocent: ).

2 „Gefällt mir“

Ja, das stimmt natürlich.

Ich auch nicht. Ich bin eher mit GitLab CI vertraut…

Ja, kann man auch so sehen und wäre für die Verifizierung auf dem dev oder im PR ausreichend. Wenn ich hingegen ein Feature gegen ein einzelnes Target entwickle ist es halt ganz nützlich die CI dann die anderen Targets testen zu lassen.

Macht sinn :+1: Habe fail-fast rausgenommen. Branch ist aktualisiert.

1 „Gefällt mir“

Ich habe eine PR für die Änderungen gestartet.

Achtung sobald dieser in der dev ist, wird das bei alle anderen PRs zu einiges an Arbeit führen. Aus dem Grund habe ich es vorerst als Draft eingestellt. Ihr könnt gerne mit dieser PR warten bis #221 und #236 gemergt sind, danach aktualisiere ich es auf die Dev Branch.

1 „Gefällt mir“