AVR Comiler Optimazation Crasht den Controller (ATMEGA328P)

[M. W. (Gast)]

Hallo Liebes Forum,

Ich bin total am Verzweifeln. Bis jetzt hatte ich eigentlich noch keine 
groben Probleme bei der Entwicklung meiner AVR Tiny und Megas aber jetzt 
sitze ich schon seit 2 Wochen ohne Erfolg.

Ziel war die Entwicklung eines kleinen USB-Sticks, der meine 
Funksteckdosen in der Wohnung schalten kann. Den wollte ich an meinen 
Microserver anstöpseln und über Serielle Schnittstelle (CH340) 
ansprechen. So weit so gut. Er schaltet die Dosen aber nach jedem 
Durchlauf Knallt es und der Controller springt wieder in die 
Main-Routine. Gemerkt habe ich es, da mit kein Success ausgegeben wird 
sonder nur folgender String:

>SuHey Dude!

Er stürzt also bei der Ausgabe von Success in der Funktion remotepwr ab.

Nachdem ich einiges Herumprobiert habe ist mir aufgefallen, dass bei 
ausgeschalteter Optimierung im Compiler alles super läuft nur dann ist 
das Programm knapp 200% größer.

Ich würde gerne verstehen warum das so ist und daraus lernen. In der 
Hoffnung hier in dem Forum treibt sich ein Profi rum für den das ein 
Kinderspiel ist.

Viele Lieben Dank für eure Unterstützung,
Michael

/*
 * USB RemoteSwitch.c
 *
 * Created: 03.02.2018 14:20:45
 * Author : Michael
 */ 

#define F_CPU 16000000UL
#include <stdlib.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/pgmspace.h>
#include <string.h>
#include <util/delay.h>
#include <ctype.h>

#include "uart.h"

/************************************************************************/
/* RF USER DEFINITIONS - EDIT HERE                      */

#define TELEGRAM_REPEAT 8  // minimum 2 (more recommended)
#define RF_PORT PORTD    // port used for pin toggle
#define RF_PIN PD7      // pin where rf transmitter is connected to

/* RF USER DEFINITIONS - DO NOT EDIT HERE */

#define HIGH 1
#define LOW 0
#define ON 1
#define OFF 0

/************************************************************************/

/************************************************************************/
/* RF FUNCTIONS EDIT HERE                      */

void ClockPin(int PinState, int Clocks){
  
  // set pin state
  switch(PinState){
    case HIGH:
    RF_PORT |= (1 << RF_PIN); break;
    case LOW:
    RF_PORT &= ~(1 << RF_PIN); break;
    default:; break;
  }
  
  // keep the pin state for the duration of clocks
  for(int i=0; i<= Clocks; i++) _delay_us(342);
}

void BuildQuery(char *query, char *group, char socket, int state){
  
  strcat(query, group);
  
  switch(socket){
    case 'a': case 'A': strcat(query, "0FFFF"); break;
    case 'b': case 'B': strcat(query, "F0FFF"); break;
    case 'c': case 'C': strcat(query, "FF0FF"); break;
    case 'd': case 'D': strcat(query, "FFF0F"); break;
    case 'e': case 'E': strcat(query, "FFFF0"); break;
    default:; break;
  }
  
  if(state>0){ strcat(query,"0F");}
  else{ strcat(query, "F0"); }
}

void SwitchSocket(char group[5], char socket, int state){
  
  char query[12] = "";
  BuildQuery(query, group, socket, state);
  
  char c;
  
  for(int i=0; i < TELEGRAM_REPEAT; i++)
  {
    for(int k=0; k<12; k++)
    {
      c = query[k];
      
      switch(c){
        case '0': /* Signal "0" -> dip on -> 1 0 0 0, 1 0 0 0 */
        ClockPin(HIGH, 1);
        ClockPin(LOW, 3);
        ClockPin(HIGH, 1);
        ClockPin(LOW, 3); break;
        case 'F': /* Signal "float" -> dip off -> 1 0 0 0, 1 1 1 0 */
        ClockPin(HIGH, 1);
        ClockPin(LOW, 3);
        ClockPin(HIGH, 3);
        ClockPin(LOW, 1); break;
        default:; break;
      }
    }
    
    ClockPin(HIGH, 1);
    ClockPin(LOW, 31);
  }
}
/************************************************************************/


//Baud-Rate
 
#define UART_BAUD_RATE  9600

//Uart Special Characters
#define TRUE 1
#define FALSE 0
#define CHAR_NEWLINE '\n'
#define CHAR_RETURN '\r'
#define RETURN_NEWLINE "\r\n"

//Uart Variable
unsigned char data_count = 0; //Character Count for Input Data
unsigned char data_in[16];    //Input Data will be stored here
char command_in[16];      //Final Command for String Compare
int tmp =0;

