Forum: Compiler & IDEs AVR + 47HC164 Code-Optimierung


von Stefan N. (stefan_n)


Lesenswert?

Hi,

Ich benutze den ATtiny2313 zusammen mit 3 Schieberegistern (74HC164) um 
24 LEDs anzusteuern. Dabei soll das laden der Bits in das 
Schieberegister natuerlich so schnell wie moeglich vonstatten gehen, da 
man sonst auch die ausgeschalteten LEDs kurz aufblinken sieht.

Seht ihr noch optimierungsmoeglichkeiten?

leds.h
1
#ifndef LEDS_H
2
#define LEDS_H
3
4
#include <avr/io.h> 
5
6
#define LEDS_PORT PORTB
7
#define LEDS_CLK PB0  // clock pin for all three 74HC164 registers
8
#define LEDS_AB0 PB1  // data in for register 0
9
#define LEDS_AB1 PB2  // data in for register 1
10
#define LEDS_AB2 PB3  // data in for register 2
11
#define LEDS_MASK (_BV(LEDS_CLK) | \
12
                   _BV(LEDS_AB0) | \
13
                   _BV(LEDS_AB1) | \
14
                   _BV(LEDS_AB2))
15
                   
16
void set_leds(uint8_t row0, uint8_t row1, uint8_t row2);
17
                   
18
#endif

leds.c
1
#include <avr/io.h> 
2
3
#include "leds.h"
4
5
void set_leds(uint8_t row0, uint8_t row1, uint8_t row2) {
6
 
7
  for (uint8_t bit = 0x80; bit; bit>>=1) {  
8
9
    // clear, also sets clock to low
10
    LEDS_PORT &= ~LEDS_MASK;
11
    
12
    if (row0 & bit)
13
      LEDS_PORT |= _BV(LEDS_AB0);
14
  
15
    if (row1 & bit)
16
      LEDS_PORT |= _BV(LEDS_AB1); 
17
  
18
    if (row2 & bit)
19
      LEDS_PORT |= _BV(LEDS_AB2);   
20
21
    // clock to high
22
    LEDS_PORT |= _BV(LEDS_CLK);
23
  
24
  }
25
  
26
}

Gruss,
Stefan

von Peter D. (peda)


Lesenswert?

Warum nimmst Du denn keinen 74HC595 ?


Peter

von Stefan N. (stefan_n)


Lesenswert?

Hi,

> Warum nimmst Du denn keinen 74HC595 ?

Oh.. den kannte ich noch nicht. Beim naechsten mal nehme ich natuerlich 
den. Das mit dem kurzen Aufblinken ist auch nicht so schlimm, ich wollte 
nur wissen, ob man nicht vielleicht noch einbisschen optimieren 
koennte..

Gruss,
Stefan

von Falk B. (falk)


Lesenswert?

@  Stefan Noack (stefan_n)

>den. Das mit dem kurzen Aufblinken ist auch nicht so schlimm, ich wollte
>nur wissen, ob man nicht vielleicht noch einbisschen optimieren
>koennte..

Ja, siehe unten. Der direkte Zugriff auf den Port ist volatile, das ist 
so in den Include Files definiert. Mit einer Hilfsvariable gehts noch 
etwas schneller.

1
#include <avr/io.h> 
2
3
#include "leds.h"
4
5
void set_leds(uint8_t row0, uint8_t row1, uint8_t row2) {
6
  uint8_t tmp;
7
 
8
  for (uint8_t bit = 0x80; bit; bit>>=1) {  
9
    
10
    // clear, also sets clock to low
11
    LEDS_PORT &= ~LEDS_MASK;
12
    tmp=0;
13
14
    if (row0 & bit)
15
      tmp |= _BV(LEDS_AB0);
16
  
17
    if (row1 & bit)
18
      tmp |= _BV(LEDS_AB1); 
19
  
20
    if (row2 & bit)
21
      tmp |= _BV(LEDS_AB2);   
22
23
    // clock to high
24
    LEDS_PORT |= tmp | _BV(LEDS_CLK);
25
  
26
  }
27
28
}

von Max (Gast)


Lesenswert?

Wenn du auf assembler zurückgreifst wird es schneller, aber 
inkompatibler.

von (prx) A. K. (prx)


