Kann man folgenden Code platzsparender schreiben

[MN (Gast)]

Hallo,

Bei einer Schaltung ist jetzt durch ein Update der Speicher des 
verwendeten Attiny2313 zu klein geworden. Bei der Suche nach Code der 
viel Speicher benötigt bin ich bei folgenden zwei Codeausschnitten 
fündig geworden.

#define DDS_CLK 125000000UL
...
uint64_t tuningword;
tuningword = (frequency * 4294967296) / DDS_CLK;
...

Hier wird das 32-bit Tuning-Word für einen 9850 DDS-Chip berechnet.

uint32_t GetFreqency(uint8_t values[])
{
  uint32_t retval;

  retval  = values[0];
  retval += values[1] * 10;
  retval += values[2] * 100;
  retval += values[3] * 1000;
  retval += values[4] * 10000;
  retval += values[5] * 100000;
  retval += values[6] * 1000000;
  retval += values[7] * 10000000;

  return retval;
}

Hier wird der Wert der an der 8-stelligen 7-Segment-Anzeige angezeigt 
wird berechnet.

Ich zerbrich mir jetzt schon länger den Kopf darüber aber ich finde 
keinen Weg diese Code-Ausschnitte effizienter zu schreiben.

[Peter II (Gast)]

MN schrieb:
> uint32_t GetFreqency(uint8_t values[])

wo wird denn die Funktion aufgerufen? Wo wird das Array gefüllt?

Kannst du den Kompletten Code zeigen, meist lässt sich da besser 
erkennen wo man etwas optimieren kann. Es macht wenig sinn eine Funktion 
zu verbessern wenn der Ansatz schon merkwürdig ist.

Hier kommt man da Array merkwürdig vor.

[Karl H. (kbuchegg)]

MN schrieb:

> Hier wird das 32-bit Tuning-Word für einen 9850 DDS-Chip berechnet.
>
Horner Regel

uint32_t GetFreqency(uint8_t values[])
{
  uint32_t retval = 0;
  uint8_t i;

  for( i = 0; i < 8; i++ )
    retval = retval * 10 + values[7-i];

  return retval;
}

Die Richtung der Schleife könnte man noch umdrehen, dann fällt der 
'unschöne' Ausdruck 7-i in der Inidzierung weg.

Aber ob das dein Platzproblem signifikant entlastet - da habe ich so 
meine Zweifel.
Auf der anderen Seite: wenn du nur ein paar Bytes mehr brauchst ...

[Clemens S. (zoggl)]

als pseudoschleife:
i=7
Ergebnis=0
{
Ergebnis=zahl[i]+Ergebnis*10
i=i-1

}solange bis i=0

und wenn möglich könnte man sogar den Input direkt in der schleife 
abholen.
das würde dann sogar das speichern der einzelstellen sparen.

i=7
Ergebnis=0
{
Ergebnis=lies_stelle_n_ein(i)+Ergebnis*10
i=i-1

}solange bis i=0
sg

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

Du übergibst der Routine die auf das 7Seg. ausgibt also einen 32-Bit 
Wert. Was macht die Funktion die das ganze auf dem 7Seg. ausgibt mit dem 
Wert...? Was ich damit sagen will Du wandelst einzelne Ziffern eines 
numerischen Wertes in einen solchen um. Zur Ausgabe auf einer 7Seg. 
Anzeige muss das ganze meist wieder Rückgängig gemacht werden. Da lässt 
sich vielleicht was optimieren. Wieviele Bytes musst Du denn sparen? 
Meist hilft es teile des Programs in Assembler zu programmieren, wenn es 
nicht um Speicher im KB Bereich geht.

[Jorge (Gast)]

Welcher Speicher ist zu knapp? RAM bei der Ausführung oder Flash für den 
Programmspeicher?

Hast du schon mal mit den Optimier-Optionen des Compilers gespielt? Ich 
vermute mal der ist eher in der Lage das zu reduzieren als irgendwelche 
Programmiertricks.
Wobei mir ehrlich gesagt nicht wirklich klar ist, weshalb gerade diese 
Funktion und das Array viel Platz wegnehmen sollten, da steht doch kaum 
etwas da? Womit hast du herausgefunden, dass gerade diese Code-Teile 
viel Platz benötigen?

[Jörg W. (dl8dtl) (Moderator)]

Jorge schrieb:
> Wobei mir ehrlich gesagt nicht wirklich klar ist, weshalb gerade diese
> Funktion und das Array viel Platz wegnehmen sollten, da steht doch kaum
> etwas da?

32-bit-Multiplikation und -Division.

Welche Compilerversion ist das?  Meiner Erinnerung nach hat sich da
in den aktuellen Versionen einiges getan.

Einen ATtiny2313 hätte ich allerdings für diesen 32-bit-Kram wohl
auch eher nicht benutzt.  Ggf. kannst du ja noch einen „Upgrade“ auf
einen ATtiny4313 in Erwägung ziehen, um paar Reserven zu bekommen.

[B. S. (bestucki)]

Jorge schrieb:
> da steht doch kaum etwas da?

Ich kenne den verwendeten Controller nicht, aber wenn dieser keinen 
Hardwaremultiplizierer besitzt und die ganzen verschiedenen 
Multiplikationen in Software durchgeführt werden müssen, kann ich mir 
schon vorstellen, dass der ROM knapp wird.

[Jörg W. (dl8dtl) (Moderator)]

be s. schrieb:
> wenn dieser keinen Hardwaremultiplizierer besitzt und die ganzen
> verschiedenen Multiplikationen in Software durchgeführt werden müssen

So ist es.

Ich würde für sowas (bzw. nehme) wenigstens einen ATmega, der hat
einen Hardware-Multiplizierer, was deutlich Code spart.

[Karl H. (kbuchegg)]

be s. schrieb:

> Ich kenne den verwendeten Controller nicht, aber wenn dieser keinen
> Hardwaremultiplizierer besitzt und die ganzen verschiedenen 32 Bit
> Multiplikationen in Software durchgeführt werden müssen, kann ich mir
> schon vorstellen, dass der ROM knapp wird.

