[C++] Code-Optimierung auf AVR

[N. G. (newgeneration)]

Hallo Forum,

ich habe hier diese "kleine" Interrupt-Routine.
Der Code läuft auf einem ATmega2560. Compiler ist der avr-g++ (also C++ 
code) mit -Os.

#define US_TIMER_REGISTER GPIOR0 //general purpose register
volatile uint16_t usVal[7];

ISR(PCINT1_vect) {
  static bool update[7];
  static uint16_t timer[7];
  for (uint8_t i = 0; i < 7; i++) {
    if (PINJ & (1 << i)) {
      if (update[i] == false)
        timer[i] = US_TIMER_REGISTER;
      update[i] = true;
    } else if (!(PINJ & (1 << i))) {
      if (update[i] == true)
        usVal[i] = US_TIMER_REGISTER - timer[i];
      update[i] = false;
    }
  }
}

leider wird dieser Code vom Compiler aufgebläht zu einem Code von fast 
200 Takten. Ich denke das das etwas zu lang für eine ISR ist.
Anbei findet ihr das Assembler-Resultat.

Falls Infos fehlen bitte fragen, ich habe garantiert etwas vergessen ;)

Nun zu meiner Frage:
Hat jemand Ideen für Optimierung(auf C++ und Assembler-Ebene)?

vielen Dank schon mal
N.G.

[(prx) A. K. (prx)]