Lesenswert?

Falk Brunner schrieb:

> Ja, siehe unten. Der direkte Zugriff auf den Port ist volatile, das ist
> so in den Include Files definiert. Mit einer Hilfsvariable gehts noch
> etwas schneller.

Nicht ratsam, weil dadurch data setup time = 0, muss aber mehr sein. 
Wenn schon dann sowas wie
1
    unsigned char temp = PORTB & ~...;
2
    if (row0 & bit)
3
      temp |= _BV(...);
4
    if (row1 & bit)
5
      temp |= _BV(...);
6
    if (row2 & bit)
7
      temp |= _BV(...);
8
    PORTB = temp;
9
    PORTB |= _BV(...);

Bringt aber zumindest in 4.3.4 ziemlich wenig.

Was mehr bringen dürfte: -O3. Aber auch mehr Platz, weil vollständiges 
unrolling.

von Stefan N. (stefan_n)


Lesenswert?

Das mit der Hilfsvariable verstehe ich ehrlich gesagt nicht.

Laut der Doku von avr-libc wird aus
1
PORTB |= _BV(...)

einfach eine set-bit instruction:

>Access to the AVR single bit set and clear instructions are provided via
>the standard C bit manipulation commands. The sbi and cbi macros are no
>longer directly supported. sbi (sfr,bit) can be replaced by sfr |= BV(bit)
>
>i.e.: sbi(PORTB, PB1); is now PORTB |= _BV(PB1);
>
>This actually is more flexible than having sbi directly, as the optimizer
>will use a hardware sbi if appropriate, or a read/or/write operation if
>not appropriate. You do not need to keep track of which registers sbi/cbi
>will operate on.
>
>Likewise, cbi (sfr,bit) is now sfr &= ~(_BV(bit));

Mit der Zwischenvariable habe ich doch dann anstatt der SBI-instructions 
erstmal variablenzuweisungen und dann noch zusaetzlich einmal eine 
OUT-instruction, oder?

von (prx) A. K. (prx)


Lesenswert?

Stefan Noack schrieb:

> Laut der Doku von avr-libc wird aus
> ...
> einfach eine set-bit instruction:

Das schon, aber die braucht 2 Takte auf Port und nur 1 Takt auf 
Register. Ausserdem gilt das nicht wenn es mehrere Bits sind. Da die 
Maskierung folglich als load/and/store ausgeführt werden muss kann man 
das auch explizit codieren und das Tempregister weiterverwenden.

> erstmal variablenzuweisungen

Diese Variable sitzt allerdings in einem Register.

von Falk B. (falk)


Angehängte Dateien:

Lesenswert?

@  Stefan Noack (stefan_n)

>Laut der Doku von avr-libc wird aus

>PORTB |= _BV(...)

>einfach eine set-bit instruction:

Ach so stimmt. Ist mir irgendwie entfallen :-0
Gilt aber in erster Näherung für den AVR und andere Prozessoren, die das 
so schnell können.
Moment, SBI dauert aber 2 Takte, ein einfaches ORI nur 1. Der Vergleich 
ist der selbe. Also ist meine Version doch schneller ;-)
Hmm, nöö, doch nicht, am Ende ist es wieder gleich, siehe Anhang.

>Mit der Zwischenvariable habe ich doch dann anstatt der SBI-instructions
>erstmal variablenzuweisungen

Die wird aber in ein CPU-Register gelegt und ist damit maximal schnell.

> und dann noch zusaetzlich einmal eine OUT-instruction, oder?

Ja, aber die dauert nur 1 Takt. Aber vorhwr muss man laden und ODERn.

MfG
Falk

von Stefan N. (stefan_n)


Lesenswert?

>> Laut der Doku von avr-libc wird aus
>> ...
>> einfach eine set-bit instruction:
>
> Das schon, aber die braucht 2 Takte auf Port und nur 1 Takt auf
> Register.

Ah.. stimmt! (Datenblatt lesen hilft...)

> Ausserdem gilt das nicht wenn es mehrere Bits sind. Da die
> Maskierung folglich als load/and/store ausgeführt werden muss kann man
> das auch explizit codieren und das Tempregister weiterverwenden.

