Bitte um Prüfung meines C-Codes

[feg_zeven (Gast)]

Hallo Leute. :)

Ich habe mir ein kleines Programm zusammen gebastelt was einen 
Drehencoder ausliest und eine Variable hoch- bzw runterzählt. Diese 
Variable wird sekündlich über USART an meinen Rechner geschickt - was 
auch funktioniert. Leider tut sich beim drehen am Encoder nichts. Die 
Variable bleibt bei 0 ^^

Dadurch, dass ich über USB programmiere sind meine DebugMöglichkeiten 
nicht so das gelbe vom Ei. Ich konnte feststellen, dass er in die ISR 
des Timer E geht, in dem die Abfrage des Encoders stattfindet.  Es 
scheint als stimme die Abfrage der Pins nicht. Bin mir aber nicht 
sicher. Könnte vielleicht mal jemand ein Auge auf den Code werfen?

Viele Grüße

/*
 *
 * USARTC0, Tx Pin -> PC3.
 * 9600 baud mit 2 MHz clock. BSCALE = 0
 * BSEL = ( 32000000 / (2^0 * 16*19200)) -1 = 12
 * Fbaud = 32000000 / (2^0 * 16 * (12+1))  = 9615 bits/sec
 */ 


#define PHASE_A     (PORTD_IN & 0x01)
#define PHASE_B     (PORTD_IN & 0x02)


#include <avr/io.h>
#include <avr/interrupt.h>
#include <string.h>
#include <asf.h>
#include <stdio.h>

void Clock_init(void);
void Int_init(void);
void UART_init(void);

void TimerC0_init(void);
void TimerE0_init(void);

void Send_UART(unsigned int data);
void impulsgeber_init (void);
int8_t encode_read1(void);

//Variablen
char lenght = 0x00;
char Counter = 0x00;

unsigned int TCTW=0x85ED;      // TimerXTopWert
unsigned int TETW=0x7D00;

unsigned int new = 0;
volatile int8_t enc_delta;          // -128 ... 127
static int8_t last;
int8_t encval = 0;


int main(void)
{
  Clock_init();  
  Int_init();
  UART_init();
  
  TimerC0_init();
  TimerE0_init();

  
  
  PORTB.DIR = 0x00;      //Datenrichtung auf Eingang
  PORTD.DIR = 0x00;  
  TCC0.PER = TCTW;  
  TCE0.PER = TETW;
  
    while(1)
    {
    
  encval=encode_read1();
  
    }
}


//____________INITFunktionen_______________

void Clock_init(void)
{  
  OSC.CTRL |= OSC_RC32MEN_bm;                                              // Oszillator auf 32Mhz stellen
  while(!(OSC.STATUS & OSC_RC32MEN_bm));                                        // Warten bis der Oszillator bereit ist
  CCP = CCP_IOREG_gc;
  CLK.CTRL = CLK_SCLKSEL_RC32M_gc;                                          // Clock auf 32MHz stellen                                              
}

void Int_init(void)
{
  PMIC.CTRL |= PMIC_LOLVLEN_bm | PMIC_MEDLVLEN_bm | PMIC_HILVLEN_bm;                          // Interrupts (Highlevel, Mediumlevel und Lowlevel freigeben)
  sei();                                                        // Globale Interruptfreigabe
}

void UART_init(void)
{
  PORTC.DIR = 0xEF;  
  
  USARTC0.BAUDCTRLB = 0;                                                // BSCALE = 0 
  USARTC0.BAUDCTRLA = 0x67;                                                // Baudrate 19200 @ 41MHz
  USARTC0.CTRLB = USART_TXEN_bm | USART_RXEN_bm;                                    // RX+TX Enable CLK
  USARTC0.CTRLC = 0x03;                                                  // Async, no parity, 8 bit data, 1 stop bit
  USARTC0.CTRLA = 0;    
}

void impulsgeber_init (void)
{
  if( PHASE_A )
  new = 3;
  if( PHASE_B )
  new ^= 1;            // new = new^1 ---- convert gray to binary
  last = new;                   // power on state
  enc_delta = 0;
}


//_______________________TIMER________________________

void TimerC0_init()
{
  TCC0.CTRLA = TC_CLKSEL_DIV1024_gc;                                          // Vorteiler einstellen
  TCC0.CTRLB = 0x00;                                                  // Timer in Normalmodus stellen
  TCC0.INTCTRLA = 0x03;                                                // Interrupt konfigurieren  
}

void TimerE0_init(void)
{
  TCE0.CTRLA = TC_CLKSEL_DIV2_gc;                                          // Vorteiler einstellen
  TCE0.CTRLB = 0x00;                                                  // Timer in Normalmodus stellen
  TCE0.INTCTRLA = 0x02;
}


//______________________TimerISR_______________________

ISR(TCC0_OVF_vect)
{

  Send_UART(encval);
  
  TCC0.PER = TCTW;

}

