Problem durch Optimierung -Os

[Torsten W. (torsten_w)]

Hi,

ich arbeite gerade an einem CAN Bootloader, der auf einem atmega32 
laufen soll, welcher dazu einen MCP2515 via SPI verwendet, um mit dem 
Bus zu kommunizieren.
Um die zum Flashen erforderlichen Daten auf den Bus zu bekommen verwende 
ich einen atmega168, der ebenfalls einen MCP2515 via SPI verwendet, 
mittels UART die Daten von meinem Computer gesendet bekommt und somit 
sozusagen ein Gateway darstellt.
Momentan schreibe ich den "Bootloader" noch in den ganz normalen 
Programmspeicher, da ich mich bis jetzt primär um das implementieren der 
Datenübertragung gekümmert habe. Da die Datenübertragung nun aber 
fehlerfrei funktioniert wollte ich mich an die eigentlichen Funktionen 
eines Bootloaders machen, nämlich dem schreiben in den Speicher.

Da ich bis jetzt immer mit -O0 die Optimierung deaktiviert habe, hat 
sich mein Code auf 7kbyte aufgebläht. Um die verfügbaren 4kbyte Boot 
Section des atmega32 nicht zu überschreiten möchte ich den Schalter -Os 
verwenden um mit Fokus auf Speicherplatz zu optimieren.

Durch diese Änderung entstehen jedoch einige Probleme, die ich als 
zugegebener Amateur im Bereich Mikrocontroller und C nicht ganz 
nachvollziehen kann. Mir ist bekannt, dass durch die Optimierung in 
vielen Fällen Dinge übergangen werden, weil der Compiler denkt, dass 
gewisse Sachen sich nicht ändern können usw. Bis jetzt bin ich aber 
davon ausgegangen, dass sich dies nur auf globale Variablen in 
Verbindung mit Interrupts beschränkt. Im Anhang hab ich mal den gesamten 
Code angehangen, durch ausprobieren konnte ich aber schon feststellen, 
dass es in folgenden Funktionen / Prozeduren hapert.
Ich kann mir aber leider nicht erklären, wo genau die Fehler sind und 
hoffe, dass ihr mir dabei helfen könnt.

Folgende Prozedur verwende ich im Bootloader um während des Empfangens 
der Daten, Daten an das UART-CAN Gateway zurück zu senden, die dann auf 
der UART Schnittstelle ausgegeben werden. In erster Linie dient es in 
der Testphase zum überprüfen der Daten. Im Moment verwende ich es jedoch 
um einfach die Funktionalität der Kommunikation über CAN zu testen.
Leider funktioniert es im Moment nicht richtig.
In meinem Terminal werden korrekterweise die ersten 7 Zeichen aus 
'input' ausgegeben, dann bleibt die Prozedur jedoch in der 
'while(escape_flag == 0)' schleife stecken.
Zum Verständnis muss ich noch sagen, dass ich mir damit zuerst das 
erfolgreiche empfangen und Verarbeiten der Daten vom UART-Gateway 
quittieren lasse um einen Buffer-Overflow im CAN Controller des UART-CAN 
Gateways zu verhindern, woraufhin weiter gesendet wird.
Dies geschieht durch das Empfangen einer CAN-Botschaft (das empfangen 
einer Nachricht signalisiert der CAN-Controller über das auf LOW ziehen 
von 'PD7') mit der ID = 1 und dem Wert 
'BOOTLOADER_MESSAGE_CLEAR_TO_SEND' im ersten Datenfeld der 
CAN-Botschaft.

void write_bootloader_message_buffered_to_can(uint16_t length, char *input){
  static uint16_t offset = 0;
  uint8_t loops = (length / 7)+1;

  for(uint8_t i=0;i<loops;i++){
    uint8_t len = 7;
    if(i+1 == loops){
      len = length % 7;
    }
    
    char *data = malloc(sizeof(char)*(len+1));
    memcpy(data+1,input+offset, (size_t)len);
    data[0] = BOOTLOADER_MESSAGE;
    offset += len;
    
    write_message_to_can(DEVICE_ID,len+1,data);
    free(data);

    uint8_t escape_flag = 0;
    while(escape_flag == 0){
      
      if((PIND & (1<<PD7)) == 0){
        Message *message;
        handle_interrupt(message);
        //escape_flag = 1;
        if((message->id == 1) && (message->data[0] == BOOTLOADER_MESSAGE_CLEAR_TO_SEND)){
          escape_flag = 1;
        }
      }
    }
  }
  offset = 0;
}