N. G. schrieb:
>     if (PINJ & (1 << i)) {

Shifts mit variabler Anzahl sind höchst ungünstig. Eine sukzessiv 
verschobene Maske mitzuführen ist billiger.

[Peter II (Gast)]

ich versteht nicht was der code machen soll, aber selbst in C kann man 
noch einiges optimieren.

> if (PINJ & (1 << i)) {
das ist schon mal sehr langsam auf einen Atmel. Vermeide das schieben 
mit einer Variable.

> if (!(PINJ & (1 << i)))
wozu die 2.abfrage? das bit kann doch nur 0 oder 1 sein, damit reicht 
das else

ISR(PCINT1_vect) {
  static bool update[7];
  static uint16_t timer[7];

  uint8_t tmp = PINJ;
  for (uint8_t i = 0; i < 7; i++) {
    if (tmp & 1) {
      if (update[i] == false)
        timer[i] = US_TIMER_REGISTER;
      update[i] = true;
    } else {
      if (update[i] == true)
        usVal[i] = US_TIMER_REGISTER - timer[i];
      update[i] = false;
    }
    tmp = tmp >> 1;
  }
}

[N. G. (newgeneration)]

A. K. schrieb:
> N. G. schrieb:
>>     if (PINJ & (1 << i)) {
>
> Shifts mit variabler Anzahl sind höchst ungünstig. Eine sukzessiv
> verschobene Maske mitzuführen ist billiger.

Hallo A.K.
ja, stimmt, das wusste ich (eigentlich). Habe aber gehofft, dass der AVR 
mir das wegoptimiert. Naja, ist halt doch kien Wunderding der GCC ;)

Peter II schrieb:
>> if (!(PINJ & (1 << i)))
> wozu die 2.abfrage? das bit kann doch nur 0 oder 1 sein, damit reicht
> das else

Hallo Peter II,
ja, da hast du vollkommen Recht, ist sinnlos.
Danke für deinen Code, das spart schon etwa 40 Takte.
Aber leider immer noch zu wenig :(

noch jemand Ideen?

[(prx) A. K. (prx)]

N. G. schrieb:
> noch jemand Ideen?

Neuer Code+ASM?

[Peter II (Gast)]

N. G. schrieb:
> noch jemand Ideen?

ja. Das sieht mir doch stark nach ein soft-PWM aus.

Kannst du die Werte nicht schon vorher berechnen? Warum müssen sie 
ständig neu berechnet werden?

Damit musst du nur eine Zuweisung in der ISR machen.

Kannst du das Problem etwas genauer beschreiben, was der code macht?

[N. G. (newgeneration)]

A. K. schrieb:
> Neuer Code+ASM?

ASM wäre schön, müsste man aber können. Das Problem ist dass ich ams 
nicht wirklich kann, nur sehr rudimentär...

Aber ich glaube, dass ich darum nicht rum komme...

Was meinst du mit neuem Code?

[(prx) A. K. (prx)]

160 Takte für die ISR ergibt bei 8 Iterationen umgelegt 20 Takte pro 
Iteration. Das ist für einen AVR keine schlechter Wert.

[(prx) A. K. (prx)]

N. G. schrieb:
> Was meinst du mit neuem Code?

Gemeint war das aus Post#1, also die aktuelle Funktion und das neue .s 
File. Aber lass es, allein mit Code-Tuning wird da kein grosser Wurf 
mehr draus. Mal -O2 statt -Os probiert?

[N. G. (newgeneration)]

Peter II schrieb:
> ja. Das sieht mir doch stark nach ein soft-PWM aus.
>
> Kannst du die Werte nicht schon vorher berechnen? Warum müssen sie
> ständig neu berechnet werden?
>
> Damit musst du nur eine Zuweisung in der ISR machen.
>
> Kannst du das Problem etwas genauer beschreiben, was der code macht?

Nein, leider nicht.

Zum Code:
also es geht um 7 Sensoren des Typs SRF05. Diese werden mit einer 
Leitung ausgelesen.
Also so grob:
eine andere Timer ISR schickt zyklisch den Auslese-Befehl.
Die Sensoren hängen alle an dem selben Pin-Change-Interrupt.
also muss man folgendes tun:
wenn die ISR aufgerufen wird muss geprüft werden welcher Pin gewechselt 
hat und welchen Zustand er vorher hatte. Wenn er auf high gewechselt ist 
dann speichert man den Timerstand eines Timers der mit 58us läuft 
(dieses US_TIMER_REGISTER).
Wenn es aber auf low gewechselt ist, dann ist die Messung fertig und das 
Ergebnis muss nur in dem usVal-Array gespeichert werden.

Soweit der Code.

[N. G. (newgeneration)]

A. K. schrieb:
> Mal -O2 statt -Os probiert?

Nein, da der restliche Code auf -Os bleiben soll.
Kann mann auch dem avr-g++ anweisen, dass ein bestimmter Codeteil anders 
compiliert werden soll?
#prama GCC optimize("-O2") wird mit einem Fehler ignoriert

[(prx) A. K. (prx)]

Und weshalb darf das keine 10µs dauern?

[(prx) A. K. (prx)]

N. G. schrieb:
> #prama GCC optimize("-O2") wird mit einem Fehler ignoriert

Schon mal richtig geschrieben probiert?
Also vorne mit "g" und hinten ohne "-"?

[N. G. (newgeneration)]

A. K. schrieb:
> Und weshalb darf das keine 10µs dauern?

Berechtigte Frage.
Leider müssen "gleichzeitig" noch X andere Dinge erledigt werden: eine 
Soft-PWM für 4 Motoren (das Layout war falsch; es wurden nicht die Pins 
fürs Hardware-PWM verwendet), ein bisschen I2C hier, ein bisschen UART 
da, dann noch eine Software-Lösung für einen Parallel-Bus, SPI, ...
Es summiert sich auf.
Alles was geht wird interruptgesteuert erledigt, aber in der 
main-Schleife wird immer noch viel gerechnet.
Der AVR langweilt sich auf jeden Fall nicht ;)

[N. G. (newgeneration)]

A. K. schrieb:
> Schon mal richtig geschrieben probiert?
> Also vorne mit "g" und hinten ohne "-"?

Ok, peinlich.
Im Code stand "-O2"
Trotzdem kommt immernoch

SRF05.cpp:83: error: ignoring #pragma GCC optimize

Muss man da beim g++ etwas anders machen?

[(prx) A. K. (prx)]

N. G. schrieb:
> Soft-PWM für 4 Motoren

Und zwar mit Mikrosekundenauflösung, nehme ich an. ;-)

> Der AVR langweilt sich auf jeden Fall nicht ;)