Na ja. So wild ist das dann auch wieder nicht.
Die 32 * 32 Bit Multiplikation wird ja nicht direkt an dieser Stelle 
gemacht, sondern dafür wird eine Funktion aufgerufen.

D.h. in der ursprünglichen Fassung werden da 8 Funktionsaufrufe stehen 
sowie entsprechende Register Lade Anweisungen um die verschiedenen 
Konstanten auf den Weg zu bringen.

Das kann man natürlich etwas zusammenstreichen, aber um die 
Multiplikationsfunktion kommt man nicht herum. D.h. im wesentlichen kann 
man 7 Funktionsaufrufe samt Registervorbelegung einsparen. Ist natürlich 
eine Einsparung, aber die Welt wird es auch nicht sein.

[Peter II (Gast)]

be s. schrieb:
> Ich kenne den verwendeten Controller nicht, aber wenn dieser keinen
> Hardwaremultiplizierer besitzt und die ganzen verschiedenen
> Multiplikationen in Software durchgeführt werden müssen, kann ich mir
> schon vorstellen, dass der ROM knapp wird.

wobei man sagen muss, das die Multiplikationen für jeden Datentype nur 
einmal im ROM ist. Wenn er an einer andere stelle noch eine 
Multiplikationen verwendet, bringt es überhaupt nichts diese Funktion zu 
optimieren.

[Peter II (Gast)]

Jörg W. schrieb:
> Ich würde für sowas (bzw. nehme) wenigstens einen ATmega, der hat
> einen Hardware-Multiplizierer, was deutlich Code spart.

oder das Problem gleich anders Lösen. Kann mir nicht so richtig 
vorstellen für was man so ein Array braucht. Wenn dort wenigsten ASCII 
drin stehen würde, könnte ich es verstehen. Aber wo werden denn die 
Ziffern ein Zahl binär übertragen?

[Jörg W. (dl8dtl) (Moderator)]

Peter II schrieb:
> Kann mir nicht so richtig vorstellen für was man so ein Array braucht.

Ist ja nicht nur das Array, sondern eben auch die Berechnung des
tuning words für die DDS.

[Peter II (Gast)]

Jörg W. schrieb:
> Ist ja nicht nur das Array, sondern eben auch die Berechnung des
> tuning words für die DDS.

aber wieso braucht man dort das Dezimalsystem? (habe noch nie mit einem 
DDS gearbeitet)

[Flo (Gast)]

MN schrieb:
> #define DDS_CLK 125000000UL
> ...
> uint64_t tuningword;
> tuningword = (frequency * 4294967296) / DDS_CLK;
> ...

Wenn man den konstanten ausdruck 4294967296) / DDS_CLK so umrechnet, 
dass eine 2er Potenz im Nenner steht, könnte man sich den Platz für die 
Divisionsroutine im Flash sparen.

[Karl H. (kbuchegg)]

Peter II schrieb:
> Jörg W. schrieb:
>> Ist ja nicht nur das Array, sondern eben auch die Berechnung des
>> tuning words für die DDS.
>
> aber wieso braucht man dort das Dezimalsystem? (habe noch nie mit einem
> DDS gearbeitet)

Keine Ahnung.
Ich schätze mal das wird ein Überbleibsel aus der Benutzersteuerung 
sein. 8 Drehgeber - für jede Stelle eine. Oder irgend so etwas.

[Peter D. (peda)]

Man könnte auf alle 8 Ziffern '0' addieren, ne 0 ranhängen und damit 
atol() füttern.

[MN (Gast)]

Hallo vielen Dank schon mal an alle. Ich komme mit dem Lesen fast nicht 
mehr hinterher. Der Code passt jetzt gerade so in den Controller ist 
aber noch nicht ganz fertig.

Peter II schrieb:
> Kannst du den Kompletten Code zeigen, meist lässt sich da besser
> erkennen wo man etwas optimieren kann. Es macht wenig sinn eine Funktion
> zu verbessern wenn der Ansatz schon merkwürdig ist.

Ja kann ich selbstverständlich. In der Zip-Datei im Anhang sollten alle 
Code-Dateien enthalten sein. Der Code ist noch etwas wirr und 
unformatiert da ich gerade ständig irgendwo etwas ändere. Ich hoffe man 
kann ihn trotzdem einigermaßen entziffern.

Karl H. schrieb:
> Horner Regel

Vielen Dank. Das hat schon mal viel gebracht! Wenn ich jetzt den Code 
sehe könnte ich mir in den Hintern beißen. Darauf hätte ich auch selber 
kommen können.

Thomas H. schrieb:
> Du übergibst der Routine die auf das 7Seg. ausgibt also einen 32-Bit
> Wert. Was macht die Funktion die das ganze auf dem 7Seg. ausgibt mit dem
> Wert...? Was ich damit sagen will Du wandelst einzelne Ziffern eines
> numerischen Wertes in einen solchen um. Zur Ausgabe auf einer 7Seg.
> Anzeige muss das ganze meist wieder Rückgängig gemacht werden. Da lässt
> sich vielleicht was optimieren.

Nein, ich übergebe der Funktion die auf die 7Segment-Anzeige schreibt 
die Stellen schon einzeln. Ich verwende einen MAX7219 als 
Display-Treiber. Den 32bit-Wert brauche ich zur Berechnung des 
Tuning-Wortes für den AD9850.
Mit der Hilfe von Karl H. passt der Code jetzt gerade so in den Chip 
(text=1908). Ich muss allerdings noch eine Wertebereich-Abfrage 
einfügen.

Jorge schrieb:
> Hast du schon mal mit den Optimier-Optionen des Compilers gespielt? Ich
> vermute mal der ist eher in der Lage das zu reduzieren als irgendwelche
> Programmiertricks.

Optimierer-Option ist leider bereits auf -Os.

[Profi Frickler (Gast)]

MN schrieb:
> Hallo vielen Dank schon mal an alle.

Fällt mir gerade so nebenbei ein:

Der DDS AD9850 braucht die Bits "umgekehrt" hineingeschaufelt
gegenüber der Wertigkeitsfolge wie sie die üblichen AVR
SPI Maschinen erzeugen.

Dies, falls jemand darauf hereinfallen sollte ....
Man braucht einen Bit Swapper.

[Karl H. (kbuchegg)]

Schau mer mal

void Send(uint16_t data)
{
  uint16_t tmp;

  PORT_DISPLAY &= ~(1<<DISP_CS);

  for(int i=15; i>=0; i--)
  {
    tmp = data & (1<<i);

    PORT_DISPLAY &= ~(1<<DISP_CLK);

    if(tmp > 0)
      PORT_DISPLAY |= (1<<DISP_DATA);
    else
      PORT_DISPLAY &= ~(1<<DISP_DATA);

    PORT_DISPLAY |= (1<<DISP_CLK);
  }

  PORT_DISPLAY &= ~(1<<DISP_CLK);

  // CS pulse
  PORT_DISPLAY |= (1<<DISP_CS);
  PORT_DISPLAY &= ~(1<<DISP_CS);
}

die Variable temp wird dir der Compiler sowieso rauswerfen. In 
Wirklichkeit braucht die keiner. Du könntest die entsprechende 
Auswertung auch hier

    if(tmp > 0)

ganz einfach einsetzen. Worum gehts an dieser Stelle tatsächlich? Es 
geht darum, ob 1 Bit auf 1 oder auf 0 stand. Ob dieses temp größer als 0 
ist, interessiert eigentlich nicht. ungleich 0 reicht und dann kann man 
das so schreiben

    if( data & (1<<i) )
      PORT_DISPLAY |= (1<<DISP_DATA);
    else
      PORT_DISPLAY &= ~(1<<DISP_DATA);

aber eigentlich willst du auf einem AVR einen Ausdruck der Form '1<<i' 
nicht haben, denn den muss der Compiler in Form einer Schleife auflösen.

eine adequate Umsetzung, die ohne so einen variablen Shift auskommt, 
lautet

void Send(uint16_t data)
{
  uint16_t mask = 0x8000;

  PORT_DISPLAY &= ~(1<<DISP_CS);

  for(uint8_t i = 0; i < 16; i++)
  {
    PORT_DISPLAY &= ~(1<<DISP_CLK);

    if( data & mask )
      PORT_DISPLAY |= (1<<DISP_DATA);
    else
      PORT_DISPLAY &= ~(1<<DISP_DATA);

    PORT_DISPLAY |= (1<<DISP_CLK);

                mask >>= 1;
  }

  PORT_DISPLAY &= ~(1<<DISP_CLK);

  // CS pulse
  PORT_DISPLAY |= (1<<DISP_CS);
  PORT_DISPLAY &= ~(1<<DISP_CS);
}

Achte auch auf Datentypen! Du schmeisst da zb bei deinem i mit einem int 
um dich, obwohl du den Wertebereich überhaupt nicht brauchst. Das alles 
kostet. Nicht viel, aber ein paar Bytes sind es auch.
Man könnte auch darüber nachdenken, sich eine raustakt-Funktion für 
einen 8 Bit Wert zu schreiben und die dann 2 mal aufzurufen. Das würde 
die ganzen 16 Bit OPerationen runterbrechen. Müsste man ausprobieren, 
was dann tatsächlich kürzer ist.

(Das war jetzt nur die allererste Funktion, auf die ich nach öffnen des 
Zip Files im ersten C-File gestossen bin)

[Jörg W. (dl8dtl) (Moderator)]

Profi Frickler schrieb:

> Der DDS AD9850 braucht die Bits "umgekehrt" hineingeschaufelt
> gegenüber der Wertigkeitsfolge wie sie die üblichen AVR
> SPI Maschinen erzeugen.

So?

Ich habe das hier:

static void
fq_ud(uint32_t fq)
{
  union
  {
    uint32_t l;
    uint8_t b[5];
  }
  u;
  uint8_t i;

  u.l = fq;
  /*
   * W32..W39: no phase angle, no power-down, control bits = 0
   */
  u.b[4] = 0;

  for (i = 0; i < 5; i++)
    {
      SPDR = u.b[i];
      while ((SPSR & _BV(SPIF)) == 0)
        /* wait */;
    }

  /* Update now. */
  PORTC |= _BV(1);
  PORTC &= ~_BV(1);
}

… und das funktioniert.  Nachgemessen. ;-)

Hier ist die Initialisierungsroutine:

static void
init_ad9850(void)
{
  /*
   * Disable SPI, and reset the DDS circuitry.
   */
  SPCR = 0;
  PORTC |= _BV(0);
  PORTC &= ~_BV(0);

  /*
   * After reset, initiate one W_CLK pulse followed by one FQ_UD pulse
   * to activate the serial configuration control word.
   */
  PORTB |= _BV(7);
  PORTB &= ~_BV(7);
  PORTC |= _BV(1);
  PORTC &= ~_BV(1);

  /*
   * Now, we're ready to hand over the data transmission to SPI.
   */
  SPCR = _BV(SPE) | _BV(DORD) | _BV(MSTR);
  SPSR = _BV(SPI2X);
}

Kann es sein, dass dir das DORD-Bit entgangen ist?

[Profi Frickler (Gast)]

Profi Frickler schrieb:
> Man braucht einen Bit Swapper.

Nein braucht man doch nicht, man kann die SPI auch umkonfigurieren.

Ich war nur damals nicht willens meinen Treiber zu modifzieren.

[Profi Frickler (Gast)]

Profi Frickler schrieb:
> Ich war nur damals nicht willens meinen Treiber zu modifzieren.

Wenn man dann auch noch ein SPI LCD mit dem gleichen Treiber
ansteuert dann muss man auch noch dauernd die Wertigkeit umdrehen .....

[Karl H. (kbuchegg)]

ISR(TIM_COMP_vect)
{
  buttonDebounceISR();
}

das willst du nicht tun!

Du willst nicht aus einer ISR heraus eine Funktion aufrufen!
Damit zwingst du den Compiler zu einer Registersicherorgie. Die kostet 
Laufzeit (was noch verschmerzbar wäre) aber sie kostet natürlich auch 
Code!

