Bitmanipulationen, Übungsaufgabe

[Georg M. (g_m)]

Hallo zusammen,
folgende Übungsaufgabe zum Thema "Bitmanipulationen".
Diese Übungsaufgabe hat keine praktische Bedeutung, aber man kann das 
Ergebnis sofort beobachten - an der blinkenden LED.
Die Aufgabe lautet:

blink-blink-blink-blink----pause----blink-blink-blink-blink----pause----....

#include <avr/io.h>

uint8_t cnt = 0xFF;          // SW counter
uint8_t avar;                // auxiliary variable

int main(void)
{
  PORTA.DIRSET = PIN3_bm;                               // PA3 output (LED)

  TCB0.CCMP = 0xFE4F;                                   // timeout value
  TCB0.CTRLA = TCB_CLKSEL_CLKDIV2_gc | TCB_ENABLE_bm;   // select clock and enable TCB

  while(1)
  {
    if(TCB0.INTFLAGS)
    {
      TCB0.INTFLAGS = TCB_CAPT_bm;                                // clear TCB0 interrupt flag
      avar = (cnt >> 2) & (cnt >> 3) & (cnt << 1) & (cnt << 2);   // combine
      avar &= 0x08;                                               // clear all bits except bit_3
      PORTA.OUT = (PORTA.OUT & 0xF7) | avar;                      // update bit_3
      cnt--;                                                      // proceed
    }
  }
}

Meine Frage ist nun, ob man diesen Code irgendwie 
verbessern/optimieren/vereinfachen bzw. komplett anders schreiben kann.

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Georg M. schrieb:
> Meine Frage ist nun, ob man diesen Code irgendwie
> verbessern/optimieren/vereinfachen bzw. komplett anders schreiben kann.
In welche Richtung soll er denn verbessert/optimiert/vereinfacht oder 
gar ganz umgeschrieben werden?

Dein Code ist derzeit insofern effizient, als er optimal auf genau diese 
Hardware zugeschnitten ist. Er ist aber völlig unverständlich. Denn auch 
wenn die Lösung relativ einfach ist, kapiert in akzeptabler Zeit kein 
Mensch, was da passiert, wenn du da nicht "Viermalblinker mit Pause auf 
Bit 3" dazuschreibst.

Besser verständlich wäre es, das Blinken auf dem LSB des Zählers zu 
machen und dieses Bit dann auf den LED-Ausgang zu schieben.

Insofern würde ich zumindest den Kommentar "combine" wesentlich 
ausführlicher gestalten.

[Mario M. (thelonging)]

Im Endeffekt prüft das Programm, ob die Bits 1, 2, 5 und 6 gleichzeitig 
gesetzt sind.

avar = ((cnt & 0x66) == 0x66) ? 0x08 : 0;

[Georg M. (g_m)]

Mario M. schrieb:
>

avar = ((cnt & 0x66) == 0x66) ? 0x08 : 0;

Wie? Es funktioniert auch ganz ohne bitweise Verschiebungen?

Dann ist die Aufgabe natürlich komplett falsch. Dann muss man sich etwas 
anderes ausdenken.

[Rolf M. (rmagnus)]

Mario M. schrieb:
> avar = ((cnt & 0x66) == 0x66) ? 0x08 : 0;

oder avar komplett weglassen und dann so:

if (cnt & 0x66 == 0x66)
    PORTA.OUT |= 0x08;
else
    PORTA.OUT &= ~0x08;

[Mario M. (thelonging)]

Sischa dat.

Rolf M. schrieb:
> if (cnt & 0x66 == 0x66)

Macht was anderes.

[Rolf M. (rmagnus)]

Mario M. schrieb:
> Macht was anderes.

Ja, Klammern vergessen. Bitte dazudenken ;-)

[Bruno V. (bruno_v)]

Georg M. schrieb:
> Meine Frage ist nun, ob man diesen Code irgendwie
> verbessern/optimieren/vereinfachen bzw. komplett anders schreiben kann.

Ich kann Lothar da nur zustimmen: Der Code ist Horror für jeden im 
beruflichen Umfeld.

Ich bin ganz sicher kein Freund von langen Variablennamen, ausführlichen 
Kommentaren oder unnützen Anweisungen. Aber am Ende sollte der Code 
erkennen lassen, was da passiert. Statt