Es gibt Dinge, für die ein AVR etwas zu knapp ist. Andererseits gibt es 
AVRs mit mehr Hardware-PWM als die 08/15 Typen bieten.

[(prx) A. K. (prx)]

N. G. schrieb:
> Muss man da beim g++ etwas anders machen?

Probier mal __attribute__((optimize("O2"))) bei der ISR. Kann allerdings 
sein, dass du dazu den Makro "ISR" zu Fuss auflösen und die Attributes 
kombinieren musst.

[Falk B. (falk)]

@ N. G. (newgeneration)

>wenn die ISR aufgerufen wird muss geprüft werden welcher Pin gewechselt
>hat und welchen Zustand er vorher hatte. Wenn er auf high gewechselt ist
>dann speichert man den Timerstand eines Timers der mit 58us läuft
>(dieses US_TIMER_REGISTER).

Das mit der Maske wurde ja schon gesagt.

https://www.mikrocontroller.net/articles/AVR-GCC-Codeoptimierung#Schiebeoperationen

Was aber auch recht gefährlich ist, sind deine if() else Konstruktionen 
mit nhur einer Anweisung ohne Klammern. Jaja, das ist gültiges C, aber 
man schießt sich dabei gern mal ins Knie. Man sollte IMO IMMER Klammern 
setzen.
Ausserdem sollte man nur EINMAL auf PINJ zugreifen. Nicht nur wegen der 
Geschwindigkeit, sondern auch wegen der Möglichkeit, dass sich während 
des Schleifendurchlaufs einzelne Pins ändern.

#define US_TIMER_REGISTER GPIOR0 //general purpose register
volatile uint16_t usVal[7];

ISR(PCINT1_vect) {
  static bool update[7];
  static uint16_t timer[7];
  uint8_t mask, i, j;
  
  j = PINJ;

  for (i = 0, mask=1; i < 7; i++, mask<<=1;) {
    if (j & mask) {
      if (update[i] == false) {
        timer[i] = US_TIMER_REGISTER;
      }
      update[i] = true;
    } else {
      if (update[i] == true) {
        usVal[i] = US_TIMER_REGISTER - timer[i];
      }
      update[i] = false;
    }
  }
}

[N. G. (newgeneration)]

A. K. schrieb:
> N. G. schrieb:
>> Soft-PWM für 4 Motoren
>
> Und zwar mit Mikrosekundenauflösung, nehme ich an. ;-)

Haha, nicht ganz. Aber 10kHz ist Maximum.

>> Der AVR langweilt sich auf jeden Fall nicht ;)
>
> Es gibt Dinge, für die ein AVR etwas zu knapp ist. Andererseits gibt es
> AVRs mit mehr Hardware-PWM als die 08/15 Typen bieten.

Hinterher ist man immer schlauer. Aber leider steht die Hardware schon.
Jetzt muss es halt irgendwie gehen...

[Falk B. (falk)]

@ N. G. (newgeneration)

>>> Soft-PWM für 4 Motoren

>Haha, nicht ganz. Aber 10kHz ist Maximum.

Dream on. Man kann sicher 10 KHz PWM-Takt (=Interruptfrequenz) 
erreichen, aber nie und nimmer 10 kHz PWM-Frequenz.

Siehe Soft-PWM.

>Hinterher ist man immer schlauer. Aber leider steht die Hardware schon.
>Jetzt muss es halt irgendwie gehen...

Ja und? Nimm einen möglichst pinkompatiblen AVR und zieh ein paar 
Drähte.

[N. G. (newgeneration)]