Zieh die ISR runter zur Funktion und bette den Funktionsinhalt direkt in 
die ISR ein.

[Rainer B. (katastrophenheinz)]

Send ( ) ist mir auch aufgefallen.
Diese Version hier

void Send(uint16_t data)
{
  uint8_t i;

  PORT_DISPLAY &= ~(1<<DISP_CS);

  for(i=15; i>=0; i--)
  {
    PORT_DISPLAY &= ~(1<<DISP_CLK);

    if(data & 0x8000)
      PORT_DISPLAY |= (1<<DISP_DATA);
    else
      PORT_DISPLAY &= ~(1<<DISP_DATA);

    PORT_DISPLAY |= (1<<DISP_CLK);
    data <<= 1;
  }

  PORT_DISPLAY &= ~(1<<DISP_CLK);

  // CS pulse
  PORT_DISPLAY |= (1<<DISP_CS);
  PORT_DISPLAY &= ~(1<<DISP_CS);
}

spart 42 Code Bytes ggü deiner.

[B. S. (bestucki)]

Peter II schrieb:
> wobei man sagen muss, das die Multiplikationen für jeden Datentype nur
> einmal im ROM ist.

Korrekt. Ich sehe hier jedoch zwei Multiplikationsroutinen:

16 Bit signed:

values[1] * 10;
values[2] * 100;
values[3] * 1000;
values[4] * 10000;

32 Bit signed:

values[5] * 100000;
values[6] * 1000000;
values[7] * 10000000;

Insbesondere diese Zeile

values[4] * 10000;

kann aufgrund des Wertebereichs zum Problem werden. Diese Multiplikation 
müsste mit 32 Bit ausgeführt werden.

Nachtrag: Ich nehme an, dass im Array nur Werte von 0 bis 9 gespeichert 
sind, ansonsten müssten andere Multiplikationen auch als 32 Bit 
durchgeführt werden.

[Karl H. (kbuchegg)]

Hier

void ddsSetFrequency(uint32_t frequency)
{
  uint64_t tuningword;
  uint8_t msg[5];// = {0x2D, 0x3E, 0x05, 0x00, 0x00}; //ca. 10kHz
  uint8_t tmp;
  int i, j;

  tuningword = (frequency * 4294967296) / DDS_CLK;

  msg[0] = (uint8_t)(tuningword & 0xFF);
  msg[1] = (uint8_t)( (tuningword>>8 ) & 0xFF);
  msg[2] = (uint8_t)( (tuningword>>16 ) & 0xFF);
  msg[3] = (uint8_t)( (tuningword>>24 ) & 0xFF);
  msg[4] = 0x00;

  for(j=0; j<5; j++)
  {
    for(i=0; i<8; i++)
    {
      tmp = (msg[j] & (1<<i));

      PORTD &= ~(1<<PIN_WCLK);

      if (tmp > 0)
        PORTD |= ((tmp >> i) << PIN_DATA);
      else
        PORTD &= ~(1<<PIN_DATA);

      PORTD |= (1<<PIN_WCLK);
    }
  }
  PORTD &= ~(1<<PIN_WCLK);
  PORTD |= (1<<PIN_FQUP);
  PORTD &= ~(1<<PIN_FQUP);
}

dasselbe Spielchen noch einmal.

Ich denke, es würde auch etwas bringen, wenn man sich eine eigene 8 Bit 
Ausgabefunktion schreibt und die dann direkt mit dem jeweils 
zurechtgeschobenen Byte aufruft. Das Array braucht man in Wirklichkeit 
nämlich gar nicht

void dddsSetFrequencyByte( uint8_t byte )
{
  uint8_t i;

  for(i = 0; i < 8; i++)
  {
    PORTD &= ~(1<<PIN_WCLK);

    if( byte & 0x01 )
      PORTD |= 1<<PIN_DATA;
    else
      PORTD &= ~( 1<<PIN_DATA );

    PORTD |= (1<<PIN_WCLK);

    byte >>= 1;
  }
}

void ddsSetFrequency(uint32_t frequency)
{
  uint64_t tuningword;

  tuningword = (frequency * 4294967296) / DDS_CLK;

  dddsSetFrequencyByte( tuningword & 0xFF );
  dddsSetFrequencyByte( ( tuningword >> 8 ) & 0xFF );
  dddsSetFrequencyByte( ( tuningword >> 16 ) & 0xFF );
  dddsSetFrequencyByte( ( tuningword >> 24 ) & 0xFF );
  dddsSetFrequencyByte( 0x00 );

  PORTD &= ~(1<<PIN_WCLK);
  PORTD |= (1<<PIN_FQUP);
  PORTD &= ~(1<<PIN_FQUP);
}

aber der Löwenanteil wird wahrscheinlich tatsächlich für die 64Bit bzw. 
32Bit Arithmetik drauf gehen.

[MN (Gast)]

Der Code passt mittlerweile in den Controller und macht auch schon das, 
was er soll. Vielen Dank an dieser Stelle noch einmal für die Hilfe.

Karl H. schrieb:
> ISR(TIM_COMP_vect)
> {
>   buttonDebounceISR();
> }
>
> das willst du nicht tun!
>
> Du willst nicht aus einer ISR heraus eine Funktion aufrufen!
> Damit zwingst du den Compiler zu einer Registersicherorgie. Die kostet
> Laufzeit (was noch verschmerzbar wäre) aber sie kostet natürlich auch
> Code!
>
> Zieh die ISR runter zur Funktion und bette den Funktionsinhalt direkt in
> die ISR ein.

Okey, das sehe ich ein.

Dieses komische Konstrukt kommt daher, dass man in der Timer-ISR neben 
dem entprellen oft andere Aufgaben mit erledigen kann. Dann habe ich 
zwei Möglichkeiten:

1) die ISR() steht in der button.c und enthält Code, der eigentlich gar 
nicht zum Taster gehört. Die Datei button.c würde dann in jedem Projekt 
anders aussehen was ja nicht der Sinn der Sache ist.
2) die ISR() steht z.B. in main.c und der Code der jetzt in 
buttonDebounceISR() steht muss kopiert werden. Auch irgendwie eine 
unschöne Arbeitsweise.

Könnte man denn die angesprochene "Registersicherorgie" nicht vermeiden, 
wenn man die Funktion buttonDebounceISR() mit "inline" definieren würde?

[Karl H. (kbuchegg)]

MN schrieb:

> Könnte man denn die angesprochene "Registersicherorgie" nicht vermeiden,
> wenn man die Funktion buttonDebounceISR() mit "inline" definieren würde?

Im Zweifelsfall: ausprobieren und im Listing File nachsehen ob und was 
sich verändert hat. In deinem Fall könnte man auch sagen: Wird das 
Hex-File kleiner, dann hat es was gebracht - auch wenn die nackten 
Zahlen interpretiert werden wollen.

[MN (Gast)]

Karl H. schrieb:
> Im Zweifelsfall: ausprobieren und im Listing File nachsehen ob und was
> sich verändert hat. In deinem Fall könnte man auch sagen: Wird das
> Hex-File kleiner, dann hat es was gebracht - auch wenn die nackten
> Zahlen interpretiert werden wollen.

Das werde ich bei Gelegenheit machen. Heute bin ich schon mal sehr 
zufrieden, dass die neue Codeversion für den DDS läuft.

danke

PS: Ich bin mal wieder entsetzt auf wie viele Dinge ich beim 
programmieren zu wenig achte. Das kommt wohl daher das ich die µC 
ansonsten eher überdimensioniere.

[Uwe S. (de0508)]

Hallo,

hier ist noch ein anderer Ansatz für die Berechnung des FTW eines AD8950 
mit FCLK = 125MHz konstant !.

FTW = (34 +1/2 -1/7 +1/385 -1/528.613) * f ; f in Hz.

D.h. man benötigt 1 Multiplikation, 4 Divisionen, 2 Additionen und 2 
Subtraktionen

[Frickelfritze (Gast)]

Uwe S. schrieb:
> 4 Divisionen, 2 Additionen und 2 Subtraktionen

um Konstanten auszurechnen? Das finde ich eine saudumme Idee.

[Uwe S. (de0508)]

Frickelfritze,

dann bist Du kein Mathematiker und hast den Vorteil dieser 
Näherungsformel, für FTW = f * 2^32 /125MHz, nicht verstanden.
Mann muss sich nur mal die verschiedenen Wertebereiche ansehen..

[Frickelfritze (Gast)]

Uwe S. schrieb:
> dann bist Du kein Mathematiker

Für soviel reicht es dass ich sagen kann dass der Ausdruck
in der Klammer

Uwe S. schrieb:
> (34 +1/2 -1/7 +1/385 -1/528.613)

konstant ist und damit nur einmal berechnet werden muss.
Du würdest es aber jedesmal neu berechnen wenn du nicht Glück
hast und der Compiler das sowieso wegoptimiert.

In deiner Formel kommt noch "erschwerend " hinzu dass eine
Floating Point Zahl vorkommt die den Compiler zwingt
in FP zu rechnen. Sehr ungünstig ..... für die Erzeugung
von minimalem Code in einem AVR .....

[Uwe S. (de0508)]

Hallo,

Um zu erfahren mit wie vielen Bits man rechnen muss, macht man eine 
Abschätzung.

Es gilt mit FCLK = 125MHz:
a) k = 2^32 /FCLK ~ 34,360 < 35
b) 0 <= f <= FCLK/3 ~ 41,667MHz

Anm.: Mit FCLK/3 hat man noch 3 Stützstellen bei der Sinusgenerierung.

Wir wählen für unsere Rechnung als obere Grenzfrequenz f_max = 40MHz.

Es folgt:
FTW = f *2^32 /FCLK = f *(k -Fehlerterm) = f *k - f *Fehlerterm <= f *k

mit k = 35 aus a) und f_max = 40MHz können wir die maximale Anzahl Bits 
für unsere Rechnung herleiten:

max. Bit = log2(40 *10⁶ *35) = log2(1.4 *10³ *10⁶)
max. Bit = log2(1.4 *10⁹) = 30,38 Bit < 31 Bit

Also können wir bei alle Rechnungen mit ganzen 32 Bit Zahlen (0<=N<2³²) 
durchführen ohne einen Überlauf erwarten zu müssen.

[Uwe S. (de0508)]

Frickelfritze schrieb:

Sorry, für das Missverständnis:

528.613 = 528613 und es wurde ein Tausendertrennzeichen eingefügt.

> Uwe S. schrieb:
>> dann bist Du kein Mathematiker
>
> Für soviel reicht es dass ich sagen kann dass der Ausdruck
> in der Klammer
>
> Uwe S. schrieb:
>> (34 +1/2 -1/7 +1/385 -1/528.613)
>
> konstant ist und damit nur einmal berechnet werden muss.
> Du würdest es aber jedesmal neu berechnen wenn du nicht Glück
> hast und der Compiler das sowieso wegoptimiert.
>
> In deiner Formel kommt noch "erschwerend " hinzu dass eine
> Floating Point Zahl vorkommt die den Compiler zwingt
> in FP zu rechnen. Sehr ungünstig ..... für die Erzeugung
> von minimalem Code in einem AVR .....

[chris (Gast)]

Uwe S. schrieb:
> FTW = (34 +1/2 -1/7 +1/385 -1/528.613) * f ; f in Hz.

1/2 = 0
1/7 = 0
1/385 = 0

integer Division...

[Frickelfritze (Gast)]

Uwe S. schrieb:
> Also können wir bei alle Rechnungen mit ganzen 32 Bit Zahlen (0<=N<2³²)
> durchführen ohne einen Überlauf erwarten zu müssen.

In deiner Formel hast du das aber nicht verinnerlicht, das
mit den ganzen Zahlen ... und der Compiler weiss auch nichts davon.

[Uwe S. (de0508)]

Hallo Chris,