void remotepwr(char input[16])
{
  char *pch = NULL;
  char cmdValue[16];
  char *ptr = NULL;
  char rgroup[16];
  int  rsocket = 0;
  char rstate = 0;
  int  error = 0;
  // Find the position the equals sign is
  // in the string, keep a pointer to it
  pch = strchr(input, '=');
  if (pch == NULL)
  {
    error = 1;
  }

  if (error == 0)
  {
    // Copy everything after that point into
    // the buffer variable
    strcpy(cmdValue, pch + 1);
    cmdValue[9] = '\0';
    // Now turn this value into an integer and
    // return it to the caller.
    ptr = strtok(cmdValue, ",");

    for (int i = 0; i < 3; i++)
    {
      if (ptr == NULL)
      {
        error = 1;
        break;
      }
      if (error != 0); else
      {
        if (i == 0 && error == 0)
        {
          //Parameter 0 (1) is group
          strcpy(rgroup, ptr);
          rgroup[5] = '\0';
          if (strlen(rgroup) < 5)
          {
            error = 1;
          }
        }
        if (i == 1 && error == 0)
        {
          //Parameter 1 (2) is socket
          //Check if Parameter 2 only contains letters from A to E or a to e
          if ((ptr[0] >= 'a' && ptr[0] <= 'e') || (ptr[0] >= 'A' && ptr[0] <= 'E'))
          {

            rsocket= ptr[0];
            
          }
          else
          {
            error = 1;
          }
        }

        if (i == 2 && error == 0)
        {
          //Parameter 2 (3) is state
          rstate = atoi(ptr);
          if (rstate == 0 || rstate == 1); else
          {
            error = 1;
          }
        }
        if (error != 0); else
        {
          ptr = strtok(NULL, ",");
        }
      }
    }



    if (error != 0)
    {
      uart_puts("error");
      uart_puts(RETURN_NEWLINE);
    }
    else
    {

      SwitchSocket(rgroup, rsocket, rstate);
      uart_puts("success");
      uart_puts(RETURN_NEWLINE);
    }

  }
}



int parse_assignment (char input[16]) {
  char *pch;
  char cmdValue[16];
  // Find the position the equals sign is
  // in the string, keep a pointer to it
  pch = strchr(input, '=');
  // Copy everything after that point into
  // the buffer variable
  strcpy(cmdValue, pch+1);
  // Now turn this value into an integer and
  // return it to the caller.
  return atoi(cmdValue);
}


void print_value (char *id, int value) {
  char buffer[16];
  itoa(value, buffer, 16);
  uart_puts(id);
  uart_putc('=');
  uart_puts(buffer);
  uart_puts(RETURN_NEWLINE);
}

void uart_ok() {
  //uart_puts("OK");
  //uart_puts(RETURN_NEWLINE);
}

void copy_command () {
  // Copy the contents of data_in into command_in
  memcpy(command_in, data_in, 16);
  // Now clear data_in, the UART can reuse it now
  memset(data_in, 0, 16);
}

void process_command() {
  if(strcasestr(command_in,"rfs") != NULL){
    if(strcasestr(command_in,"=?") != NULL)
      uart_puts_P("Used to Switch RF Socket rfs=GROUP,SOCKET, STATE");
    else
      remotepwr(command_in);
  }
} 

void process_uart(){
  /* Get received character from ringbuffer
   * uart_getc() returns in the lower byte the received character and 
   * in the higher byte (bitmask) the last receive error
   * UART_NO_DATA is returned when no data is available.   */
  unsigned int c = uart_getc();
  
  if ( c & UART_NO_DATA ){
    // no data available from UART 
  }
  else {
    // new data available from UART check for Frame or Overrun error
    if ( c & UART_FRAME_ERROR ) {
      /* Framing Error detected, i.e no stop bit detected */
      uart_puts_P("UART Frame Error: ");
    }
    if ( c & UART_OVERRUN_ERROR ) {
      /* Overrun, a character already present in the UART UDR register was 
       * not read by the interrupt handler before the next character arrived,
       * one or more received characters have been dropped */
      uart_puts_P("UART Overrun Error: ");
    }
    if ( c & UART_BUFFER_OVERFLOW ) {
      /* We are not reading the receive buffer fast enough,
       * one or more received character have been dropped  */
      uart_puts_P("Buffer overflow error: ");
    }
    
    // Add char to input buffer
    data_in[data_count] = c;
    
    // Return is signal for end of command input
    if (data_in[data_count] == CHAR_RETURN) {
      // Reset to 0, ready to go again
      data_count = 0;
      uart_puts(RETURN_NEWLINE);
      
      copy_command();
      process_command();
      uart_ok();
    } 
    else {
      data_count++;
    }
    //Local echo of command (in terminal mode)
    uart_putc( (unsigned char)c );
  }
}








int main(void) { 
  
  //########################################INIT+++##########################
 
  uart_init( UART_BAUD_SELECT(UART_BAUD_RATE,F_CPU) ); //Select Baud Rate and initialize library
  sei();                         //Needed for UART Library - Enable global Interrupt
  uart_puts("Hey Dude!");
  uart_puts(RETURN_NEWLINE);
  PORTD |= (1<<RF_PIN);
  
 
 while (1) {
  process_uart();

  } 
}

[MaWin O. (Gast)]

data_in kann durch Nutzerdaten zum Überlauf gebracht werden und 
command_in kann möglicherweise keine NUL-Terminierung haben.

[M. W. (Gast)]

Hi,

das Kommando ist sehr kurz gehalten.
Es schaltet nur die Steckdosen ein und aus.

Hier ein Beispiel:

rfs=0FF0F,B,1

Das komische ist ja, das wenn ich die Optimazation in AVR Studio 
ausschalte (-O0) funzt alles. Bei (-01) knallt es immer an der Selben 
stelle:

>SuHey Dude!

Der ersten 2 Ziffern von Success werden ausgegeben. Ich habe auch schon 
mit dem ICE-Debugger nanchgeschaut, es schein alles zu passen und mit 
Optimization plötzlich: Crash. Reproduzierbar.

[Theor (Gast)]

In der Regel liegt so etwas an unentdeckten Fehlern im Code. Denn der 
Compiler macht mit an Sicherheit grenzender Wahrscheinlichkeit nichts 
falsch. Eher hast Du Code geschrieben, der dem Compiler Deutungen offen 
lässt, die Du nicht beabsichtigt hast.

Leider ist der Code teilweise unübersichtlich. Ich sehe nichts 
Offensichtliches, aber eine Menge Stellen, die ich so nicht schreiben 
würde.


Der erste Rat wäre, alle Warnungen einzuschalten -Wall und alle so 
ausgegebenen Warnungen zu beseitigen.