Stimmt.. Ich maskiere sowieso, d.h. ich habe sowieso einen 
load/and/store-zyklus, den kann ich natuerlich gleich aufweiten und 
zwischendrin direkt mit dem register arbeiten, was doppelt so schnell 
ist.

>
>> erstmal variablenzuweisungen
>
> Diese Variable sitzt allerdings in einem Register.

..und das ist schneller.. habe kapiert.

Es hat auch was gebracht. Mit der Variable ist das aufblitzen kaum noch 
zu sehen. Mit -O3 sieht man es dann nur im dunkeln und bei ganz scharfem 
hingucken.

Naja naechstes mal nehme ich gleich den 595er..

Danke!

von Stefan N. (stefan_n)


Lesenswert?

@ Falk: wie erzeugst du die .lss-Datei?

von (prx) A. K. (prx)


Lesenswert?

Originalversion 24-27 Takte (datenabhängig) pro Iteration, die 
modifizierte Version 23 Takte (gcc 4.3.4). Nicht wirklich der Brüller. 
Ergibt zusammengerechnet also an die 200 Takte.

Mit -O3 sind es jedoch für alle 8 Iterationen insgesamt nur knapp 90 
Takte.

von Falk B. (falk)


Lesenswert?

@  Stefan Noack (stefan_n)

>@ Falk: wie erzeugst du die .lss-Datei?

Einfach im AVR-Studio in den Projektoptionen den Haken setzen.

von Sauger (Gast)


Lesenswert?

Moin,

Falk Brunner schrieb:
> void set_leds(uint8_t row0, uint8_t row1, uint8_t row2) {
>   uint8_t tmp;

wenn man tmp durch eines der 3 GPIOR ersetzt, ist vielleicht noch was 
rauszukitzeln.

MfG

von (prx) A. K. (prx)


Lesenswert?

Du meinst, ein I/O-Port sei effizienter als ein Prozessorregister??

von Falk B. (falk)


Lesenswert?

@  Sauger (Gast)

>wenn man tmp durch eines der 3 GPIOR ersetzt, ist vielleicht noch was
>rauszukitzeln.

Nö, das Setzen der Bits in tmp als CPU-Register ist schon maximal 
schnell, ori dauert 1 Takt.
Aber SBI dauert nur 2 Takte. Die drei zusätzlichen Takte durch 3x sbi 
werden in der tmp-Variante am Ende wieder kompensiert, wo der Port 
gelesen, modifiziert und wieder geschrieben wird.
Wie gewonnen, so zerronnen. ;-)

Die tmp-Variante ist dann schneller, wenn man auf variable Bits per 
Bitmasken zugreifen muss, dann geht kein sbi/cbi.

http://www.mikrocontroller.net/articles/Bitmanipulation#Siehe_auch

MfG
Falk

von (prx) A. K. (prx)


Lesenswert?

Falk Brunner schrieb:

> Die tmp-Variante ist dann schneller, wenn man auf variable Bits per
> Bitmasken zugreifen muss, dann geht kein sbi/cbi.

Die I/O-Bits liegen fest, was fehlt ist die passende Operation zum Test 
ob gesetzt. Auch ein TEST Befehl, d.h. ein AND ohne Resultat, wäre 
hilfreich.

In der -O3 Variante ändert insbesondere diese Aspekt, denn bei 
vollständigem unrolling liegen die Bits fest und damit wird die 
Testoperation von MOV/AND/BREQ zu SBIC.

von jw (Gast)


Lesenswert?

Was spricht gegen USI, wenn's dann ganz schnell gehen muß?
Laut Atmel Doku "SPI Master Operation Example" (der zweite Teil mit 
"enrolled loop") schaft man damit F_CPU/2.
Gruß,
Jürgen

von (prx) A. K. (prx)


Lesenswert?

jw schrieb:

> Was spricht gegen USI, wenn's dann ganz schnell gehen muß?

Vermutlich der Umstand, dass er 3 davon bräuchte ;-).

von Sauger (Gast)


Lesenswert?

Moin,

A. K. schrieb:
> Du meinst, ein I/O-Port sei effizienter als ein Prozessorregister??