Falk Brunner schrieb:
> Was aber auch recht gefährlich ist, sind deine if() else Konstruktionen
> mit nhur einer Anweisung ohne Klammern. Jaja, das ist gültiges C, aber
> man schießt sich dabei gern mal ins Knie. Man sollte IMO IMMER Klammern
> setzen.
> Ausserdem sollte man nur EINMAL auf PINJ zugreifen. Nicht nur wegen der
> Geschwindigkeit, sondern auch wegen der Möglichkeit, dass sich während
> des Schleifendurchlaufs einzelne Pins ändern.

Hallo Falk,
okay, da hast du natürlich recht. Beim lesen erkennt man, gerade bei 
schlechter Einrückung, nicht immer direkt was wozu gehört. Werde 
dahingehen meinen Stil ändern.
Das mit dem Portzugriff ist auch wichtig, ich hatte das vollkommen 
übersehen, Danke.

[(prx) A. K. (prx)]

N. G. schrieb:
> Hinterher ist man immer schlauer. Aber leider steht die Hardware schon.
> Jetzt muss es halt irgendwie gehen...

Tja, jetzt gibts eben einen Wettbewerb. Einer von euch baut neue 
Hardware, der andere lernt Assembler. Wer schneller fertig ist gewinnt. 
Wird die Hardware sein, nehme ich an. ;-)

In gut überlegtem Assembler kann man jeweils ein paar Register für die 
ISRs reservieren, was sowohl hier wie auch bei der PWM deutlich Zeit 
einspart. Aber das geht praktisch nur bei 100% Assembler.

[Bastler (Gast)]

Mal PeDas Entprellcode (Entprellung) anschauen und abschauen, wie 
mal bis zu 8 Eingansleitungen eines Port gleichzeitig untersuchen kann. 
Deine Anforderung scheint zumindest nicht was völlig Anderes zu sein.

[N. G. (newgeneration)]

Falk Brunner schrieb:
> Ja und? Nimm einen möglichst pinkompatiblen AVR und zieh ein paar
> Drähte.

Gibt leider IMHO keinen. Der ATmega2560 und der ATmega128 sind soweit 
ich weiß der einzige 100pinner die sind Pinkompatibel sind. Aber 
gewonnen hat man dadurch nichts, nur weniger Speicher :(

Den Artikel hab ich schon mal Überflogen, muss ihn jetzt aber mal 
durcharbeiten

[(prx) A. K. (prx)]

Bastler schrieb:
> Deine Anforderung scheint zumindest nicht was völlig Anderes zu sein.

Doch, weil hier pinabhängiger Arrayzugriff vonnöten ist. Den kriegst du 
nicht parallelisiert.

[N. G. (newgeneration)]

A. K. schrieb:
> Wird die Hardware sein, nehme ich an. ;-)