Der zweite wäre, soviel wie möglich defensiv zu programmieren. Etwa 
strncpy anstelle von strcpy und so weiter. Wo das nicht möglich ist, 
Prüfungen auf Array-Längen einzubauen.

Der dritte, dieses ziemlich komplizierte if-geraffel streng zu straffen. 
Verschachtelte Prüfungen auf die selbe Bedingung sind unnötig und 
fehleranfällig.

Falls das dann immer noch auftritt, den Code hier nochmal - als Anhang - 
posten.

[Hubert (Gast)]

Delay weg optimiert?

[Kaj (Gast)]

Hat zwar nichts mit dem Problem zu tun, aber was mit auffaellt:

if (...); else
{
    ...
}

Das Semikolon hat da nichts zu suchen.

Und auch sonst:

if (error != 0); else

Das hast du 2x drin. Wozu soll das gut sein? Das macht den Code nur 
schwerer zu lesen und zu verstehen. Warum machst du das nicht 
ordentlich? Denn was da steht, mit den leeren if und dem else ist nichts 
anderes als das:

if (error == 0)
{

}

          if (rstate == 0 || rstate == 1); else
          {
            error = 1;
          }
        }
        if (error != 0); else
        {
          ptr = strtok(NULL, ",");
        }

Sorry, aber der Code ist echt schwer zu lesen. Raeum den mal bitte auf, 
dann ist es auch leichter einen Fehler zu finden...

[neuer PIC Freund (Gast)]

Du musst spätestens alle (MAGIGNUMBER-1) ein CHAR_RETURN empfangen. 
Ansonsten schredderst du dein RAM.

[Stefan F. (Gast)]

Wenn du in einem Elektronik Forum schreibst, dass es "Knallt" dann 
verstehe ich das im Sinne des Wortes. Bei Dir hat aber wohl doch nichts 
geknallt.

Du meinst, dass dein Programm ausfällt. Da solltest du mal untersuchen, 
an welcher Stelle das passiert. Deine Aussage, dass es immer die selbe 
Stelle sei, hilft nicht weiter und ist womöglich sogar irreführend.

Füge die Ausgabe von Debug Meldungen hinzu, damit du daran ablesen 
kannst, ab wo das Programm nicht mehr so abläuft, wie geplant. Eventuell 
hilft Dir dabei meine Vorlage: 
http://stefanfrings.de/avr_hello_world/index.html

Aber als aller erstes solltest Du Theors und Kajs Vorschläge umsetzen. 
Danach löst sich dein Problem womöglich schon in Luft auf, oder Dir 
fällt die Fehlerursache selbst auf. Denn gut lesbarer Code enthält 
tendenziell weniger Fehler und die sind dort offensichtlicher.

[Rufus Τ. F. (rufus)]

Kaj schrieb:
> Das Semikolon hat da nichts zu suchen.

Wie ein Compiler so etwas ohne Fehlermeldungen übersetzen kann, ist mir 
gelinde gesagt ein Rätsel.

[Apollo M. (Firma: @home) (majortom)]

Rufus Τ. F. schrieb:
> Wie ein Compiler so etwas ohne Fehlermeldungen übersetzen kann, ist mir
> gelinde gesagt ein Rätsel.

warum? das ist doch einfach nur ein leerer if-zweig, kein fehler wo der 
compiler was zu sagen muss.

der coding style hat was von wildwest romantik à la ein cowboy reitet 
durch die wüste der ahnungslosen.

nahezu unlesbar für mein spatzenhirn. ich würde mal strukuren andenken 
für die commando verarbeitung und sinnvolle namen benutzen anstatt 
nahezu sinnlose kommentare.

respekt an alle die da noch so nett bleiben konnten.


mt

[Rufus Τ. F. (rufus)]

Apollo M. schrieb:
> warum? das ist doch einfach nur ein leerer if-zweig, kein fehler wo der
> compiler was zu sagen muss.

Das stimmt natürlich; für mich ist so etwas so grindig, daß ich es 
automatisch als Fehler betrachte.

[Apollo M. (Firma: @home) (majortom)]

... "grindig" kannte ich noch nicht - danke wieder was dazu gelernt!

meine triggerschwelle ist noch niedriger - ich bekomme schon eine krise, 
wenn ich so etwas sehe

 if (error != 0) ...

oder

 if (error == 0) ...

anstatt

 if (error) ...

und

 if (!error) ...

[M.K. B. (mkbit)]

Ich kann den anderen Posts nur zustimmen. Warnings sind immer hilfreich 
und Längen sollten geprüft werden, damit man den Fehler erkennt, wenn 
dieser auftritt und nicht erst durch späteres Fehlverhalten.

Meine Vermutung ist, dass sich durch die Optimierung die Verteilung der 
Variablen auf dem Stack ändert. Damit kann sich das Verhalten bei einem 
Zugriff außerhalb des Arrays verändern. Und Zugriffe außerhalb des 
Arrays sind undefiniert, d.h. das Programm darf machen, was es will.

Bei einem kurzen Blick auf den Code ist mir folgendes aufgefallen.

process_uart()
Kommt kein CHAR_RETURN innerhalb von 16 Zeichen, dann schreibst du über 
den Puffer hinaus. Was machst du, wenn Teile eines Kommandos verloren 
gehen?

process_command()
command_in hat nicht zwingend einen '\0' als letztes Zeichen ('\n' ist 
für c-strings eine ganz normales Zeichen). Die Stringfunktionen 
verlassen sich aber darauf, dass ein String mit '\0' endet. strcasestr 
hört damit nie auf, außer es findet zufällig eine null auf seinem Weg 
durch den Speicher. Irgendwann wird es aber einen ungültigen 
Adressbereich lesen wollen, was in der Regel zu einer Exception oder 
einem Reset des Controllers führt.