Georg M. schrieb:

  avar = (cnt >> 2) & (cnt >> 3) & (cnt << 1) & (cnt << 2);   // combine
  avar &= 0x08;                                               // clear all bits except bit_3
  PORTA.OUT = (PORTA.OUT & 0xF7) | avar;                      // update bit_3
  cnt--;                                                      // proceed

wäre plain code das mittel der Wahl (hier mit cnt++ und 32 Takte 
insgesamt)

   
   const unsigned char mask = 0x08;         

      cnt++;
      switch(cnt%32)
      {
      case 1:
      case 3:
      case 5:
      case 7:  PORTA.OUT |= mask;
               break;
      default: PORTA.OUT &= ~mask;
               break;
      
      }

[Obelix X. (obelix)]

Georg M. schrieb:
> avar = ((cnt & 0x66) == 0x66) ? 0x08 : 0;

oder

avar = (!(cnt ^ 0x66)) ? 0x08 : 0;

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Georg M. schrieb:
> Dann muss man sich etwas anderes ausdenken.
Warum brauchst du eigentlich so viele Bits für diese Aufgabe?

Mein Ansatz wäre sowas mit 4 Bits:

Bit     LED
3210
----    -
---1    1
--1-    -
--11    1
-1--    -
-1-1    1
-11-    -
-111    1
1---    -
1--1    -
1-1-    -
1-11    -
11--    -
11-1    -
111-    -
1111    -

Und dann ist die LED an, wenn Bit 3 = 0 und Bit 0 = 1 ist:

    if(TCB0.INTFLAGS)
    {
      TCB0.INTFLAGS = TCB_CAPT_bm;                // clear TCB0 interrupt flag
      if (~cnt&8 && cnt&1)    PORTA.OUT |= 0x08;  // LED an wenn Bit3=0 und Bit0=1
      else                    PORTA.OUT &= ~0x08; // LED aus
      cnt++;
    }

https://onlinegdb.com/8EY-dN1ca

[Bruno V. (bruno_v)]

Nachtrag: Bei einer Taktanzahl, die nicht 2^n ist, z.B. 77, einfach ein 
"case 77: cnt = 0; break;" hinzu. Und % im switch weglassen

[Mario M. (thelonging)]

Ist ja alles schön und gut, aber eure Programme machen was anderes. So 
sieht es im Original aus:

1100000011000000
1100000011000000
0000000000000000
0000000000000000
0000000000000000
0000000000000000
0000000000000000
0000000000000000
1100000011000000
1100000011000000
0000000000000000
0000000000000000
0000000000000000
0000000000000000
0000000000000000
0000000000000000

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Mario M. schrieb:
> Ist ja alles schön und gut, aber eure Programme machen was anderes.
Meins macht das, was die Spec verlangt: vier mal blinken, dann eine 
Pause, dann von vorn...  ;-)


> So sieht es im Original aus
Es macht das, was die Spec verlangt: vier mal blinken, dann eine Pause, 
dann von vorn.

https://onlinegdb.com/_FIuA6gpPO

[Mario M. (thelonging)]

Lothar M. schrieb:
> Es macht das, was die Spec verlangt: vier mal blinken, dann eine Pause,
> dann von vorn.
Ein Auto ist alles, was vier Räder hat und sich von selbst bewegt. 😂
Noch kürzer (und noch kryptischer) wäre:

if (~cnt & 0x66)
    PORTA.OUT &= ~0x08;
else
    PORTA.OUT |= 0x08;

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Obelix X. schrieb:
> Georg M. schrieb:
>> avar = ((cnt & 0x66) == 0x66) ? 0x08 : 0;
> oder
> avar = (!(cnt ^ 0x66)) ? 0x08 : 0;
Hast du das ausprobiert oder nur vermutet?

Mario M. schrieb:
> Noch kürzer (und noch kryptischer) wäre
Auf jeden Fall weniger kryptisch als das Original...  ;-)

[Mario M. (thelonging)]

Man könnte auch noch "cnt" aufwärts zählen lassen, dann fiele das 
Negieren weg:

for (int cnt = 0; cnt < 256; cnt++)
{
    if (cnt & 0x66) printf("-");
    else printf ("#");
}

[Obelix X. (obelix)]

Lothar M. schrieb:
> Obelix X. schrieb:
>> Georg M. schrieb:
>>> avar = ((cnt & 0x66) == 0x66) ? 0x08 : 0;
>> oder
>> avar = (!(cnt ^ 0x66)) ? 0x08 : 0;
> Hast du das ausprobiert oder nur vermutet?

Sorry, ich hatte da was verwechselt.