ich sehe schon, diese Art von Berechnung ist nicht vielen geläufig.
Natürlich löst man die Klammer auf.

Es gilt ja weiterhin laut Datenblatt des AD9850:

FTW = f * k mit k = 2³²/125MHz

und k ~ 34,3597 ~ (34 +1/2 -1/7 +1/385 -1/528613)

FTW ~ 34*f +f/2 -f/7 +f/385 -f/528613

chris schrieb:
> Uwe S. schrieb:
>> FTW = (34 +1/2 -1/7 +1/385 -1/528.613) * f ; f in Hz.
>
> 1/2 = 0
> 1/7 = 0
> 1/385 = 0
>
> integer Division...

Ich kann auch behaupten und beweisen, schon für die DDS AD9912, AD9851, 
AD9851/AD9850 und AD9834 Funktionen für die Berechnung des FTW 
geschrieben zu haben.

[S. R. (svenska)]

Karl H. schrieb:
> ISR(TIM_COMP_vect)
> {
>   buttonDebounceISR();
> }
>
> das willst du nicht tun!
>
> Du willst nicht aus einer ISR heraus eine Funktion aufrufen!

Sorge dafür, dass die Funktion inlined wird (eventuell per LTO), dann 
ist das kein Problem. Den Ansatz, in der ISR mehrere Funktionen 
aufzurufen, ist jedenfalls grundsätzlich nicht schlecht; das Ganze 
hässlich machen kann der Compiler selbst.

Uwe S. schrieb:
> hier ist noch ein anderer Ansatz für die Berechnung des FTW eines AD8950
> mit FCLK = 125MHz konstant !.
>
> FTW = (34 +1/2 -1/7 +1/385 -1/528.613) * f ; f in Hz.
>
> D.h. man benötigt 1 Multiplikation, 4 Divisionen, 2 Additionen und 2
> Subtraktionen

Das ist großer Unfug. Du ersetzt jetzt nämlich eine Laufzeitoperation 
("Konstante einsetzen") durch 9x Arithmetik, plus Registerbenutzung und 
so weiter. Die Division alleine wird so viel Code erzeugen, dass der 
Flash wieder platzt; die Rechenzeit ignoriere ich dabei mal gepflegt.

Zumal ich nicht verstehe, warum du 2^32/FCLK unbedingt zur Laufzeit 
ausrechnen willst. Das kann der Compiler auch selbst. Dann bleibt da 
nämlich "FTW = f * k" stehen, und das ist eine Laufzeitmultiplikation.

[B. S. (bestucki)]

S. R. schrieb:
> Zumal ich nicht verstehe, warum du 2^32/FCLK unbedingt zur Laufzeit
> ausrechnen willst. Das kann der Compiler auch selbst. Dann bleibt da
> nämlich "FTW = f * k" stehen, und das ist eine Laufzeitmultiplikation.

Weil

(frequency * 4294967296) / 125000000UL;

nicht immer das gleiche Ergebnis wie

frequency * (4294967296 / 125000000UL);

liefert.

Die Idee von Uwe ist, die 64 Bit Operationen durch 32 Bit Operationen zu 
ersetzen.

[Uwe S. (de0508)]

Hallo S. R.,

an wen ist der Kommentar gerichtet ?

S. R. schrieb:
> Uwe S. schrieb:
>> hier ist noch ein anderer Ansatz für die Berechnung des FTW eines AD8950
>> mit FCLK = 125MHz konstant !.
>>
>> FTW = (34 +1/2 -1/7 +1/385 -1/528.613) * f ; f in Hz.
>>
>> D.h. man benötigt 1 Multiplikation, 4 Divisionen, 2 Additionen und 2
>> Subtraktionen
>
> Das ist großer Unfug. Du ersetzt jetzt nämlich eine Laufzeitoperation
> ("Konstante einsetzen") durch 9x Arithmetik, plus Registerbenutzung und
> so weiter. Die Division alleine wird so viel Code erzeugen, dass der
> Flash wieder platzt; die Rechenzeit ignoriere ich dabei mal gepflegt.
>
> Zumal ich nicht verstehe, warum du 2^32/FCLK unbedingt zur Laufzeit
> ausrechnen willst. Das kann der Compiler auch selbst. Dann bleibt da
> nämlich "FTW = f * k" stehen, und das ist eine Laufzeitmultiplikation.

Wenn ich gemeint bin, dann zeige bitte genau, wie Du die Berechnung des 
FTW ohne Nutzung der 64Bit Arithmetik machen möchtest.

Bei mir belegt die Funktion uint32_t ftw_ad9850(uint32_t freq); 380Byte 
Code im Flash.

[Frickelfritze (Gast)]

Uwe S. schrieb:
> Bei mir belegt die Funktion uint32_t ftw_ad9850(uint32_t freq); 380Byte
> Code im Flash.

Ich habe es nachvollzogen. Die Rechnung mit Approximation und mehrfacher
Division in uint32_t ist deutlich kompakter als die Rechnung der
Multiplikation und Division mit long long (uint64_t). Grund ist
hauptsächlich dass bei der 64 bit Rechnung grössere Divisionsroutinen
aus der AVR Lib (ich nenne es mal so) mit eingebunden werden müssen.

[Frickelfritze (Gast)]

Frickelfritze schrieb:
> dass bei der 64 bit Rechnung grössere Divisionsroutinen
> aus der AVR Lib (ich nenne es mal so) mit eingebunden werden müssen.

Wenn allerdings bei grösseren Programmen der (64-bit-)Code aus der
Lib sowieso vorhanden ist, ist der Grössen-Vorteil dahin.

[Jörg W. (dl8dtl) (Moderator)]

Frickelfritze schrieb:
> Ich habe es nachvollzogen.

Mit welcher Compilerversion?  (Hatte ich oben den TE schon mal gefragt.)

Ich habe so in Erinnerung, dass Johann da in letzter Zeit mal was
getan hatte an den Bibliotheksroutinen.

Ggf. stellst du ja einfach deinen (compilierfähigen) Code mal allen
zur Verfügung, damit das jeder selbst testen kann?