remotepwr()
Gleiches Problem wie process_command().

[Kaj G. (Firma: RUB) (bloody)]

Rufus Τ. F. schrieb:
> Kaj schrieb:
>> Das Semikolon hat da nichts zu suchen.
>
> Wie ein Compiler so etwas ohne Fehlermeldungen übersetzen kann, ist mir
> gelinde gesagt ein Rätsel.
So, jetzt auch wieder als angemeldeter Nutzer, war mir gestern zu spaet 
bzw. zu frueh :D

Ich hab mal folgendes in Eclipse eingehackt:

int main(void) {
    int i = 5;

    if (i == 4); else
    {
        printf("i is not 4\n");
    }

    return 0;
}

Da zeigt der Editor immerhin eine Meldung und markiert das Semikolon:

Suspicious semicolon

Eine gute IDE (ob Eclipse das ist, darueber kann man streiten) ist durch 
nichts zu ersetzen.

Das das compiliert liegt aber wirklich an dem leeren if. Versucht man 
das if mit Leben zu fuellen gibt es, wie zu erwarten, einen Error:

$ make
gcc -std=c11 -O0 -g3 -Wall -Wextra -Wunused-variable -Wunreachable-code -Wsign-conversion -Wmissing-braces -Wparentheses -Wsequence-point -pedantic -o c_test.elf src/main.c
src/main.c: In function 'main':
src/main.c:8:16: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
     if (i == 4); {
                ^
src/main.c:8:5: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
     if (i == 4); {
     ^~
src/main.c:8:18: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
     if (i == 4); {
                  ^
src/main.c:10:6: error: 'else' without a previous 'if'
     }else {
      ^~~~
make: *** [Makefile:37: c_test] Error 1

Wie auch immer:
Eine Warnung vom Compiler waere schon schoen gewesen (beim if(...); 
else).

[Apollo M. (Firma: @home) (majortom)]

off topic - but very important!

... für die, die interesse haben an effektives flag handling mit 
attiny/atmega/... welche extra gpio register im io address space dafür 
haben, die bit orientiert mit sbi/cbi manipuliert werden. das spart dann 
bei vielen flags richtig size und space. der atmega328P hat 3 gpio also 
platz für 24 flags! die nachfolger kommen mit 5 gpio daher.

und noch ein vorteil - nach reset werden die register via hw auf null 
initialisiert!

hier am beispiel der variable "int error" einsetztbar aus der dann 
errorFLAG wird. alles für die 3 gpio's beliebig erweiterbar auf 24 
flags.

enum error_t {OK, NOK};

typedef struct {
  enum error_t eflag:1;
  enum error_t      :7;
} flag_t;

#ifdef DEBUG //flags defined as byte/int structure to get symbol infos
  volatile flag_t flags;
  #define errorFLAG ((flag_t*) &flags)->eflag

#else //flag defined as gpio bit structure
  #define errorFLAG ((flag_t*) &GPIOR0)->eflag

#endif


also flash/ram code space gespart und speed verbessert!

und noch ein link zu einem recht guten program 
(https://github.com/sepich/SynNotes) um code snippets strukturiert 
abzulegen und wieder zu finden.
gerade dann, wenn die ide so etwas nicht mitbringt - das program bekommt 
eine klare empfehlung!


mt

[Egon N. (egon2321)]

Du benutzt relativ wenige IO Funktionen. Imho würde es reichen, diese 
auszukommentieren und anschließend kannst du deinen Code durch Valgrind 
jagen. Der findet bei C eigentlich immer irgendetwas das man selbst 
übersehen hätte.

Kompilier immer mit -Wall, -Wconversion und den ganzen Optionen um 
möglichst viele Warnungen zu erhalten, generell würde ich dazu raten 
nichts kompilieren zu lassen was auch nur irgendeine Warnung ausspuckt.



An welcher Stelle stürzt dein Programm nun immer ab? Diese Information 
wäre recht hilfreich. ;)

[Johann L. (gjlayde)]

Kaj G. schrieb:
> Wie auch immer:
> Eine Warnung vom Compiler waere schon schoen gewesen (beim if(...);

Na die ist doch da!

> src/main.c:8:16: warning: suggest braces around empty body in an 'if'
> statement [-Wempty-body]
>      if (i == 4); {
>                 ^

D.h. wenn man gerne einen leeren Block hätte dann sollte man {} (braces) 
verwenden statt ;

[devzero (Gast)]

Egon N. schrieb:
> An welcher Stelle stürzt dein Programm nun immer ab? Diese Information
> wäre recht hilfreich. ;)

Das schrieb er doch bereits mehrmals.

[Kaj G. (Firma: RUB) (bloody)]

Johann L. schrieb:
> Kaj G. schrieb:
>> Wie auch immer:
>> Eine Warnung vom Compiler waere schon schoen gewesen (beim if(...);
>
> Na die ist doch da!
Ja, aber nur, wenn:

if(...); {
    ...
} else {
    ...
}

Mein "eine Warnung waere schoen" bezog sich aber auf:

if(...); else {
    ...
}

Da gibt es naemlich keine Warnung. Auch nicht mit -Wall -Wextra.

[Apollo M. (Firma: @home) (majortom)]

Kaj G. schrieb:
> Mein "eine Warnung waere schoen" bezog sich aber auf:if(...); else {
>     ...
> }
> Da gibt es naemlich keine Warnung. Auch nicht mit -Wall -Wextra.

warum auch? nur weil es grindig aussieht?!
von der syntax ist alles im grünen bereich.

ich empfehle mal das buch "moderne c-programmierung" von schellong, zu 
finden als pdf!

schon mal was von ko-logik bei c gehört? mit dem buch lernt man, was 
c-programmierung wirklich bedeutet kann.

ich denke, die meisten legen das buch aber schell beiseite. weil das ist 
richtig starker tabak und eher nicht für unser "niveau" hier.

[codeguy (Gast)]

Das ist absolut verheerender Code. "Hoffnung" scheint Dein Schlüsselwort 
zu sein.

1) Wilde Konstanten werden ohne Begründung (Kommentar!) irgendwo im Code 
verwendet:

> char group[5]
> ...
> char query[12]
> ...
> char command_in[16]
> ...
> char input[16]
> ...
> char rgroup[16]

Für Konstanten gibt es #define, und dann ist ein Kommentar fällig, warum 
die Größe genau die richtige ist!


2) Ein wilder for-if-else-Verhau:

> if (error == 0)
> ...
> for (int i = 0; i < 3; i++)
>   ...
>   if (i == 0 && error == 0)
>   ...
>   if (i == 1 && error == 0)
>   ...
>   if (i == 2 && error == 0)

Schon mal überlegt, dass der Code leichter zu lesen wäre, wenn die 
Schleife eliminiert wird?!


3) Und dann diese vielen kleinen unschönen Details:

- Man schreibt niemals nie:
> if (rstate == 0 || rstate == 1); else
> {
>   error = 1;
> }

sondern:
if ( rstate != 0 && rstate != 1 ) error = 1;

oder:
if ( rstate < 0 || rstate > 1 ) error = 1;


- Wenn du mit if i == 0 bis i == 2 testest, dann sollte die zugehörige 
Schleife auch diese Zahlen verwenden: "for (int i = 0; i <= 2; i++)"

(Aber ich sagte ja schon, ganz weg mit der Schleife!)


4) Schmerzhafte Formatierung:

Es gibt zu viele gute Gründe, folgendes:
> }
> else
> {
zu ersetzen durch "} else {". Z.B. dass weniger Zeilen vollgemüllt 
werden, oder dass ein zu einem Block gehörendes "else" nicht so leicht 
übersehen werden kann.

5) Ach, bitte noch mal neu.

