C code UART Atmega und Optimierung

[Bernd (Gast)]

Hallo Leute,

habe für einen ATMega folgenden Code erstellt, kann ich da noch was 
optimieren?

ich bin leider Anfänger in Sachen C Programmierung und wollte jetzt
etwas Hilfe. Vielleicht lässt sich da noch was verbessern...

 
 #define FOSC 16000000 
 #define F_CPU FOSC
 #define BAUD 9600
 #define MYUBRR FOSC/16/BAUD-1
 
 #include <avr/io.h>
 #include <util/delay.h>
 #include <avr/interrupt.h>
 #include <string.h>
 
 #define UART_MAXSTRLEN 32

 volatile uint8_t uart_str_complete = 0;   
 volatile uint8_t uart_str_count = 0;
 volatile char uart_string[UART_MAXSTRLEN + 1] = "";
   

void USART_Init ( unsigned int ubrr)
{
  UBRR0H = (unsigned char)(ubrr>>8);
  UBRR0L = (unsigned char)ubrr;
  UCSR0B = ((1<<RXEN0)|(1<<TXEN0)|(1<<RXCIE0));
  UCSR0C = (3<<UCSZ00);
}

ISR(USART_RX_vect)
{
  unsigned char nextChar;

  nextChar = UDR0;
  if( uart_str_complete == 0 ) {  

    
    if( nextChar != '\n' &&
    nextChar != '\r' &&
    uart_str_count < UART_MAXSTRLEN ) {
      uart_string[uart_str_count] = nextChar;
      uart_str_count++;
    }
    else {
      uart_string[uart_str_count] = '\0';
      uart_str_count = 0;
      uart_str_complete = 1;
    }
  }
}

void USART_Transmit( unsigned char data )
{
  while ( !( UCSR0A & (1<<UDRE0)) )
  ;
  
  UDR0 = data;  
}

void uart_puts (char *s)
{
  while (*s)
  {   
    USART_Transmit(*s);
    s++;
  }
}

 int main(void)
 {
    USART_Init (MYUBRR);
  sei();
  DDRB |= ((1 << PB0) | (1 << PB1) | (1 << PB2) | (1 << PB3) | (1 << PB4) | (1 << PB5));
  DDRC |= ((1 << PC0) | (1 << PC1) | (1 << PC2) | (1 << PC3) | (1 << PC4) | (1 << PC5));
  
  while (1)
  {
    if (uart_str_complete == 1)
    {
      if ( (uart_string[0] == '#') && (uart_string[1] == 'a'))
       {
      for (unsigned int i = 2;i < strlen(uart_string);i++)
       {
         switch (uart_string[i])
          {
            case 'A':  PORTC |= ( 1 << PB0 ); break;
            case 'a':  PORTC &= ~( 1 << PB0 ); break;
            case 'B':  PORTC |= ( 1 << PB1 ); break;
            case 'b':  PORTC &= ~( 1 << PB1 ); break;
            case 'C':  PORTC |= ( 1 << PB2 ); break;
            case 'c':  PORTC &= ~( 1 << PB2 ); break;
            case 'D':  PORTC |= ( 1 << PB3 ); break;
            case 'd':  PORTC &= ~( 1 << PB3 ); break;
            case 'E':  PORTC |= ( 1 << PB4 ); break;
            case 'e':  PORTC &= ~( 1 << PB4 ); break;
            case 'F':  PORTC |= ( 1 << PB5 ); break;
            case 'f':  PORTC &= ~( 1 << PB5 ); break;
            
            case 'G':  PORTB |= ( 1 << PB0 ); break;
            case 'g':  PORTB &= ~( 1 << PB0 ); break;
            case 'H':  PORTB |= ( 1 << PB1 ); break;
            case 'h':  PORTB &= ~( 1 << PB1 ); break;
            case 'I':  PORTB |= ( 1 << PB2 ); break;
            case 'i':  PORTB &= ~( 1 << PB2 ); break;
            case 'J':  PORTB |= ( 1 << PB3 ); break;
            case 'j':  PORTB &= ~( 1 << PB3 ); break;
            case 'K':  PORTB |= ( 1 << PB4 ); break;
            case 'k':  PORTB &= ~( 1 << PB4 ); break;
            case 'L':  PORTB |= ( 1 << PB5 ); break;
            case 'l':  PORTB &= ~( 1 << PB5 ); break;  
          }
       }
       }
      uart_str_complete = 0;
    }
  }
}