unter umständen schon. Betrachte die GPIOR's als globale Variablen die 
in einen Register liegen. Dieses dürfte für den Compiler/Optimizer ein 
gefundenes Fressen sein, wenn der Funktionsaufruf von set_leds(...) und 
was davor kommt, inline wird. Ist aber zugegebenermaßen stark vom 
Programmfluss abhängig.

MfG

von Stefan N. (stefan_n)


Lesenswert?

A. K. schrieb:
> jw schrieb:
>
>> Was spricht gegen USI, wenn's dann ganz schnell gehen muß?
>
> Vermutlich der Umstand, dass er 3 davon bräuchte ;-).

Und sobald es 4 werden wird es langsamer als die Variante mit der 
Variable.

von jw (Gast)


Lesenswert?

Naja, deshalb doch seriell, oder?
(164(Q7) an (in)164(Q7) an (in)164, paralleler Takt)

von Stefan N. (stefan_n)


Lesenswert?

Sauger schrieb:
> unter umständen schon. Betrachte die GPIOR's als globale Variablen die
> in einen Register liegen. Dieses dürfte für den Compiler/Optimizer ein
> gefundenes Fressen sein, wenn der Funktionsaufruf von set_leds(...) und
> was davor kommt, inline wird. Ist aber zugegebenermaßen stark vom
> Programmfluss abhängig.

Der aufruf an set_leds() ist vollkommen unkritisch (davor und danach 
wird eh 250ms gepennt). Es geht nur darum, die bits so schnell wie 
möglich in das Register reinzukrachen, damit man es nicht sieht, dass 
ich zu blöd war, ein register mit Output Latches zu verwenden.

von Stefan N. (stefan_n)


Lesenswert?

jw schrieb:
> Naja, deshalb doch seriell, oder?
> (164(Q7) an (in)164(Q7) an (in)164, paralleler Takt)

Nein, hat jeder einen eigenen pin (bleiben eh genug übrig). Clock ist 
gemeinsam.

Egal.. Ich löte jetzt Kondensatoren an die LEDs und dann hat sichs... 
;-)

von (prx) A. K. (prx)


Lesenswert?

Sauger schrieb:

> unter umständen schon. Betrachte die GPIOR's als globale Variablen die
> in einen Register liegen.

In einem I/O-Register, ja.

> Dieses dürfte für den Compiler/Optimizer ein
> gefundenes Fressen sein, wenn der Funktionsaufruf von set_leds(...) und
> was davor kommt, inline wird.

Inwieweit es sinnvoll sein könnte, ein Prozessorregister durch ein 
GPIORx zu ersetzen, das kann ich absolut nicht erkennen. Mit ist kein 
einziger Befehl bekannt, der mit GPIORx kürzer und/oder schneller ginge 
als mit einem Prozessorregister.

Die GPIORs sind genau dann sehr praktisch wenn man globale(!) Flagbits 
irgendwo unterbringen will, weil man dann die CBI/SBI/SBIC/SBIS-Befehle 
an Stelle LDS/.../STS verwenden kann. Gegenüber Byte-Variablen sind die 
Lade/Speicheroperationen immerhin ein bissel schneller. Beide Fälle sind 
hier nicht gegeben.

von Karl H. (kbuchegg)


Lesenswert?

Stefan Noack schrieb:
> jw schrieb:
>> Naja, deshalb doch seriell, oder?
>> (164(Q7) an (in)164(Q7) an (in)164, paralleler Takt)
>
> Nein, hat jeder einen eigenen pin (bleiben eh genug übrig). Clock ist
> gemeinsam.
>
> Egal.. Ich löte jetzt Kondensatoren an die LEDs und dann hat sichs...
> ;-)

Und das nächste mal Schieberegister mit getrenntem Ausgangsregister. 
Dann stellt sich das Problem erst gar nicht.

von Stefan N. (stefan_n)


Lesenswert?

Karl heinz Buchegger schrieb:
> Stefan Noack schrieb:
>> jw schrieb:
>>> Naja, deshalb doch seriell, oder?
>>> (164(Q7) an (in)164(Q7) an (in)164, paralleler Takt)
>>
>> Nein, hat jeder einen eigenen pin (bleiben eh genug übrig). Clock ist
>> gemeinsam.
>>
>> Egal.. Ich löte jetzt Kondensatoren an die LEDs und dann hat sichs...
>> ;-)
>
> Und das nächste mal Schieberegister mit getrenntem Ausgangsregister.
> Dann stellt sich das Problem erst gar nicht.