Haha, leider ja, aber da fehlt leider das nötige Kleingeld, und auch die 
liebe Zeit. Aber beim Assembler auch :(

Bastler schrieb:
> Mal PeDas Entprellcode (Entprellung) anschauen und abschauen, wie
> mal bis zu 8 Eingansleitungen eines Port gleichzeitig untersuchen kann.
> Deine Anforderung scheint zumindest nicht was völlig Anderes zu sein.

Hallo Bastler,
ja,  das mach ich.
Danke

[Peter II (Gast)]

ich denke man kann das update Array noch wegoptimieren.

Es wird ja nur gebraucht um ein bit zu speichern. Das sollte sich aber 
alles über bitoperationen mit einem Byte zu schaffen sein.

[c]
ISR(PCINT1_vect) {
  static uint8_t changed;
  static uint16_t timer[7];

  uint8_t value = PINJ;
  changed = changed ^ value;

  uint8_t tmp = 1;
  for (uint8_t i = 0; i < 7; i++) {
    if ( changed & tmp ) {
       if (value & tmp ) {
          timer[i] = US_TIMER_REGISTER;
       } else {
          usVal[i] = US_TIMER_REGISTER - timer[i];
       }
    }
    tmp = tmp << 1;
  }

  changed = value;
}
[c]

so in der art.

[N. G. (newgeneration)]

Danke Peter,

werde ich auch noch testen.
Leider habe ich die Hardware aktuell nicht bei mir, muss also 
ausweichen...

[(prx) A. K. (prx)]

Lässt sich die Aufgabenstellung so umdefinieren, dass nur bei geändertem 
Pinzustand etwas für diesen Sensor getan werden muss? Also für die 
übrigens Pins nichts getan werden muss. Dann gäbe es eine mögliche 
Lösung, als Grundidee:

Per XOR gegenüber Vorzustand die Maske des geänderten Bits ermitteln und 
per 256 Byte Tabelle in eine Bitnummer umsetzen (ein "find first set 
bit" Befehl wär da hilfreich gewesen).

Eine while Schleife verbleibt zwar noch, aber die schlägt nur dann 
mehrfach zu, wenn mehrere Bits gleichzeitig kippten, was die Tabelle 
ebenfalls anzeigen wird.

[Peter II (Gast)]

N. G. schrieb:
> Danke Peter,
>
> werde ich auch noch testen.
> Leider habe ich die Hardware aktuell nicht bei mir, muss also
> ausweichen...

Spannend ist erst mal ob der ASM-code kürzer geworden ist.

[N. G. (newgeneration)]

A. K. schrieb:
> Lässt sich die Aufgabenstellung so umdefinieren, dass nur bei geändertem
> Pinzustand etwas für diesen Sensor getan werden muss?

Wenn ich dich richtig verstehe, ja.
Sind 7 unabhängige Signale. Und es kommt immer nur auf die 
Flankenwechsel an.

PS: ich hönge einfach nochmal das Datasheet an, der Modus mit nur einer 
Leitung(Modus 2) wird genutzt.

[(prx) A. K. (prx)]

Peter II schrieb:
> Spannend ist erst mal ob der ASM-code kürzer geworden ist.

Nö. Hier ist schneller gefragt, nicht kürzer. Deshalb ja die O2 
Optimierung mit unrolling. Die wird länger aber u.U. schneller.

[Bastler (Gast)]

Oder konkreter:

#define US_TIMER_REGISTER GPIOR0 //general purpose register
volatile uint16_t usVal[7];

ISR(PCINT1_vect) {
  static uint8_t prev;

  static uint16_t timer[7];
  uint8_t mask, i, j;
  
  i = PINJ;
  j = i^prev;  // j enthält die geänderten Bits
  prev = i;    // aktuellen Zustand merken

  // j    : geänderte Bits
  // prev : aktueller Zustand 

  // und nun (schnell auf'm iPad getippt und sicher nicht fehlerfrei!
  for (i = 0, mask=1; i < 7; i++, mask<<=1;) {
    if (j & mask) {
      if (!(prev & mask)) {
        timer[i] = US_TIMER_REGISTER;
      } else {
        usVal[i] = US_TIMER_REGISTER - timer[i];
      }
    }
  }
}

[N. G. (newgeneration)]

Peter II schrieb:
> N. G. schrieb:
>> Danke Peter,
>>
>> werde ich auch noch testen.
>> Leider habe ich die Hardware aktuell nicht bei mir, muss also
>> ausweichen...
>
> Spannend ist erst mal ob der ASM-code kürzer geworden ist.

gegenüber der Methode von Falk: 2 Instruktionen. Ich muss noch 
zusammenrechnen, ob es bei den Takten einen größeren Unterschied macht

[Bastler (Gast)]

Zu lange getippt (und gewischt ;-)

[(prx) A. K. (prx)]

N. G. schrieb:
> PS: ich hönge einfach nochmal das Datasheet an, der Modus mit nur einer
> Leitung(Modus 2) wird genutzt.

So tief gehe ich da nicht rein. Aber wenn im Regelfall nur ein Pin sich 
ändert, dann käme beispielsweise sowas raus:
   static uint8_t vorher;
   uint8_t zustand = PINJ;
   uint8_t maske = zustand ^ vorher;
   vorher = zustand;
   uint8_t nummer = maske2bit[maske];
   if (nummer < 7) {
      // Nur Pin "nummer" hat sich geändert
      ...
   } else {
      // mehrere Pins haben sich geändert, Schleife nötig
      ...
   }
was durchaus geeignet sein könnte, statistisch Zeit einzusparen.

[Peter II (Gast)]

Bastler schrieb:
> Zu lange getippt (und gewischt ;-)

aber immerhin die gleiche Idee nur etwas anders umgesetzt.

[N. G. (newgeneration)]

reele Laufzeit muss ich leider mit der Harware testen. Zusammengezählt 
ergeben sich etwa 130 Takte

Aber nochmal was anderes:

Wenn ich nur einen Codeteil mit O2 kompilieren will gibts immer einen 
Fehler:
#pragma GCC optimize("O2")
SRF05.cpp:124: error: ignoring #pragma GCC optimize

[(prx) A. K. (prx)]

N. G. schrieb:
> SRF05.cpp:124: error: ignoring #pragma GCC optimize

Beitrag "Re: [C++] Code-Optimierung auf AVR"

[N. G. (newgeneration)]

A. K. schrieb:
> N. G. schrieb:
>> SRF05.cpp:124: error: ignoring #pragma GCC optimize
>
> Beitrag "Re: [C++] Code-Optimierung auf AVR"

Peinlich, übersehen. Wird direkt ausprobiert.
Sorry

[Thomas H. (Firma: CIA) (apostel13)]

So seltsamer Code kommt heraus, wenn jemand der in C++ denkt versucht 
einen µC zu programmieren. Auf Multicore Systemen sind solche 
verkorksten Ansätze in der Regel egal. Auf einem µC denket man in 
Assembler und programmiert in C. Wenn man dann vernünftigen C Code hat 
und will diesen Weitergeben oder häufiger wiederverwenden kann man 
diesen in C++ Kapseln.

[Mark B. (markbrandis)]

Thomas Holmes schrieb:
> So seltsamer Code kommt heraus, wenn jemand der in C++ denkt versucht
> einen µC zu programmieren.

Wobei der eingangs gepostete Code mit C++ nicht wirklich was zu tun hat.

[Thomas H. (Firma: CIA) (apostel13)]

Mark Brandis schrieb:
> Wobei der eingangs gepostete Code mit C++ nicht wirklich was zu tun hat

Absolut richtig. Das war auch mein erster Gedanke. Deshalb schrieb ich 
bewusst "in C++ denken" Mal davon abgesehen ist C++  eh nur 
"eingepacktes" C.

[Falk B. (falk)]

@ Thomas Holmes (Firma: CIA) (apostel13)

>bewusst "in C++ denken" Mal davon abgesehen ist C++  eh nur
>"eingepacktes" C.

Jehova, Jehova!

Lass dass mal nicht die Leute hören, die wirklich wissen was C++ ist ;-)

[Schiko (Gast)]

eventuell noch die 16bit-rechnung rausziehen aus der ISR
zum kürzen der maximalen Takte innerhalb der ISR.
Ungprüft:

ISR(..)
{
  
  static  uint8_t prev =0;
  uint8_t pin    ;
  uint8_t i,mask,changed ; 

  Pin      = PINJ;
  changed  = pin ^ prev;     
  
  for(i=0 , mask=1 ; i<7 ; i++, mask<<=1 )
  {
    if( mask & changed )
      if( mask & pin ) //steigende Flanke
        tStart[i] = US_TIMER_REGISTER;
      else             //fallende Flanke
        tStop [i] = US_TIMER_REGISTER;
  }
  prev = pin;
}

...
sonstwo 
  dtVal[i] = tStop[i] - tStart[i]

ich gehe mal davon aus, ab und an Murks wegen fehlender
Synchronisation war Dir auch vorher nicht so wichtig..

Benötigst Du denn alle Sensorwerte gleichzeitig?
Wenn nicht, dann trigger sie nacheinander und Du kannst Dir die Schleife 
hier sparen.