Gruß

  Bernd

[Sascha_ (Gast)]

Wenn du jetzt vielleicht noch sagen würdest, was dein Code tun soll?

[Bernd (Gast)]

oh, ja mein Code ist zum Empfang von einer String Zeile um
die PortPin von PORTB[0-5] und PORTC[0-5] ein- und auszuschalten.
Er soll aber nur dann reagieren, wenn die Zeile mit "#a" anfängt.

[fürn Hugo (Gast)]

Der Code ist perfekt! Keine Sorge wenn er das macht was er tun soll dann 
passt schon!

[Diek (Gast)]

Ich würde die Senderoutine auch noch auf Interrupt umstellen um die 
blockierende while Schleife loszuwerden. Sonst scheint das zu passen. 
Sieht mir ziemlich nach dem Beispiel aus dem Atmel Datenblatt aus.

[Bernd (Gast)]

ja, doch leider soll es eine quasi Master-Slave Geschichte werden. Also 
wollte ich den Zeitpunkt des Sendes fest in der Hand haben.
So das die µC nur auf ihren Start hören "#a", "#b", "#c" ....

[Achim (Gast)]

So ist es wirklich schön.

Verwirrend könnte sein, dass ein Telegramm mit \r und \n am Ende 2 Main 
-durchläufe startet.

[Draco (Gast)]

Bei deiner Portzuweisung passt was nicht:

case 'A':  PORTC |= ( 1 << PB0 ); break;

//...etc

Das macht bestimmt was es soll (Pin0-PortC einschalten) aber halt falsch 
geschrieben, damit du später mal nicht durcheinander kommst ändere dies 
noch in PC0, PC1, etc... um.

[Bernd (Gast)]

Oh, das ist mir wollte durch Kopie und "past" entstanden. Ja mache danke 
für den Tipp.

Gruss Bernd

[Bernd (Gast)]

Achim schrieb:
> So ist es wirklich schön.
>
> Verwirrend könnte sein, dass ein Telegramm mit \r und \n am Ende 2 Main
> -durchläufe startet.

Hi, sollte ich da nur aus eines Testen, also nur \r?
Dachte nur weil Windows und/oder Linux..

[nicht"Gast" (Gast)]

Moinsen,



ich würde an deiner Stelle den Seiteneffekt aus der Main rauswerfen.

Bernd schrieb:
> uart_str_complete = 0;

das sollte aus der main raus. Mach dir eine Funktion int GetLine(char 
*line), die 0 zurückgibt, wenn keine Zeile gelesen wurde und nicht null, 
wenn eine komplette Zeile da ist. Da kannst du dann prüfen, ob du eine 
Zeile im buffer hast.

Wenn du mal dein Programm erweitern willst, tust du gut daran, so was in 
Zukunft zu kapseln. Das kann sonst schnell ausarten.


Grüße,

[Peter D. (peda)]

Man könnte die Regel aufschreiben, nach der die Bits gesetzt werden:

uint8_t setbit( uint8_t val, uint8_t bit, uint8_t level )
{
  uint8_t mask = 1<<bit;
  val |= mask;
  if( level == 0 )
    val ^= mask;
  return val;
}

void setiobit( char c )
{
  uint8_t level = 0;
  switch( c ){
    case 'A' ... 'L': level = 1;
                      c += 'a' - 'A';
  }
  switch( c ){
    case 'a' ... 'f': PORTC = setbit( PORTC, c - 'a', level );
                      break;
    case 'g' ... 'l': PORTB = setbit( PORTB, c - 'g', level );
  }
}

[Tom (Gast)]

Das Hackergetrickse spart ein paar Zeilen, aber erfordert eine 
Dokumentation, die ungefähr so aussehen würde wie der Code vom OP 
sowieso schon aussah.

main() ist viel zu lang. Das Auswerten des Strings ist ein 
übersichtlicher und in sich abgeschlossener Block, das Setzen eines 
Ausgangs abhängig vom Zeichen auch. Deshalb darf man Funktionen daraus 
machen:

void set_output(char cmd)
{
    switch (cmd)
    {
    case 'A': PORTC |= ( 1 << PB0 ); break;
    case 'a': PORTC &= ~( 1 << PB0 ); break;
// .....
    case 'l': PORTB &= ~( 1 << PB5 ); break;
    default: break;
    }
}


void parse_command(const char* s)
{
    if(*s != '#') return;
    ++s;
    if(*s != 'a') return;
    ++s;
    while(*s)
    {
        set_output(*s);
        ++s;
    }
}

int main(void)
//.....
        if (uart_str_complete == 1)
        {
            parse_command(uart_string);
            uart_str_complete = 0;
        }
//.....

[Tom (Gast)]

Nachtrag: Man könnte überlegen, ob man einen einfachen Zustandsautomaten 
mitlaufen lässt, nach den Zeichen '#a' vom "Ich lauere"- in den "Ich bin 
dran"-Modus geht und ab dann jedes empfangene Zeichen ohne 
Zwischenspeicherung direkt auf die Ausgänge wirken lässt. Ein '\n' würde 
dann wieder auf "Ich lauere" schalten.

[Achim (Gast)]

Bernd schrieb:
> Hi, sollte ich da nur aus eines Testen, also nur \r?

Ich würde den Code so lassen schadet ja nicht, solange keiner "Fehler" 
zählt.. Eher würde ich das # als Startzeichen festlegen, und immer dann 
und nur dann neu starten, wenn es kommt.

Aber das gehört nicht zu C sondern zu Protokoll.

[Bernd (Gast)]

Peter D. schrieb:
> Man könnte die Regel aufschreiben, nach der die Bits gesetzt
> werden:uint8_t setbit( uint8_t val, uint8_t bit, uint8_t level )
> {
>   uint8_t mask = 1<<bit;
>   val |= mask;
>   if( level == 0 )
>     val ^= mask;
>   return val;
> }
>
> void setiobit( char c )
> {
>   uint8_t level = 0;
>   switch( c ){
>     case 'A' ... 'L': level = 1;
>                       c += 'a' - 'A';
>   }
>   switch( c ){
>     case 'a' ... 'f': PORTC = setbit( PORTC, c - 'a', level );
>                       break;
>     case 'g' ... 'l': PORTB = setbit( PORTB, c - 'g', level );
>   }
> }

DANKE, cool, genau so was hatte ich gesucht, da wäre ich noch/nie darf 
gekommen. Das mit dem Auslagern und den "nur" warten auf  "#", werde ich 
auch noch umsetzten.

Und ja es soll am Schluss eine "Art" Protokoll sein.

Gruß
  Bernd

[Draco (Gast)]

Peter D. schrieb:
> void setiobit( char c )
> {
>   uint8_t level = 0;
>   switch( c ){
>     case 'A' ... 'L': level = 1;
>                       c += 'a' - 'A';
>   }
>   switch( c ){
>     case 'a' ... 'f': PORTC = setbit( PORTC, c - 'a', level );
>                       break;
>     case 'g' ... 'l': PORTB = setbit( PORTB, c - 'g', level );
>   }
> }

Peter... kannst du mir das mal aufdrösseln bitte?!

Bei den case ('A'...'L' z.b.) wird da jeder Wert welcher in diesem 
Bereich liegt beachtet? Das hab ich noch nie so gesehen, auch in keinem 
C Buch. Aber wäre ja klasse wenn das so der Fall ist.

[Bernd (Gast)]

Hi,
habe es so mal in ein Shell C Code mit fprint Ausgabe geschrieben. Und 
ja es werden alle Werte einzel verarbeitet.
Der erste switch merke sich nur da "AN" in level.
Im zweiten wird dann nur das entsprechen Bit gesetzt/gelöscht.

echt klasse, nochmals Danke

Gruß

Bernd

[Leo C. (rapid)]

Draco schrieb:
> Bei den case ('A'...'L' z.b.) wird da jeder Wert welcher in diesem
> Bereich liegt beachtet? Das hab ich noch nie so gesehen,

Das ist eine GCC Erweiterung. Also nicht portabel.
https://gcc.gnu.org/onlinedocs/gcc-6.2.0/gcc/Case-Ranges.html#Case-Ranges