Ich habs ja verstanden...

Ich glaube nun ist alles gesagt, was hierzu gesagt werden kann.

von Sauger (Gast)


Lesenswert?

Mahlzeit,

A. K. schrieb:
> Die GPIORs sind genau dann sehr praktisch wenn man globale(!) Flagbits
> irgendwo unterbringen will, weil man dann die CBI/SBI/SBIC/SBIS-Befehle
> an Stelle LDS/.../STS verwenden kann. Gegenüber Byte-Variablen sind die
> Lade/Speicheroperationen immerhin ein bissel schneller. Beide Fälle sind
> hier nicht gegeben.

Wenn man die Flagbits zwecks Visualisierung auf Leuchtidoten legen 
möchte schon :-). Wird aber OT dem TE wurde geholfen, damit kann das 
Thema knitterfrei abgeschlossen werden.

MfG

von Peter D. (peda)


Lesenswert?

Wenns schnell schieben soll, bastele zuerst die 8 Portbytes zusammen und 
gebe dann aus:
1
void fast_out( uint8_t d0,
2
               uint8_t d1,
3
               uint8_t d2,
4
               uint8_t d3,
5
               uint8_t d4,
6
               uint8_t d5,
7
               uint8_t d6,
8
               uint8_t d7 )
9
{
10
    LEDS_PORT = d0;
11
    LEDS_PORT |= _BV(LEDS_CLK);
12
    LEDS_PORT = d1;
13
    LEDS_PORT |= _BV(LEDS_CLK);
14
    LEDS_PORT = d2;
15
    LEDS_PORT |= _BV(LEDS_CLK);
16
    LEDS_PORT = d3;
17
    LEDS_PORT |= _BV(LEDS_CLK);
18
    LEDS_PORT = d4;
19
    LEDS_PORT |= _BV(LEDS_CLK);
20
    LEDS_PORT = d5;
21
    LEDS_PORT |= _BV(LEDS_CLK);
22
    LEDS_PORT = d6;
23
    LEDS_PORT |= _BV(LEDS_CLK);
24
    LEDS_PORT = d7;
25
    LEDS_PORT |= _BV(LEDS_CLK);
26
}

Das Schieben dauert dann nur 24 Zyklen.


Peter

von Stefan N. (stefan_n)


Lesenswert?

Peter Dannegger schrieb:
> Wenns schnell schieben soll, bastele zuerst die 8 Portbytes zusammen und
> gebe dann aus
>
> Das Schieben dauert dann nur 24 Zyklen.
>

Passiert das bei -O3 nicht sowieso?

EDIT:

oh.. und wenn man die mit CLK geODERten auch noch vorher ausrechnet sind
es nur noch 16 takte :D Ich denke, ich werde das mal probieren...

2nd EDIT:

Könnte man die eingabewete auch als Array übergeben, ohne dass es 
langsamer wird?

von Peter D. (peda)


Lesenswert?

Stefan Noack schrieb:
> Passiert das bei -O3 nicht sowieso?

Der Compiler ist doch kein Hellseher, er weiß nicht auf was es Dir 
ankommt.

Ein Compiler wird immer so wenig wie möglich Variablen verwenden.
Das Aufblasen von 3 auf 8 Register-Variablen mußt Du erzwingen.

Es kann durchaus sein, daß Du fast_out() noch als noinline definieren 
mußt, damit es nicht wieder zurückoptimiert wird.


Peter

von Stefan N. (stefan_n)


Lesenswert?

verliere ich etwas, wenn ich ein array verwende?
1
void fast_out( uint8_t d[16]) __attribute__ ((noinline))
2
{
3
    LEDS_PORT = d[0];
4
    LEDS_PORT = d[1];
5
    LEDS_PORT = d[2];
6
    LEDS_PORT = d[3];
7
    LEDS_PORT = d[4];
8
    ...
9
}

da würde mir das zusammenpacken vorher einfacher fallen.

von Peter D. (peda)


Lesenswert?

Stefan Noack schrieb:
> verliere ich etwas, wenn ich ein array verwende?

Ich denke, ja.
Schau mal ins Listing.