ISR(TCE0_OVF_vect)
{
    int8_t new, diff;
        new = 0;
    if( PHASE_A )
    new = 3;
    if( PHASE_B )
    new ^= 1;                   // convert gray to binary
    diff = last - new;                // difference last - new
    if( diff & 1 ){               // bit 0 = value (1)
      last = new;                 // store new as next last
      enc_delta += (diff & 2) - 1;        // bit 1 = direction (+/-)
    }
    
   TCE0.PER=TETW;

}


//_______________Sonstige Funktionen__________________

void Send_UART(unsigned int data)
{
  while (!(USARTC0.STATUS & USART_DREIF_bm));
  USARTC0.DATA = data;
}


int8_t encode_read1( void )         // read single step encoders
{
  int8_t val;
  
  cli();
  val = enc_delta;
  enc_delta = 0;
  sei();
  return val;                   // counts since last call
}

[feg_zeven (Gast)]

Ach und ich nutze eine XMega128A1 auf dem Xplained-Board.

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Dieser "atomare" Zugriff ist unnötig, weil enc_delta eh' nur 8 Bit hat 
und sowieso unterbrechungsfrei ausgelesen wird:

  cli();
  val = enc_delta;
  enc_delta = 0;
  sei();

feg_zeven schrieb:
> Leider tut sich beim drehen am Encoder nichts.
Wie sieht die Hardware aus? Funktioniert die? Hast du Pulse am uC-Pin?
Im Code werden keine Pullups am PortC eingeschlatet. Hast du externe 
Pullups?

[Karl H. (kbuchegg)]

feg_zeven schrieb:

> des Timer E geht, in dem die Abfrage des Encoders stattfindet.  Es
> scheint als stimme die Abfrage der Pins nicht. Bin mir aber nicht
> sicher.

Na, ja
Wenn du das denkst, was hindert dich daran, das mal in einem eigenen 
Testprogramm zu verifzieren?

...

int main(void)
{
  PORTD.DIR = 0x00;  

  while( 1 ) {

    lass dir PORTD_IN wo ausgeben
  }
}

du musst ja nicht gleich in die vollen gehen. Genau dieses (Port 
Initialisieren und PORTD_IN) abfragen ist ja auch der Kern der Encoder 
Auswertung. Wenn du da ein Problem vermutest, dann nimm dir diese 
Kernfunktionalität getrennt vor.

Zb kommt mir komisch vor, dass in dem Code die Wörter Pullup bzw. 
Pulldown nirgends vorkommen.

(Das Testprogramm wirst du wahrscheinlich noch um eine UART ergänzen, 
damit du auch irgendwo Output generieren kannst)

[Christian K. (the_kirsch)]

feg_zeven schrieb:
> ISR(TCE0_OVF_vect)
> {
>     int8_t new, diff;
>         new = 0;
>     if( PHASE_A )
>     new = 3;
>     if( PHASE_B )
>     new ^= 1;                   // convert gray to binary
>     diff = last - new;                // difference last - new
>     if( diff & 1 ){               // bit 0 = value (1)
>       last = new;                 // store new as next last
>       enc_delta += (diff & 2) - 1;        // bit 1 = direction (+/-)
>     }
>
>    TCE0.PER=TETW;
>
> }

Ich verstehe gerade nicht warum du den Drehencoder in einer Timer_ISR 
bearbeitest, wäre es nicht besser das per PinChange-Interrupt zu machen?

Du fragst Regelmäßig die Pins ab, du must diese aber auch mit dem 
letzten Wert der Pins vergleichen.

Und zwar so:

00 => 01 => +1
00 => 10 => -1

01 => 11 => +1
01 => 00 => -1

10 => 00 => +1
10 => 11 => -1

11 => 10 => +1
11 => 01 => -1

Alle andern Übergänge sind invalid, da wurde dann ein Schritt nicht 
erkannt

Entweder du pollst das weiterhin in einer TimerISR und besser über einen 
pinInterrupt.

[Falk B. (falk)]

@ Lothar Miller (lkmiller) (Moderator) Benutzerseite

>Dieser "atomare" Zugriff ist unnötig,

Nein!

>weil enc_delta eh' nur 8 Bit hat
>und sowieso unterbrechungsfrei ausgelesen wird:

Das reicht nicht. Es muss auch unterbrechungsfrei gelöscht werden, damit 
nicht zufällig ein Interrupt GENAU zwischen Auslesen und Löschen 
reinspuckt und dadurch 1 Zählerstand verloren geht! Das ist ein SEHR 
schwer lokalisierbarer Fehler! Siehe

https://www.mikrocontroller.net/articles/Interrupt#Atomarer_Datenzugriff

[Falk B. (falk)]

@ Christian K. (the_kirsch)

>Ich verstehe gerade nicht warum du den Drehencoder in einer Timer_ISR
>bearbeitest,

Weil das der normale, bewährte Weg ist.

> wäre es nicht besser das per PinChange-Interrupt zu machen?

Nein. Und nein, das will ich nicht wieder endlos diskutieren.