[codeguy (Gast)]

Jetzt ersthaft: Schreibe das Programm ein zweites Mal und nimm ein gute 
integrierte Entwicklungsumgebung (IDE), die Dir Deinen Code besser 
formatiert und Dir sogar Tips gibt, wie Du redundante Formulierungen 
verbessers. Es wird so viel einfacher!

[codeguy (Gast)]

Kaj G. schrieb:
> $ make
> gcc -std=c11 -O0 -g3 -Wall -Wextra -Wunused-variable -Wunreachable-code
> -Wsign-conversion -Wmissing-braces -Wparentheses -Wsequence-point
> -pedantic -o c_test.elf src/main.c
> src/main.c: In function 'main':
> src/main.c:8:16: warning: suggest braces around empty body in an 'if'
> statement [-Wempty-body]
>      if (i == 4); {
>                 ^
> src/main.c:8:5: warning: this 'if' clause does not guard...
> [-Wmisleading-indentation]
>      if (i == 4); {
>      ^~
> src/main.c:8:18: note: ...this statement, but the latter is misleadingly
> indented as if it were guarded by the 'if'
>      if (i == 4); {
>                   ^
> src/main.c:10:6: error: 'else' without a previous 'if'
>      }else {
>       ^~~~
> make: *** [Makefile:37: c_test] Error 1

Du hast das Semikolon nicht verstanden!

Es beendet ein Statement. Geschweifte Klammern ermöglichen einen Block 
an einer Stelle, wo sonst nur ein Statement erlaubt ist.

Nach if kommen runde Klammern mit der zu prüfenden Bedingung und danach 
ein Statement:

if ( bedingung ) statement ;

Wie eben geschrieben, kannst Du statt des stements einen Block mit 
mehreren Statements setzen:

if  ( bedingung ) { statement ; statement ; statement ; }

Wenn Du schreibst:

if ( bedingung ) ;

dann hast Du ein leeres Statement definiert, d.h. es passiert nichts im 
if-Zweig. nun kannst Du "else statement ;" anhängen und vielleicht 
dämmert Dir nun, warum der Compiler oben genau das tut, was jeder 
erfahrene C-Programmierer erwarten würde.

Und die von Dir gewünschte Warnung liefert bereits Eclipse!

[Hubert (Gast)]

Kann denn einer der Semikolon Fritzen was zumThema sagen? Was wird 
kaputt optimiert?

[codeguy (Gast)]

Nichts wird "kaputt optimiert", das Programm ist schon "kaputt" und wenn 
es scheinbar funktioniert, dann nur "zufällig". ENDE

[Einer K. (Gast)]

Nichts wird kaputt optimiert.
Der Kot ist kaputt..
Und kaputter Kot verhält sich manchmal anders, als man meint.

Also Aufräumen!
Pingelig auf Array Bereichsüberschreitungen achten.
Alle Zeiger immer gültig?

Ist der Code mal Fehlerfrei, dann tut ihm die Optimierungsstufe auch 
nicht mehr weh.

[codeguy (Gast)]

Kaj G. schrieb:
>>> Eine Warnung vom Compiler waere schon schoen gewesen (beim if(...);
>>
>> Na die ist doch da!
> Ja, aber nur, wenn:if(...); {
>     ...
> } else {
>     ...
> }
>
> Mein "eine Warnung waere schoen" bezog sich aber auf:if(...); else {
>     ...
> }
> Da gibt es naemlich keine Warnung. Auch nicht mit -Wall -Wextra.

Oh nein, Du weisst auch den Unterschied zwischen warning und error 
nicht. Obiger Fall ist ein error (warum? Hab ich vorhin erklärt: der 
Block nach dem leeren Statement hat nichts mehr mit dem if zu tun, daher 
kann kein else angehängt werden).

Der zweite Fall ist ein warning "leeres statement"

Das hier ist insgesamt der grausamste Fall an Programmierhalbwissen, mit 
dem ich es in den letzten Jahren zu tun hatte. Bitte Kernighan/Ritchie 
2nd Ed. durcharbeiten und verstehen. Ansonsten vielleicht besser 
handwerken.

[codeguy (Gast)]

if ( Kernighan/Ritchie 2nd Ed. durchgearbeitet && verstanden )
  programmieren;
else
  handwerken;

[Apollo M. (Firma: @home) (majortom)]

codeguy schrieb:
> Nichts wird "kaputt optimiert", das Programm ist schon "kaputt" und wenn
> es scheinbar funktioniert, dann nur "zufällig". ENDE

korrekt, das war auch mein erster gedanke!

wenn ich lese

"... In der Hoffnung hier in dem Forum treibt sich ein Profi rum für den 
das ein Kinderspiel ist."

dann ist das vom gleichen tenor wie z.b. bei ebay, wenn da schrott 
verkauft werden soll.

"... ich habe keine ahnung von dem gerät, aber für einen profi sicher 
einfach zu reparieren ... vielleicht nur die sicherung, oder eine 
lockere schraube,
aber sonst funktioniert das gerät super!"

Äh?!


mt

[codeguy (Gast)]

Ich bin direkt etwas ärgerlich, aber es tut mir auch leid, dass jemand 
an dem Programm 2 Wochen verschwendet hat. Ich bin Profi und es ist viel 
einfacher für mich, es neu zu schreiben, als all die Unschönheiten zu 
beseitigen. Denn dann fängt man an, die Fehler zu sehen, die auch noch 
korrigiert werden müssen. Und dann stellt man fest, dass das Konzept 
falsch durchdacht ist, und muss sowieso alles neu programmieren.

Jetzt aber wirklich ENDE

[Hubert (Gast)]

@codegay
Du bist kein Profi, sondern ne arme Sau, die hier auf dicke Hose macht
 Schreib das doch gerade um und Zeit was du fürs Rakete bist.

Aber das kannst du gar nicht, Troll.

[Gregor M. (Gast)]

Was mich in diesem Forum hier tatsächlich wundert (in Gegensatz zu 
vielen Englisch-Sprachigen pendants) das hier augenscheinlich 
Reihenweise Profis rumlaufen die eine Dicke Lippe riskieren und "alles 
anders machen" würden.

Ich programmiere selber seit vielen Jahren und im Prinzip stimme ich zu. 
Nur für mich sieht das aus, als lernt da gerade jemand seine ersten 
Schritte was ich durchaus begrüße.

Aber anstatt hier Erfahrungen auszutauschen und zu Helfen wird (leider 
nicht nur in diesem Thread) draufgehauen was das Zeug hält und es wird 
zum Teil weit unter die Gürtellinie geschossen.

Was soll das denn sein? Es gibt weder "Grindigen" noch "Perfekten" Code, 
aber es gibt Erfahrungswerte und "Best Practices" die man unerfahrenen 
doch Vermitteln kann. Auch wenn vieles Hier stimmt, ist der Ton einfach 
brutal unagebracht und jemand der sich versucht mit einem Thema zu 
beschäftigen wird gleich zu Beginn niedergemacht.

Ich frage mich wie viele Leute hier tatsächlich Profis sind und wie 
viele Jahre Ihr alle gebraucht habt um so weit zu kommen. Und wenn ich 
ehrlich bin sehe ich es als große Leistung an auch erst den Schritt in 
die Programmierung zu wagen - Gerade C ist nicht sehr einfach zu lernen 
--> Pointer.

Aus meiner Erfahrung und auch bei meinen Uni-Abgängern im Büro kommt es 
in erster Linie darauf an Erfahrungen auszutauchen und zu HELFEN anstatt 
draufzuhauen und blöde sprüche zu Kloppen.

Traurig. Ich bin in vielen Foren (speziell in USA) unterwegs aber solch 
dummen sprüche sind scheinbar in Deutschland (und auch Österreich --> 
Grindig) üblich.

Mich persönlich würde ja mal interessieren wie die großen Maker hier 
programmieren.

@Michael: Kopf hoch und nicht entmutigen lassen. Der Code gehört 
ordentlich aufgeräumt aber das kommt mit der Erfahrung!

Gute Nacht und Gutes Gelingen - Und dem Rest ein bischen Einsicht und 
die Erkentniss das JEDER mal kein angefangen hat.

Gregor

[codeguy (Gast)]

Hatte gestern einen schlechten Tag und möchte das wieder gut machen:

  Das hier ersetzt den bisherigen Aufruf von remotepwr:

    if ( remotepwr( input ) == false ) {
      uart_puts( "error" );
      uart_puts( RETURN_NEWLINE );
    }

  Das ist die umgeschriebene Kernfunktion:

  boolean remotepwr( char input[16] ) {
    char cmdValue[16];
    char *ptr;
    char rgroup[16];
    int rsocket = 0;
    char rstate = 0;
    int error = 0;

    // Find the position of the equal sign in the string, keep a pointer to it
    ptr = strchr( input, '=' );
    if ( ptr == NULL ) return false;

    // Copy everything behind the equal sign into the buffer
    strcpy( cmdValue, ptr + 1 );
    cmdValue[9] = '\0';

    // ??? Now turn this value into an integer and return it to the caller
    ptr = strtok( cmdValue, "," );
    if ( ptr == NULL ) return false;

    // Parameter 0 (1) is group
    strcpy( rgroup, ptr );
    rgroup[5] = '\0';
    if ( strlen( rgroup ) < 5 ) return false;

    ptr = strtok( NULL, "," );
    if ( ptr == NULL ) return false;

    // Parameter 1 (2) is socket
    // Check if Parameter 2 only contains a letter from A to E or a to e
    if ( ( ptr[0] >= 'a' && ptr[0] <= 'e' ) || ( ptr[0] >= 'A' && ptr[0] <= 'E' ) )
      rsocket = ptr[0];
    else
      return false;

    ptr = strtok( NULL, "," );
    if ( ptr == NULL ) return false;

    // Parameter 2 (3) is state
    rstate = atoi( ptr );
    if ( rstate != 0 && rstate != 1 ) return false;

    SwitchSocket( rgroup, rsocket, rstate );
    uart_puts( "success" );
    uart_puts( RETURN_NEWLINE );
    return true;
  }

[codeguy (Gast)]

Offensichtlich wird hier ein String der Form "=ccccc,[a-eA-E],[01]," 
geparst und in seine Einzelteile zerlegt. Das sollte als Kommentar vor 
der Funktion stehen.

Der letzte "ptr = strtok( NULL, "," );" erwartet ein Komma, was ev. ein 
Fehler sein könnte. Aber das kann ja jemand anders herausfinden. Ich 
wollte nur einen Denkanstoß geben und mein schlechtes Verhalten von 
gestern korrigieren.

Nichts für ungut

[Blechbieger (Gast)]

Generell helfen IDEs und Autoformater wie Astyle bei der 
Codeschachtelung. Liefern sie seltsame Einrückungen stimmt etwas mit den 
Klammern oder Semikolons nicht. Auch kann Astyle erzwingen dass nach if 
immer ein Block kommt statt eines einzelnen Statements.

[codeguy (Gast)]

Nun hab ich mich daran festgebissen, daher hier die nächste Iteration, 
nämlich das Konzept des Parsings (kein strtok, kein Hilfsbuffer):

  boolean remotepwr( char input[16] ) {
    char *left, *right;
    char rgroup[6];      // The expected string needs exactly 6 chars
    int rsocket = 0;
    char rstate = 0;
    int error = 0;

    // Find the position of the equal sign in the string
    if ( (left = strchr( input, '=' )) == NULL ) return false;

    // left now points to the first character of rgroup
    ++left;

    // Find the position of the comma behind the = in the string
    if ( (right = strchr( left, ',' )) == NULL ) return false;

    // rgroup should exactly be 5 characters long
    if ( right - left != 5 ) return false;

    strncpy( rgroup, left, 5 );
    rgroup[5] = '\0';

    // Parameter 1 (2) is socket. right now points to it (or whatever)
    ++right;

    // Check if Parameter 2 only contains a letter from A to E or a to e
    if ( ( *right >= 'a' && *right <= 'e' ) || ( *right >= 'A' && *right <= 'E' ) )
      rsocket = *right;
    else
      return false;

    // Find the position of the next comma behind the comma in the string
    if ( (right = strchr( right, ',' )) == NULL ) return false;

    // right now points to the 3rd parameter (or whatever)
    ++right;

    // Parameter 2 (3) is state
    switch ( *right ) {
    case '0': rstate = 0; break;
    case '1': rstate = 1; break;
    default: return false;
    }

    // Make sure that the number has only 1 digit and that the
    // input string is not longer
    if ( right[1] != '\0' ) return false;

    SwitchSocket( rgroup, rsocket, rstate );
    uart_puts( "success" );
    uart_puts( RETURN_NEWLINE );
    return true;
  }

[Markus F. (mfro)]

M. W. schrieb:

> void BuildQuery(char *query, char *group, char socket, int state){
>
>   strcat(query, group);
>
>   switch(socket){
>     case 'a': case 'A': strcat(query, "0FFFF"); break;
>     case 'b': case 'B': strcat(query, "F0FFF"); break;
>     case 'c': case 'C': strcat(query, "FF0FF"); break;
>     case 'd': case 'D': strcat(query, "FFF0F"); break;
>     case 'e': case 'E': strcat(query, "FFFF0"); break;
>     default:; break;
>   }
>
>   if(state>0){ strcat(query,"0F");}
>   else{ strcat(query, "F0"); }
> }
>
> void SwitchSocket(char group[5], char socket, int state){
>
>   char query[12] = "";
>   BuildQuery(query, group, socket, state);
>

Mir scheint, hier geht vor lauter "Jehova"-Schreien der Blick für's 
Wesentliche verloren.

query ist als char[12] deklariert.

Im oben gezeigten Ausschnitt werden fünf Zeichen Nutzdaten in query 
kopiert, dann kommen (in BuildQuery()) nochmal fünf Zeichen dazu und 
anschliessend werden (mit strcat()) noch zwei Zeichen hinten angehängt.

Macht in der Summe 12.

Der geneigte, unvoreingenommene Leser fragt sich: wo bleibt das 
Stringende-Kennzeichen?

Dass der Rest vom gezeigten Code nicht gerade schön ist, wurde ja schon 
ausführlich durchgehechelt ...

[codeguy (Gast)]

Das allerwichtigste, nämlich aussagekräftige Fehlermeldungen, will ich 
nicht unterschlagen (übrigens hatte ich auch atoi() eliminiert, da es in 
diesem simplen Fall aus Effizienzgründen (Speicherbedarf, Laufzeit) 
vermieden werden kann):

  // Parse a string of the form "=ccccc,[a-eA-E],[01]\n"
  // and call SwitchSocket() with the three extracted parameters
  boolean remotepwr( char input[16] ) {
    char *left, *right;
    char rgroup[6];      // The expected string needs exactly 6 chars
    int rsocket = 0;
    char rstate = 0;

    // Find the position of the equal sign in the string
    if ( (left = strchr( input, '=' )) == NULL ) {
      uart_puts( "ERROR: No equal sign!\r\n" );
      return false;
    }

    // left now points to the first character of rgroup (1. parameter)
    ++left;

    // Find the position of the comma behind the = in the string
    if ( (right = strchr( left, ',' )) == NULL ) {
      uart_puts( "ERROR: No comma sign!\r\n" );
      return false;
    }

    // rgroup should exactly be 5 characters long
    if ( right - left != 5 ) {
      uart_puts( "ERROR: 1st parameter does not have 5 chars!\r\n" );
      return false;
    }

    strncpy( rgroup, left, 5 );
    rgroup[5] = '\0';

    // 2nd arameter is socket. right now points to it (or whatever)
    ++right;

    // Check if Parameter 2 only contains a letter from A to E or a to e
    if ( ( *right >= 'a' && *right <= 'e' ) || ( *right >= 'A' && *right <= 'E' ) )
      rsocket = *right;
    else {
      uart_puts( "ERROR: 2nd parameter is not a-e and A-E!\r\n" );
      return false;
    }

    // Find the position of the next comma behind the comma in the string
    if ( (right = strchr( right, ',' )) == NULL ) {
      uart_puts( "ERROR: No second comma symbol!\r\n" );
      return false;
    }

    // right now points to the 3rd parameter (or whatever)
    ++right;

    // 3. parameter is state
    switch ( *right ) {
    case '0': rstate = 0; break;
    case '1': rstate = 1; break;
    default:
      uart_puts( "ERROR: 3rd parameter is not '0' or '1'!\r\n" );
      return false;
    }

    // Make sure that the number has only 1 digit and that the
    // input string is not longer
    if ( right[1] != '\0' ) {
      uart_puts( "ERROR: 3rd parameter is longer than 1 symbol or input continues behind 3rd parameter!\r\n" );
      return false;
    }

    SwitchSocket( rgroup, rsocket, rstate );
    uart_puts( "success\r\n" );
    return true;
  }

Der Aufruf kann nun den Rückgabewert ignorieren, da die Fehlermeldungen 
direkt in der Funktion erzeugt und ausgegeben werden.

[Kaj G. (Firma: RUB) (bloody)]

codeguy schrieb:
> da die Fehlermeldungen
> direkt in der Funktion erzeugt und ausgegeben werden.
Aber will man das? Ich wuerde aus Sicht der Wartbarkeit lieber den 
Errorstring zusammenbauen, und an einer einzigen Stelle ausgeben.
Also irgendwie so (pseudocode):

if (...) {
    error_msg = "...";
}

if (...) {
    error_msg = "...";
}

if (...) {
    error_msg = "...";
}

if (error_msg) {
    uart_puts(error_msg);
}

Bzw. den String an den Aufrufer zurueckgeben. Dann kann man das ganze so 
gestallten, das es nur genau eine Stelle im Code gibt, wo die 
Schnittstelle bedient wird. Macht das ganze auch einfacher und wenn man 
Interrupts nutzt auch weniger fehleranfaellig (pseudocode):

int main(void)
{
    char error_msg[xxx] = {0};
    char status_msg[xxx] = {0};

    while (1) {
        foo(status_msg, error_msg);
        bar(status_msg, error_msg);
        
        if (status_msg[0]) {
            uart_puts(status_msg);
            memset(status_msg, 0, sizeof(status_msg));
        }

        if (error_msg[0]) {
            uart_puts(error_msg);
            memset(error_msg, 0, sizeof(error_msg));
        }
    }
}

Auch das staendige manuelle Anhaengen von "\r\n" wuerde ich in einer 
Funktion verstecken. Also irgendwie so (pseudocode):

void uart_puts_rn(char* msg)
{
    uart_puts(msg + "\r\n");
}

Ist weniger laestig, und man kann das Anhaengen nicht vergessen.

Auch die Zuweisungen innerhalb eines if-Statements sind nicht wirklich 
schoen, und werden zurecht von genuegend Tools angemeckert.

codeguy schrieb:
> übrigens hatte ich auch atoi() eliminiert, da es in
> diesem simplen Fall aus Effizienzgründen (Speicherbedarf, Laufzeit)
Bevor wir uns hier ueber Effizienz unterhalten, sollte erstmal die 
korrekte Funktion sichergestellt sein. Effizienz bringt nichts, wenn der 
Code immernoch fehlerhaft (im sinne von: "macht nicht was es soll") ist.

Da ich deiner Meinung nach ja aber eh keine Ahnung hab, solls mir auch 
egal sein.

[Marco H. (damarco)]

ehrlich gesagt ist die Art in deinem Programm wie du mit Strings umgehst 
sehr problematisch. Auch mit Pointern ! Genau das wird dir in die Suppe 
spucken wenn der Compiler anfängt zu optimieren packt er die Sachen 
aneinander womit das in die Hose gehen muß.

[codeguy (Gast)]

@ Kaj G.

Ja, wunderbar: lauter gute Vorschläge von Dir. Kann man alles umsetzen 
und verbessert das Programm weiter. Worauf ich hinauswollte ist, dass es 
unbedingt Sinn macht, Konzept und Struktur eines Programms nach 
Alternativen zu untersuchen. In obigem Fall ist einfach die 
Schleifenstruktur das falsche Konzept.

Ich würde tatsächlich die Fehler sofort ausgeben, wenn sie passieren, 
sonst kann es vorkommen, dass eine in error_msg gespeicherte 
Fehlermeldung durch einen nachfolgenden Absturz verschluckt wird und 
damit sinnlos wird.


@ Marco H.

Meinst Du mich?