[Peter D. (peda)]

Georg M. schrieb:
> Meine Frage ist nun, ob man diesen Code irgendwie
> verbessern/optimieren/vereinfachen bzw. komplett anders schreiben kann.

Der ist doch perfekt als Beispiel für Code Obfuscation, besser geht es 
nicht.

https://de.wikipedia.org/wiki/Obfuskation_(Software)

[Peter D. (peda)]

Georg M. schrieb:
> Wie? Es funktioniert auch ganz ohne bitweise Verschiebungen?

Schiebeoperationen nimmt man typisch nur für seriell parallel Wandlung.
Oder um Bitmasken an die gewünschte Stelle zu schieben.

Planloses und mehrfaches Schieben ist ungebräuchlich, da es eben die 
Lesbarkeit massiv erschwert.

Zählvariablen wertet man einfacher über Arrays oder Switch aus. Dann 
sind auch Änderungen leicht möglich, z.B. 5 Pulse.
Daher sind auch LUTs in FPGAs so beliebt.

[Gregor J. (Firma: Jasinski) (gregor_jasinski)]

Die richtigen Übungen machen den Meister, die falschen bewirken in der 
Regel leider nur, dass man sich immer weiter in der eigenen Suppe, die 
nie über den Tellerrand hinausragen wird, weil sie es gar nicht kann, 
dreht. Das gleiche gilt z.B. auch für Liegestützen, die man falsch – 
indem man z.B. nur den Popo auf und ab bewegt – macht, sofern man eine 
Veranschaulichung in diesem Kontext verwenden möchte. Alle anderen, die 
diese Übung so ähnlich ausführen, werden natürlich begeistert sein und 
sich ermuntern, ähnliche Übungen auszuführen – eine Gruppe der 
gegenseitigen Adoration, Begeisterung und des Klatschens entsteht.

[Veit D. (devil-elec)]

Hallo,

diese Zeile jemanden Neuen zu erklären

avar = (cnt >> 2) & (cnt >> 3) & (cnt << 1) & (cnt << 2);

wird der Knackpunkt sein. Auf den ersten Blick sieht das wie sinnfreies 
hin- und herschieben aus. Bevor du deine Leute damit von Beginn an 
überforderst, schiebe ein einziges Bit durch und zeige das mit Lauflicht 
am Port wie sich dadurch die Led bewegt. Dann noch invertiert. Kannste 
auch mit schöner formatierter serieller Ausgabe machen. Wenn das klar 
ist kannste tiefer einsteigen. Wäre so mein Ansatz.

[Johann L. (gjlayde)]

Georg M. schrieb:
> Meine Frage ist nun, ob man diesen Code irgendwie verbessern...

> uint8_t avar;                // auxiliary variable

Das sollte keinesfalls eine globale Variable sein (und auch keine im 
Static Storage).  Einfach eine lokale Variable in der entsprechenden 
Funktion machen / block machen (while Loop von main).

Gleeiches gile für cnt; diese Variable ist lokal in main.

> TCB0.CCMP = 0xFE4F;         // timeout value

Magische Werte erschweren es den Code nachzuvollziehen (warum nicht 
0xFE4E? etc.) und auch anzupassen, etwa auf eine andere Taktfrequenz 
oder Event-Rate.  Oft lassen sich AVR Timerwerte berechnen gemäß

SFR = (uint32_t) F_CPU / PRESCALE / IRQs - 1;

wobei:

F_CPU = Taktrate (des Quatzes, R/C-Oszillators) in Hz

PRESCALE = Eingestellter Prescaler-Wert (Kommentare schweigen dazu)

IRQs = Anzahl Ereignisse pro Sekunde

Es wäre wünschenswert, dass die Kommentare genau und nachvollziehbar 
erklärt, wie man auf exakt diesen Wert kommt (oder eben eine Formel wie 
oben, die übrigens bereits vom Compiler ausgewertet wird.

Aber dem steht die 2-spaltige Formatierung entgegen, die einen dazu 
drängt, knappe und schlampige Kommentare zu schreiben statt hilfreicher. 
Kommentare sollten einem Wert genug sein, ihnen eine eigene Zeile zu 
gönnen, statt sie dem Ziel unterzuordnen, möglichst viele Zeilen ins 
Fenster zu quetschen.

Auch die anderen Kommentare sind mangelhaft, egal ob es sich "nur" um 
eine Übungsaufgabe handelt, um ein Tutrial, oder um produktiven Code.