[Frickelfritze (Gast)]

Jörg W. schrieb:
> Mit welcher Compilerversion?

GCC vX.xx --> AVR Studio 4.18

Source momentan nicht greifbar.

[Jörg W. (dl8dtl) (Moderator)]

Frickelfritze schrieb:
> GCC vX.xx --> AVR Studio 4.18

Ach du Sch***e!  Der ist ja älter als die Braunkohle. ;-)

Das taugt keineswegs als Beweis, inzwischen hat sich die Welt mächtig
weitergedreht.

[avion23 (Gast)]

Ich habe letztens mit -flto herum gespielt und konnte dadurch 16kb 
sparen.

[Jörg W. (dl8dtl) (Moderator)]

avion23 schrieb:
> 16kb sparen.

Auf einem ATtiny2313 wäre das eine sehr interessante Variante. :-))

[Frank M. (ukw) (Moderator)]

avion23 schrieb:
> Ich habe letztens mit -flto herum gespielt und konnte dadurch 16kb
> sparen.

Das Problem ist, dass der zu WinAVR 4.18 ursprünglich mal gehörende 
avr-gcc von 2010 gar kein -flto kann.

Nimmt der TO eine aktuellere Toolchain, wird sich sein Problem nach ein 
paar Handgriffen wahrscheinlich schon in Luft auflösen.

Hier ist beschrieben, wie das auch mit WinAVr 4.18 möglich ist:

    Beitrag "Re: WinAVR 2010 / AVR-GCC und Atmega48PA"

Eine aktuelle Toolchain (z.B. 4.7.2 oder höher) bekommt man hier:

    http://sourceforge.net/projects/mobilechessboar/files/avr-gcc%20snapshots%20%28Win32%29/

[Uwe S. (de0508)]

Hallo Miteinander,

hier ist noch ein weiterer Ansatz, aber von der Codegröße schlechter, 
für die Berechnung des FTW für einen AD9850 mit Fclk = 125MHz.
Diesen "findet" man unter Betrachtung der Wertigkeiten der 
Nachkommastellen von: k = 2³²/125MHz ~ 34,359738368

#include <avr/io.h>
#include <stdint.h>
#include <stdlib.h>

// Hilfsmacro
#define FTW_TERM(mul,div)  ((uint32_t)(mul) * freq) / (uint32_t)(div)

// Prototype
uint32_t ftw_ad9850( uint32_t freq );


int main()
{
  volatile uint32_t ftw;

  ftw = ftw_ad9850( 10000000 );
  while (1) { }
}


uint32_t ftw_ad9850( uint32_t freq )
{
// ftw = f * 34.359738368
return ((uint32_t)34 *freq) +
  FTW_TERM(3,10) +
  FTW_TERM(5,100) +
  FTW_TERM(9,1000) +
  FTW_TERM(7,10000) +
  FTW_TERM(3,100000) +
  FTW_TERM(8,1000000) +
  FTW_TERM(3,10000000) +
  FTW_TERM(6,100000000) +
  FTW_TERM(8,1000000000) ;
}

[Frickelfritze (Gast)]

Jörg W. schrieb:
> Das taugt keineswegs als Beweis, .....

Ich hatte nicht den Anspruch, einen Beweis anzutreten, das wird mir
von dir einfach so angedichtet. Ich wollte lediglich wissen was los
ist und habe die Breechnung nachvollzogen.

[Frickelfritze (Gast)]

Jörg W. schrieb:
> Ggf. stellst du ja einfach deinen (compilierfähigen) Code mal allen
> zur Verfügung, damit das jeder selbst testen kann?

Hier mal meine Verifikation ..... your mileage may vary.
Vielleicht findet jemand noch Fehler oder Verbesserungsmöglichkeiten.

Einfach wechselweise  die beiden  #if 0  und  #if 1  togglen um
beide Codeversionen zu erzeugen. Bin gespannt was bei neueren
Compiler-Versionen rauskommt.

Interessant wäre noch (für Binärcode-Kenner) warum die 64 Bit Version
zusätzlich viel RAM braucht.

/********************************************************/
/***  DDS9850_calc.c                                   **/
/***  DDS Word berechnen aus Frequenz                  **/
/********************************************************/

#include "avr/io.h"

/*******************************************************/
/*  Dummy Set Routine                                  */
/*  fuer den Fall dass der Compiler wegoptimieren will */
/*******************************************************/
void Set_DDS (uint32_t val)
{
  PORTB = val;
  PORTB = val >>  8;
  PORTB = val >> 16;
  PORTB = val >> 24;

} /* ---- Set_DDS ---- */


#if 0
/*******************************************************/
/*  Berechnung mit 64 bit Multiplikation / Division    */
/*******************************************************/
uint32_t Calc_FTW (uint32_t freq)
{
  const uint64_t  cSysClock = 125000000;
  uint32_t  dds_word;

  dds_word = (uint32_t) ( ((long long)freq * (long long)(0x100000000ll)) / cSysClock);

  return dds_word;

} /* ---- Calc_FTW ---- */
#endif


#if 1
/*******************************************************/
/*  Approximation mit mehreren 32 bit Divisionen       */
/*******************************************************/
uint32_t Calc_FTW (uint32_t freq)
{
  /*  FTW = f * k -->  k = 2³²/125MHz                 */
  /*  k ~ 34,3597 ~ (34 + 1/2 - 1/7 + 1/385 - 1/528613)  */
  /*      34,35973836   */
  /*  FTW ~ f*34 + f/2 - f/7 + f/385 - f/528613   */

  uint32_t  dds_word;

  dds_word = (freq*34) + (freq/2) - (freq/7) + (freq/385) - (freq/528613);

  return dds_word;

} /* ---- Calc_FTW ---- */
#endif


/***********************/
/*      main           */
/***********************/
int main (void)
{
  uint32_t  ftw_val;
  uint32_t  freq_dds ;

  freq_dds = 32123765;
  ftw_val = Calc_FTW (freq_dds);
  Set_DDS (ftw_val);

  return 0;

}  /* ---- main ---- */