> auch in keinem C Buch.

Deshalb.

[Bernd (Gast)]

PS.:

angenommen c = 'B':

> switch( c ){
>>     case 'A' ... 'L': level = 1;
>>                       c += 'a' - 'A';

ist c in der Menge von 'A' bis 'L', ja dann 1. level = 1 //merke war 
groß Buchstabe. 2. c = c + 97 - 65 -->> c = 66 + 97 - 65 somit ist c = 
98 = 'b'

[Peter D. (peda)]

Leo C. schrieb:
> Das ist eine GCC Erweiterung.

Ja, das ist schade. Man damit viele Sachen deutlich besser lesbar 
hinschreiben. Bzw. man stelle sich mal vor, bis zu 65536 cases einzeln 
hinschreiben zu müssen.
Allerdings hat der GCC in der MC-Welt recht große Verbreitung gefunden.

Ich weiß, man kann das auch mit if/else-Ketten machen, aber das ist 
lange nicht so übersichtlich. Schon, daß man bei jedem Vergleich 
überprüfen muß, ob da auch wirklich die selbe Variable steht, macht es 
schwer lesbar. Ein kleiner Vertipper (100* i und einmal j) und man sucht 
sich nen Wolf.
Und außerdem ist es fehlerträchtig, da man versehentlich überlappende 
Bereiche schreiben kann.

[nicht"Gast" (Gast)]

Peter D. schrieb:
> Ich weiß, man kann das auch mit if/else-Ketten machen, aber das ist
> lange nicht so übersichtlich. Schon, daß man bei jedem Vergleich
> überprüfen muß, ob da auch wirklich die selbe Variable steht, macht es
> schwer lesbar. Ein kleiner Vertipper (100* i und einmal j) und man sucht
> sich nen Wolf.
> Und außerdem ist es fehlerträchtig, da man versehentlich überlappende
> Bereiche schreiben kann.

Für diesen Fall ist das aber nicht sooooo schlimm. Da tuts ein:

if(c >= 'A' && c <= 'L') ...

auch.

[Peter D. (peda)]

Tom schrieb:
> Das Hackergetrickse spart ein paar Zeilen, aber erfordert eine
> Dokumentation

Es mag zwar sein, daß die Regel etwas schwerer zu verstehen ist, aber 
bei Copy&Paste-Monstern habe ich immer das Problem, daß sich schnell ein 
Schreibfehler in eine Kopie einschleicht.
Und ich muß auch immer sämtliche Fälle prüfen. Bei der Regel reicht es, 
wenn ich prüfe, daß 'a' und 'L' funktionieren.
Daher bevorzuge ich das Finden einer Regel. Die Regel kann ich dann auch 
viel leichter anpassen, z.B. eine Case-Zeile für PORTD hinzufügen oder 
andere Bitbereiche je Port.

Und anscheinend wurde die Regel ja auch ohne Dokumentation gut 
verstanden.

[Stefan F. (Gast)]

Für die Berechnung der Baudrate kannst du dich alternativ auf die 
Library setbaud.h verlassen 
http://www.nongnu.org/avr-libc/user-manual/group__util__setbaud.html.

Die Taktfrequenz (F_CPU) stellt man überlicherweise im Makefile bzw. in 
den Projekteinstellungen ein, damit nicht nur dein Hauptprogramm sondern 
alle Files die Taktfrequenz kennen. Bei diesem kleinen Programm macht 
das sicher keinen Unterschied, bei komplexeren Projekten aber schon.

[Achim (Gast)]

ich halte den Originalcode für einen Anfänger für sehr gut. Ich möchte 
jedoch auf 2 Punkte bei diesem Konstrukten eingehen:

> case 'A':  PORTC |= ( 1 << PB0 ); break;
> case 'a':  PORTC &= ~( 1 << PB0 ); break;

Punkt 1: Die Bitmanipulation: Zu Beginn völlig OK, doch bald sollte es 
dafür generelle Konstrukte geben, um nicht jedesmal "umzurechnen". Egal 
ob Funktion oder Makro, egal ob BIT als 0..7 oder als 1,2,4,...128 
definiert wird. Wichtig ist mir nur, das die Variable nur einmal in 
einer Anweisung steht. Also z.B.:

#define BIT0 1 
...
#define BIT7 128

#define SET(reg, Bit) ((reg) |=  (Bit))
#define RES(reg, Bit) ((reg) &= ~(Bit))
...
    case 'A':  SET(PORTC,BIT0); break;
    case 'a':  RES(PORTC,BIT0); break;

Also nicht PORTC = SET(PORTC,BIT0);
und auch keine Makros, die den Registernamen zerstückeln, also nicht

#define SetPortBit(port,bit) (PORT##port)|=(Bit))
...
    SetPortBit(C,BIT0);

Punkt 2: Im Original sieht man, welcher Pin zu welchem Buchstaben 
gehört. Das ist gut. "case 'A'..'L'" sind schwieriger Lesbar und nicht 
wartbar. Will man (aufgrund eines Verdrahtungsfehlers) a mit f 
vertauschen, zerfällt der Code zu Spaghetti. Im Urpost vertauscht man 
2*2 Buchstaben und kommentiert das kurz. Der 'A'..'L'-Code ist auch 
nicht effektiver, eher im Gegenteil.

Ich würde eher die copy-paste Anteile in ein Quick-n-dirty-Makro 
packen um den Konfigurationsaspekt herauszustellen:

#define qd(OnCmd, OffCmd, Port, Bit) \
    case OnCmd: SET(Port,1<<Bit); break; 
    case OffCmd: RES(Port,1<<Bit); break; 
...
     qd('A','a',PORTB,PB0);
     qd('B','b',PORTB,PB1);

Diese Zeilen verdeutlichen auch nochmal, wie wichtig eine konsistent 
Interpretation von Bits im Projekt ist (BIT7==7 || BIT7==128??). Die 
Vor- und Nachteile findet jeder selber im Einsatz schnell heraus.

[Rene K. (xdraconix)]

Achim schrieb:
> Ich würde eher die copy-paste Anteile in ein Quick-n-dirty-Makro
> packen um den Konfigurationsaspekt herauszustellen:
> #define qd(OnCmd, OffCmd, Port, Bit) \
>     case OnCmd: SET(Port,1<<Bit); break;
>     case OffCmd: RES(Port,1<<Bit); break;
> ...
>      qd('A','a',PORTB,PB0);
>      qd('B','b',PORTB,PB1);
> Diese Zeilen verdeutlichen auch nochmal, wie wichtig eine konsistent
> Interpretation von Bits im Projekt ist (BIT7==7 || BIT7==128??). Die
> Vor- und Nachteile findet jeder selber im Einsatz schnell heraus.

Hier bin ich raus, mach da bitte mal ein richtiges Beispiel von, würde 
mir das gerne mal anschauen.

[nicht"Gast" (Gast)]

Achim schrieb:
> #define qd(OnCmd, OffCmd, Port, Bit) \
>     case OnCmd: SET(Port,1<<Bit); break;
>     case OffCmd: RES(Port,1<<Bit); break;
> ...
>      qd('A','a',PORTB,PB0);
>      qd('B','b',PORTB,PB1);

Cases hinter Makros verstecken? Das ist keine gute Idee. In einem halben 
Jahr weiß der Programmierer selber nicht mehr, was das soll. Da finde 
ich sogar das Konstrukt vom Arduino Fanboy aus dem anderen Thread noch 
lesbarer:

Arduino F. schrieb:
> state=__LINE__; return; case __LINE__:
>       while(millis()-timestamp<interval) return;


Achim schrieb:
> Will man (aufgrund eines Verdrahtungsfehlers) a mit f
> vertauschen, zerfällt der Code zu Spaghetti.

Das willst du auch nicht in Software ändern. Nachdem das ein 
Master/Slave Konstrukt wird mit mehreren Slaves, willst du keinen 
"besonderen" mit einer angepassten Firmware haben. Lieber die Verdratung 
korrigieren.

Wenn schon hochflexibel (ob das überhaupt nötigh ist wäre eine wichtige 
Frage), dann die Daten in ein Struct Array packen  und das auswerten. 
Wenn man so was über den kürzesten weg machen möchte wird es immer 
unübersichtlich.

Grüße,