Die 8 Variablen müssen in Registern vorliegen, sonst gewinnst Du nichts.


Peter

von Stefan N. (stefan_n)


Lesenswert?

So. Schneller geht es mit dem Schieben nicht (16 out-Anweisungen 
hintereinander):
1
static uint8_t get_bits(uint8_t d, uint8_t bit,
2
                 uint8_t row0, uint8_t row1, uint8_t row2)
3
{
4
  if (row0 & bit)
5
    d |= _BV(LEDS_AB0);
6
  
7
  if (row1 & bit)
8
    d |= _BV(LEDS_AB1); 
9
  
10
  if (row2 & bit)
11
    d |= _BV(LEDS_AB2);
12
      
13
  return d;
14
}
15
               
16
void set_leds(uint8_t row0, uint8_t row1, uint8_t row2) 
17
{ 
18
  uint8_t d = LEDS_PORT & ~LEDS_MASK;
19
  
20
  uint8_t d0 = get_bits(d, _BV(7), row0, row1, row2);
21
  uint8_t d1 = d0 | _BV(LEDS_CLK);
22
  uint8_t d2 = get_bits(d, _BV(6), row0, row1, row2);
23
  uint8_t d3 = d2 | _BV(LEDS_CLK);
24
  uint8_t d4 = get_bits(d, _BV(5), row0, row1, row2);
25
  uint8_t d5 = d4 | _BV(LEDS_CLK);
26
  uint8_t d6 = get_bits(d, _BV(4), row0, row1, row2);
27
  uint8_t d7 = d6 | _BV(LEDS_CLK);
28
  uint8_t d8 = get_bits(d, _BV(3), row0, row1, row2);
29
  uint8_t d9 = d8 | _BV(LEDS_CLK);
30
  uint8_t dA = get_bits(d, _BV(2), row0, row1, row2);
31
  uint8_t dB = dA | _BV(LEDS_CLK);
32
  uint8_t dC = get_bits(d, _BV(1), row0, row1, row2);
33
  uint8_t dD = dC | _BV(LEDS_CLK);
34
  uint8_t dE = get_bits(d, _BV(0), row0, row1, row2);
35
  uint8_t dF = dE | _BV(LEDS_CLK);
36
  
37
  asm("out %0, %1"  "\n\t"
38
      "out %0, %2"  "\n\t"
39
      "out %0, %3"  "\n\t"
40
      "out %0, %4"  "\n\t"
41
      "out %0, %5"  "\n\t"
42
      "out %0, %6"  "\n\t"
43
      "out %0, %7"  "\n\t"
44
      "out %0, %8"  "\n\t"
45
      "out %0, %9"  "\n\t"
46
      "out %0, %10" "\n\t"
47
      "out %0, %11" "\n\t"
48
      "out %0, %12" "\n\t"
49
      "out %0, %13" "\n\t"
50
      "out %0, %14" "\n\t"
51
      "out %0, %15" "\n\t"
52
      "out %0, %16" "\n\t"
53
      :
54
      : "I" (_SFR_IO_ADDR(LEDS_PORT)),
55
        "r" (d0), "r" (d1), "r" (d2), "r" (d3),
56
        "r" (d4), "r" (d5), "r" (d6), "r" (d7),       
57
        "r" (d8), "r" (d9), "r" (dA), "r" (dB),
58
        "r" (dC), "r" (dD), "r" (dE), "r" (dF)                
59
     );
60
}

Damit ist die Optimierung nun offiziell abgeschlossen ;-)

von Peter D. (peda)


Lesenswert?

Du darfst natürlich keine Wunder erwarten. Das Auge ist logarithmisch.
Wenn die Schiebezeit halbiert wird, hast Du nicht den Eindruck, daß das 
Aufblitzen der ausgeschalteten LEDs auch nur halb so hell ist.

Sinnvoll ist auch, alle 100Hz eine neue Ausgabe zu machen. Dann sieht 
man das Aufblitzen nicht mehr.


Peter

Bitte melde dich an um einen Beitrag zu schreiben. Anmeldung ist kostenlos und dauert nur eine Minute.
Bestehender Account
Schon ein Account bei Google/GoogleMail? Keine Anmeldung erforderlich!
Mit Google-Account einloggen
Noch kein Account? Hier anmelden.