(File: can_interface.c Line:63)

Genau hier tritt jedoch das Problem auf. Die ID wird fälschlicherweise 
mit einem wert von 30-40 angegeben (kann es leider nicht genau sagen, da 
ich es mir durch eine blinkende LED ausgeben lassen musste), den Inhalt 
von 'data[0]' habe ich mir gar nicht erst anzeigen lassen.
Ich vermute, dass in der 'handle_interrupt(message);' welche die Daten 
per SPI aus dem MCP2515 ausließt ein Konflikt mit der Optimierung 
auftaucht, da das Programm bis zu diesem Punkt problemlos arbeitet.

Die 'handle_interrupt(message);' sieht wie folgt aus und verwendet 
einige weitere Funktionen / Prozeduren aus 'can2515.c':

uint8_t handle_interrupt(Message *message)
{
    uint8_t status = read_rx_status();

  set_cs(LOW);

  if(status & (1<<3)){
    message->rtr = 1;
  }
    if (status & (1 << 6)) {
    spi_putc(SPI_READ_RX);
    get_message_from_buffer(0, message);
    set_cs(HIGH);
    return MESSAGE_IN_BUFFER_0;
    } else if (status & (1 << 7)) {
    spi_putc(SPI_READ_RX | 0x04);
    get_message_from_buffer(1, message);
    set_cs(HIGH);
    return MESSAGE_IN_BUFFER_1;
    }else{
    set_cs(HIGH);
    return NO_MESSAGE;
  }
  
}

(File: can2515.c Line: 114)


Da ich 'can2515.c' für Testzwecke auch in anderen Programmen verwendet 
habe und z.B. auch Grundstein des CAN-UART Gateway Programms ist, 
befinden sich die Dateien 'can2515.c/h', 'spi.c/h' und 'regs.h' nicht 
direkt in diesem Projekt sondern werden gesondert als Library gebaut und 
dann eingebunden. Ich hab auch das Makefile dafür in den Anhang gepackt.

Ich arbeite auf einem Linux System und und verwende avr-gcc 4.3.5.

Ich hoffe ich habe bei dem vielen Text nichts wichtiges vergessen und 
bedanke mich jetzt schon mal fürs lesen.
Ich poste natürlich gerne weitere wissenswerte Informationen.


Gruß,
toti

[Stefan E. (sternst)]

        Message *message;
        handle_interrupt(message);

Du hast gar keinen Speicher für deine Message reserviert. message ist 
ein Pointer, der einfach irgendwo in den Speicher zeigt. Wenn das mit 
-O0 funktioniert hat, war das purer Zufall.

Richtig wäre z.B.:

        Message message;
        handle_interrupt(&message);

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

Sowas sollte der Compiler eigentlich anzeigen. Warnungen ignoriert?

[Stefan E. (sternst)]

Noch eine kleine Extrabemerkung:
Wozu das umständliche malloc/free für data? Warum nicht einfach 
data[8]? Ist doch egal, wenn im letzten Schleifendurchlauf nicht alle 8 
gebraucht werden.

[Torsten W. (torsten_w)]

Hi,

so viel zum Thema Amateur in C ;) Vielen Dank, das wars wirklich. Ich 
weiß nicht wirklich was mich da geritten hat. Ich glaube ich bin 
dummerweise davon ausgegangen, dass der Compiler bei einem 'struct' 
anders handelt und den Speicher reserviert. Dummer Gedanke, der dann 
durch den Erfolg beim Verzicht auf Optimierung auch noch gefördert 
wurde.
Bezüglich Warnungen war dar Hinweis darauf, dass ohne Optimierung die 
Funktionen aus dem Header 'delay.h' nicht funktionieren würden das 
einzige. Beim Compilen mit -Os lief der Compiler ohne eine einzige 
Warnung durch. An der Stelle würde mich jetzt auch interessieren, hätte 
der Compiler ein 'Warning' ausgeben müssen?
Was mich auch noch interessieren würde ist, ob es wirklich reiner Zufall 
war, dass es ohne Optimierung funktioniert hat oder ob es daran liegt, 
dass der Compiler um Rechenzeit während des Compilens zu sparen einfach 
pauschal immer Speicher mit anlegt? Würde ja eigentlich auch keinen Sinn 
machen oder?