/* ----------------------------------------- */
/* ------ End of file DDS9850_calc.c  ------ */
/* ----------------------------------------- */

[eProfi (Gast)]

Es reicht eine 32x32-Multiplikation.
Beitrag "Rechnen mit AVR"

[Jörg W. (dl8dtl) (Moderator)]

Frickelfritze schrieb:
> Ich hatte nicht den Anspruch, einen Beweis anzutreten, das wird mir
> von dir einfach so angedichtet.

Sorry, ich wollte dir nichts "andichten".

Danke für den Code.  Hab's mal in #ifdef USE_64 / #else umgeschrieben,
damit kann man das direkt auf der Kommandozeile testen, ohne die Datei
nochmal zu editieren.

Hier das Ergebnis für einen GCC 4.8.3 (auch schon nicht mehr der
Jüngste):

$ avr-gcc -Os -mmcu=attiny2313 -o foo.elf foo.c
$ avr-size foo.elf
   text    data     bss     dec     hex filename
    598       0       0     598     256 foo.elf
$ avr-gcc -Os -mmcu=attiny2313 -DUSE_64 -o foo.elf foo.c
$ avr-size foo.elf
   text    data     bss     dec     hex filename
    380       0       0     380     17c foo.elf

Hier ist die 64-bit-Rechnung also deutlich platzsparender als der
Workaround.

Frickelfritze schrieb:
> Interessant wäre noch (für Binärcode-Kenner) warum die 64 Bit Version
> zusätzlich viel RAM braucht.

Das müsstest du an deinem Compilat untersuchen.  Ich erinnere mich
dunkel, dass da mal irgendwas war, wo die libgcc ein 256 Byte großes
Array angelegt hat, um irgendwelche Bitsucherei zu beschleunigen.
Möglicherweise ist das ja bei dir der Fall.

[Frickelfritze (Gast)]

.... hmmmm ... schon interessant, aber ich habe nicht die Kenntnis
und Erfahrung um aus dem Binary schlau zu werden.

Ich poste hiermit mal die für die obige Source erzeugten *.lss und *.map
files für die 64 bit Version. Vielleicht kann sich jemand einen Reim
daraus machen. Es fällt auf dass viel Code dazugelinkt wird, warum auch
immer. Und es scheinen "pauschal" 256 Byte RAM allokiert zu werden.
Aus der Menge an Variablen nicht zu erklären ....

Wie bereits angedeuted ist das mit Studio 4.18 erzeugt worden. Ist
ja aber in den Files auch ersichtlich.

[Mark B. (markbrandis)]

Frank M. schrieb:
> Nimmt der TO eine aktuellere Toolchain, wird sich sein Problem nach ein
> paar Handgriffen wahrscheinlich schon in Luft auflösen.

Also ein klassischer Fall von "Problem an der falschen Stelle gelöst"?

[Frickelfritze (Gast)]

Frickelfritze schrieb:
> Wie bereits angedeuted ist das mit Studio 4.18 erzeugt worden.

Nachtrag: und für einen Mega88 übersetzt ... ob das eine Rolle spielt?

[Jörg W. (dl8dtl) (Moderator)]

Posten des ELF-Files wäre hilfreicher gewesen: Disassemblieren kann es
sich jeder selbt, aber man könnte dann auch schnell einen Blick auf die
Symboltabelle werfen.

Ja, der „Störenfried“ ist dieser da:

 .data          0x00800100      0x100 c:/winavr-20100110/bin/../lib/gcc/avr/4.3.3/avr4\libgcc.a(_clz.o)
                0x00800100                __clz_tab

Wenn du mal nach AVR-GCC __clz_tab gugelst, findest du das Problem
seinerzeit sicher hinreichend diskutiert.

Aber vielleicht wären ja solche Artefakte (zusammen mit der ja offenbar
insgesamt deutlich mieseren Optimierung) endlich mal ein Anlass für
einen Upgrade der Toolchain?  Du kannst ja auch eine aktuelle Toolchain
unter das olle Studio schieben, allerdings musst du dann dem Compiler
mit -gstrict-dwarf sagen, dass er keine erweiterten Debug-Infos erzeugt,
denn diese lassen den handgefrickelten DWARF-Parser dieses alten Studios
abstürzen (nicht, weil der Compiler buggy ist, sondern weil der Parser
nicht nach dem DWARF-Standard geschrieben worden war, sondern offenbar
per trial&error auf das maßgeschneidert, was der damalige Compiler so
rausgeworfen hat).

> Nachtrag: und für einen Mega88 übersetzt ... ob das eine Rolle spielt?

Ja, denn der hat einen Hardwaremultiplizierer, den der vom TE
genutzte ATtiny2313 nicht hat.  Damit würde das Compilat also dann
nochmal größer werden.

[Frickelfritze (Gast)]

Danke für die Hinweise.

Jörg W. schrieb:
> Aber vielleicht wären ja solche Artefakte (zusammen mit der ja offenbar
> insgesamt deutlich mieseren Optimierung) endlich mal ein Anlass für
> einen Upgrade der Toolchain?

Ja, damit hadere ich schon die ganze Zeit, die Lösung mit aktuellerem
GCC und Studio 4.18 wäre eine gute Alternative um das überladene,
resourcenfressende Studio 7.xx zu umgehen. Zumal die Durchsichtigkeit,
Übersichtlichkeit eines Projekts in der neuen Umgebung nicht besser
wird. Mal sehen was ich mir dabei noch für Nebeneffekte einhandele.

Übrigens der Unterschied zwischen Mega88 und Tiny2313 in der Grösse
des Codes ist nur minimal....  abgesehen davon dass die 4K nicht in
einen Tiny2313 passen würden ....

[Frickelfritze (Gast)]

Jörg W. schrieb:
> Wenn du mal nach AVR-GCC __clz_tab gugelst, findest du das Problem
> seinerzeit sicher hinreichend diskutiert.

--> siehe  Beitrag "Stack Überschreiber beim Rechnen mit uint64_t"

auf diesen Thread bzw danach zu suchen wäre ich mein Leben lang
nicht gekommen.