>Du fragst Regelmäßig die Pins ab, du must diese aber auch mit dem
>letzten Wert der Pins vergleichen.

Das macht die Routine schon, steht alles im Artikel Drehgeber, von 
dort stammt auch der Code!

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Falk Brunner schrieb:
> Das reicht nicht. Es muss auch unterbrechungsfrei gelöscht werden
Ach schlags kaputt. Das ist ja ein Read-Modify-Write...
Gut. Das Zeug muss weiterhin drin bleiben.

Christian K. schrieb:
> wäre es nicht besser das per PinChange-Interrupt zu machen?
Ich stimme Falk zu: wäre es nicht.
Wozu unnötig viele Interrupts? Wenn man aus jedem "Furz" einen Interrupt 
macht, dann kanns durchaus mal hecktisch werden.
Das siehst du sicher an dir selber auch: wenn jede Minute einer kommt 
und dich unterbricht, ist das lästig. Wenn dann mal gleichzeitig sogar 2 
oder 3 kommen, dann kann es schon mal passieren, dass du was 
durcheinander bringst.
Ich spreche jetzt dem uC keine Nachlässigkeit oder Vergesslichkeit zu, 
aber das Hauptproblem ist meist, dass dein Programm (das tadellos läuft, 
solange alle Interrupts hübsch nacheinander kommen) auf einmal 
"abstürzt", wenn 3 Interrupts auf einmal anstehen. Und das ganz einfach, 
weil du solche Unterbrechungen nicht einkalkuliert hast...

[feg_zeven (Gast)]

Also der Encoder funktioniert eigentlich. Hab den letztes Jahr (klingt 
komisch, aber ist ja erst 9 Tage her :D) mal mit einem Oszi geprüft und 
der macht was er soll.
Zum Thema PullUp: Eigentlich habe ich zwei PullUp am Encoder an den 
beiden Kanälen angelötet. Auf dem Foto könnt ihr euch das mal angucken. 
Das schwarz/braune Twistet Pair sind die Signalleitungen. Das zweite 
braune ist +, weis ist GND. Muss ich denn dann trotzdem noch für die 
Pins PullUps/-downs konfigurieren?

[Falk B. (falk)]

@ feg_zeven (Gast)

>komisch, aber ist ja erst 9 Tage her :D) mal mit einem Oszi geprüft und
>der macht was er soll.

Gut.

>braune ist +, weis ist GND. Muss ich denn dann trotzdem noch für die
>Pins PullUps/-downs konfigurieren?

Nein.

Zu deinem Programm.


>#define PHASE_A     (PORTD_IN & 0x01)
>#define PHASE_B     (PORTD_IN & 0x02)

Ist dein Encoder WIRKLICH dort angeschlossen?

>  encval=encode_read1();

Das ist eher nicht das, was du willst, eher so.

encval += encode_read1();

Kleiner, aber feiner Unterschied ;-)


>  CCP = CCP_IOREG_gc;
>  CLK.CTRL = >CLK_SCLKSEL_RC32M_gc;                                          // 
Clock >auf 32MHz stellen

Das geht bisweilen schief, wenn die Optimierung nicht eingeschaltet ist. 
Besser mit einem ASM-Makro arbeiten, das ist sicher. Kann man als 
einfache Funktion nutzen.

Beitrag "Re: PLL Startprobleme beim ATXmega128A1U"

>void impulsgeber_init (void)

Wir bei dir beim Start nicht aufgerufen, ist aber unkritisch.

>ISR(TCC0_OVF_vect)
>{
>  Send_UART(encval);
>  TCC0.PER = TCTW;

Das braucht man nicht, der Timer wird per Hardware automatich 
nachgeladen.
Im Gegenteil, durch dein Nachladen stimmt das Timing nicht mehr exakt.


>   TCE0.PER=TETW;

Hier ebenso.

Funktioniert denn der Rest? Kannst du per UART was senden bzw. auf dem 
PC empfangen?

[feg_zeven (Gast)]

Ich denke schon, dass ich alles richtig angeschlossen habe. Pin0 und 
Pin1 des PortD, oder?

Ok, dann werde ich mal die von dir gefundenen Fehler ausmerzen. Das mit 
dem encval+=.... hätte mir selbst auffallen müssen. Danke.

Der Rest funktioniert eigentlich problemlos. Der Wert wird sekündlich 
gesendet bzw am PC empfangen. Bis jetzt ist es halt nur 0x00. Wie schon 
gesagt - die Zwei Timer-ISR scheint er zu zyklisch zu durchlaufen. Wenn 
ich in die ISR des Encoders die SendUart-Funktion aufrufen lasse bekomme 
ich auch alle paar ms einen Wert gesendet.

Ich probiere erstmal die Hinweise von dir aus und danach erstatte ich 
nochmal bericht :)

[Falk B. (falk)]

ein

volatile int8_t encval = 0;

wäre auch nicht schlecht.

[feg_zeven (Gast)]

Es geht. Danke nochmal an euch :)