EDIT: Bezüglich des mallocs/free stimme ich völlig zu. Ich hatte vor an 
möglichst allen Stellen darauf zu verzichten um mir den Speicherplatz 
von malloc() ganz zu sparen. Der soll ja auch nicht wenig sein.

Nochmal vielen Dank,
toti

[Klaus W. (mfgkw)]

Torsten W. schrieb:
> Beim Compilen mit -Os lief der Compiler ohne eine einzige
> Warnung durch. An der Stelle würde mich jetzt auch interessieren, hätte
> der Compiler ein 'Warning' ausgeben müssen?

Kompilierst du mit -Wall -Wextra (falls gcc, sonst halt entsprechende 
Optionen bei deinem Compiler)?

[Torsten W. (torsten_w)]

Mit -Wall wird es mir jetzt angezeigt. Sogar noch drei weitere Warnings. 
Danke

[Stefan E. (sternst)]

Torsten W. schrieb:
> Was mich auch noch interessieren würde ist, ob es wirklich reiner Zufall
> war, dass es ohne Optimierung funktioniert hat

Ja, war es. Wahrscheinlich hat dort der Pointer einfach nur in einen 
ansonsten unbenutzten Bereich des RAM gezeigt.

[Klaus W. (mfgkw)]

Torsten W. schrieb:
> Mit -Wall wird es mir jetzt angezeigt. Sogar noch drei weitere Warnings.

Daraus kann man messerscharf schließen, daß es generell eine gute Idee
ist, mit -Wall und am besten auch gleich mit -Wextra zu kompilieren,
und sich um die Warnungen auch zu kümmern bzw. sie mit Sinn und
Verstand zu beseitigen.

Z.B. wird bei deinem Programm (-teil) nicht so recht klar, wieso
du für normale Strings unsigned char[] nimmst.
De Verwendung von signed/unsigned-Varianten für ganze Zahlen ist
ebenfalls immer eine Überlegung wert.
Weiterhin übergibst du einen nichtkonstanten Zeiger für den String,
was sich mir nicht erschließt.
Gleich vorweg: "funktioniert doch so" ist die dämlichste Ausrede :-)

[Torsten W. (torsten_w)]

Hi,

auf "funktioniert doch" wollte ich mich heute mal nicht rausreden ;) . 
Mein Problem bezüglich der Datentypen ist, dass ich programmiertechnisch 
eher aus dem Bereich Java komme, wo man sich über solche Sachen ja eher 
weniger Gedanken macht und einem als Programmierer auch eine ganze Menge 
Arbeit abgenommen wird.
Aus diesem Grund tue ich mich mit der Menge an Datentypen (im Vergleich 
zu Java), die einem bei c zur Verfügung stehen noch etwas schwer und 
muss noch reichlich Erfahrung sammeln was das angeht.
Die Erfahrung, dass man mit 'Warnings' nicht scherzen sollte habe ich im 
Vorfeld bereits gemacht und angefangen diese sehr ernst zu nehmen. Das 
Vergessen von -Wall war dabei eher ein Versehen aus Mangel an Erfahrung, 
da dies auch mein erstes aufwändigeres Projekt in C ist.

Was genau meinst du im Bezug auf die Verwendung von 'unsigned char[]' 
und in welchem Teil genau?

Im Hinblick auf das Übergeben des Pointers für den String hätte ich doch 
bei einer Deklaration des Parameters als 'const' den Vorteil, dass ich 
wegen der Read-Only Übergabe nicht aus Versehen in den Daten 
rumschreiben könnte oder? Gibt es noch andere Aspekte in aus deren Sich 
das sinnvoll ist? In diesem Punkt muss ich aber leider zugeben, dass ich 
da bis jetzt mit der Motivation "funktioniert doch so" gehandelt habe ;)

