Bitmanipulationen, Übungsaufgabe

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.

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

1

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

Mario M. schrieb:

1

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.

Mario M. schrieb:

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

oder avar komplett weglassen und dann so:

1

if (cnt & 0x66 == 0x66)

2

    PORTA.OUT |= 0x08;

3

else

4

    PORTA.OUT &= ~0x08;

Sischa dat.

Rolf M. schrieb:

if (cnt & 0x66 == 0x66)

Macht was anderes.

Mario M. schrieb:

Macht was anderes.

Ja, Klammern vergessen. Bitte dazudenken ;-)

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:

1

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

2

  avar &= 0x08;                                               // clear all bits except bit_3

3

  PORTA.OUT = (PORTA.OUT & 0xF7) | avar;                      // update bit_3

4

  cnt--;                                                      // proceed

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

1

2

   const unsigned char mask = 0x08;         

3

4

      cnt++;

5

      switch(cnt%32)

6

      {

7

      case 1:

8

      case 3:

9

      case 5:

10

      case 7:  PORTA.OUT |= mask;

11

               break;

12

      default: PORTA.OUT &= ~mask;

13

               break;

14

15

      }

Georg M. schrieb:

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

oder

1

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

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:

1

Bit     LED

2

3210

3

----    -

4

---1    1

5

--1-    -

6

--11    1

7

-1--    -

8

-1-1    1

9

-11-    -

10

-111    1

11

1---    -

12

1--1    -

13

1-1-    -

14

1-11    -

15

11--    -

16

11-1    -

17

111-    -

18

1111    -

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

1

    if(TCB0.INTFLAGS)

2

    {

3

      TCB0.INTFLAGS = TCB_CAPT_bm;                // clear TCB0 interrupt flag

4

      if (~cnt&8 && cnt&1)    PORTA.OUT |= 0x08;  // LED an wenn Bit3=0 und Bit0=1

5

      else                    PORTA.OUT &= ~0x08; // LED aus

6

      cnt++;

7

    }

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

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

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

1

1100000011000000

2

1100000011000000

3

0000000000000000

4

0000000000000000

5

0000000000000000

6

0000000000000000

7

0000000000000000

8

0000000000000000

9

1100000011000000

10

1100000011000000

11

0000000000000000

12

0000000000000000

13

0000000000000000

14

0000000000000000

15

0000000000000000

16

0000000000000000

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

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:

1

if (~cnt & 0x66)

2

    PORTA.OUT &= ~0x08;

3

else

4

    PORTA.OUT |= 0x08;

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... ;-)

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

1

for (int cnt = 0; cnt < 256; cnt++)

2

{

3

    if (cnt & 0x66) printf("-");

4

    else printf ("#");

5

}

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.

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)

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.

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.

Hallo,

diese Zeile jemanden Neuen zu erklären

1

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.

Georg M. schrieb:

Meine Frage ist nun, ob man diesen Code irgendwie verbessern...

1

> 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.

1

> 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äß

1

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.