Gruß,
toti

[Klaus W. (mfgkw)]

Torsten W. schrieb:
> Was genau meinst du im Bezug auf die Verwendung von 'unsigned char[]'
> und in welchem Teil genau?

Sorry, das hatte ich mit einem anderen Thread verwechselt :-(

Der Sinn meiner Aussage bleibt aber natürlich ...

Torsten W. schrieb:
> Im Hinblick auf das Übergeben des Pointers für den String hätte ich doch
> bei einer Deklaration des Parameters als 'const' den Vorteil, dass ich
> wegen der Read-Only Übergabe nicht aus Versehen in den Daten
> rumschreiben könnte oder? Gibt es noch andere Aspekte in aus deren Sich
> das sinnvoll ist?

Ein weiterer Vorteil ist, daß der Compielr von Fall zu Fall
deutlich besseren Code erzeugen kann.

Außerdem kann man eine Funktion, die selber durch const zusichert,
einen String nicht zu ändern, wiederum aus einer anderen aufrufen,
die selbiges zusichert.
Ein const-String beim Aufrufer dürfte nämlich strenggenommen nicht
als Parameter an eine Funktion übergeben werden, die den als
nicht-const bekommt.

Torsten W. schrieb:
> In diesem Punkt muss ich aber leider zugeben, dass ich
> da bis jetzt mit der Motivation "funktioniert doch so" gehandelt habe ;)

Wie gesagt, meinte ich dich eigentlich gar nicht, aber war trotzdem
ein Treffer :-)

[Stefan E. (sternst)]

Klaus Wachtler schrieb:
> Torsten W. schrieb:
>> Im Hinblick auf das Übergeben des Pointers für den String hätte ich doch
>> bei einer Deklaration des Parameters als 'const' den Vorteil, dass ich
>> wegen der Read-Only Übergabe nicht aus Versehen in den Daten
>> rumschreiben könnte oder? Gibt es noch andere Aspekte in aus deren Sich
>> das sinnvoll ist?
>
> Ein weiterer Vorteil ist, daß der Compielr von Fall zu Fall
> deutlich besseren Code erzeugen kann.
>
> Außerdem kann man eine Funktion, die selber durch const zusichert,
> einen String nicht zu ändern, wiederum aus einer anderen aufrufen,
> die selbiges zusichert.
> Ein const-String beim Aufrufer dürfte nämlich strenggenommen nicht
> als Parameter an eine Funktion übergeben werden, die den als
> nicht-const bekommt.

Nur mal so nebenbei:
Das gilt natürlich nicht nur für Strings, sondern für jede Art von 
Pointer, egal worauf er zeigt.

[Torsten W. (torsten_w)]

das hab ich mir schon gedacht und es gleich mal bei allen Funktionen die 
nur lesend auf die ihnen übergebenen Zeiger zugreifen angepasst.
Danke nochmal.

[Klaus W. (mfgkw)]

Torsten W. schrieb:
> nur lesend auf die ihnen übergebenen Zeiger zugreifen

zur Sicherheit: ich rede von einem const für das, worauf
die Zeiger zeigen, nicht für die Zeiger:

void f( char *s /* Zeiger auf char */ )
{
    ++s;    // s darf verändert werden
    ++(*s); // *s auch
}

void fconst( const char *s /* Zeiger auf konstante char */ )
{
    ++s;       // s darf verändert werden
    // ++(*s); // FALSCH! *s ist konstant
}

...
   char tmp[10] = "Hallo"; // 10 änderbare char
   f( tmp );          // ok
   fconst( tmp );     // ok (Funktion dürfte Zeichen ändern,
                      // tut es aber nicht)
   f(( "Hallo" );     // FALSCH! "Hallo" ist eine Konstante,
                      // f() sichert aber nicht zu, den String
                      // nicht zu ändern!
   fconst( "Hallo" ); // ok

[Torsten W. (torsten_w)]

Ja, so meinte ich das auch ;) Danke.