Stoppuhr Code Optimierung

[Franny (Gast)]

Hallo alle miteinander!

Ich habe für meinen ATMega8 ein kleines Programm für eine Stoppuhr 
geschrieben. Hierbei wird mit einem Taster die Stoppuhr gestartet und 
auf vier 7-Segment Anzeigen ausgegeben. Bei erneutem Drücken des Tasters 
wird die Uhr gestoppt. Drückt man jetzt erneut auf den Taster wird die 
Stoppuhr zurückgesetzt und erneut gestartet.
Das Programm funktioniert auch recht gut.
Allerdings bin ich noch recht neu in der Programmierung von uC und 
dementsprechend sieht auch mein Code aus.
Habt ihr vielleicht Tipps, ob ich hier ein paar Sachen zusammenfassen 
bzw. effizienter schreiben könnte?
Und wenn ja, wie?

Schonmal vielen Dank im Voraus!
Franny

#define F_CPU 8000000
#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>

uint16_t damn [10] = {(0x44), (0x7D), (0xA4), (0x34), (0x19), (0x16), (0x06), (0x7C), (0x04), (0x14)};
uint8_t counts[4];

volatile uint16_t var;
volatile uint16_t value;
volatile uint16_t digit;
volatile uint16_t x;
volatile uint16_t x1;
volatile uint16_t x2;
volatile uint16_t x3;

int main(void)
{
  DDRB |= (1<<PB0);    //Datenrichtungsregister
  DDRC |= 0x1E;
  DDRD |= 0xFB;
  
  MCUCR |= (1<<ISC01);  //externer Interrupt
  GICR |= (1<<INT0);
  
  TCCR0 |= (1<<CS00) | (1<<CS01);    //Timer 0
  TIMSK |= (1<<TOIE0);
  TCCR1B |= (1<<CS10);  //Timer 1
  TIMSK |= (1<<TOIE1);
  
  sei();    //Freigabe der Interrupts
  
  PORTC |= 0x1C;
  PORTD |= ~0xFB;
  
  while (1)
  {
  }
}

ISR(TIMER1_OVF_vect)
{  
  if (x2 == 1)
  {
    x1++;
  }
  if (x2 == 0)
  {
    x1 = 0;
  }
  
  if (x1 == 15/6)        //Für annähernden Sekundentakt
  {
    x3++;
    x1=0;
  }
  
  if (x3>9999)
  {
    x3=0;
  }
  
  value = x3;
    
  counts [3] = 0;          //Alles wieder auf 0 setzen
  counts [2] = 0;
  counts [1] = 0;
  counts [0] = 0;
    
  while(value >= 1000)      //Wertzerlegung
  {
    counts[0]++;
    value = value - 1000;
  }
    
  while(value >= 100)
  {
    counts[1]++;
    value = value - 100;
  }
    
  while(value >= 10)
  {
    counts[2]++;
    value = value - 10;
  }
    
  counts[3] = value;
}

ISR(TIMER0_OVF_vect)        //Weitergabe über die Transistoren
{
  var++;
  
  if (var > 1)
  {
    var = 0;
    digit++;
    
    if (digit > 4)
    {
      digit = 1;
    }
  }
  
  PORTC |= 0x1E;
  PORTC &= ~(1 << digit);
  PORTD = damn[counts[digit - 1]];
}

ISR(INT0_vect)
{
  PORTB ^= (1<<PB0);    //Debug LED
  x2++;
  if (x2 == 2)
  {
    x2 = 0;
  }
  if (x2 == 1)
  {
    x3=0;
  }
}

[Walter T. (nicolas)]

Franny schrieb:
> Habt ihr vielleicht Tipps, ob ich hier ein paar Sachen zusammenfassen
> bzw. effizienter schreiben könnte?

Was stört Dich denn an Deinem Quelltext? Ich sehe jetzt keine schlimmen 
Antipattern für so ein kurzes Programm, außer dass die 
Programmbeschreibung (Kommentar) am Anfang fehlt.

Normalerweise pausiert man bei einer Stoppuhr mit einem kurzen 
Tastendruck. Mit einem weiteren kurzen Tastendruck kann weiterlaufen 
gelassen werden.

Zurückgesetzt wird mit einem langen Tastendruck oder mit einer extra 
Rücksetztaste.

[Sebastian R. (sebastian_r569)]

Eigentlich sollte in den Interrupts so wenig wie möglich passieren, 
dafür mehr in der Main-Loop. Aber gut.

Für deine Wertezerlegung: Schau dir mal an, wie der Modulo-Operator (%) 
funktioniert. Der könnte sehr nützlich für dich sein.
Und bei Integern gilt: 7821/1000 = 7

Damit solltest du die While-Schleifen wegoptimieren können und das ganze 
schneller und übersichtlicher hinbekommen.

Noch ein Tipp zur Dokumentation:
Schreibe noch dazu, wie die einzelnen Timer konfiguriert sind und was 
sie machen.

  // Timer 2 konfigurieren
  GTCCR |= (1 << TSM) | (1 << PSRASY);  //Timer anhalten, Prescaler Reset
  ASSR |= (1 << AS2);                   //Asynchron Mode einschalten
  TCCR2A = (1 << WGM21);                //CTC Modus
  TCCR2B |= (1 << CS22) | (1 << CS21);  //Prescaler 256
  // 32768 / 256 / 1 = 128                Intervall = 1s
  OCR2A = 128 - 1;
  TIMSK2 |= (1<<OCIE2A);                //Enable Compare Interrupt
  GTCCR &= ~(1 << TSM);                 //Timer starten
  sei();

Hierbei kann man sofort sehen, was man sich bei der Konfiguration der 
Timer gedacht hat und vor allem, dass das Interrrupt jede Sekunde 
aufgerufen wird.

Nach einem halben Jahr wirst du bei deinem Code nicht mehr wissen, wann 
welches Timerinterrupt aufgerufen wird.

[Walter T. (nicolas)]

Nachtrag: Und ich verstehe nicht, warum zwei ISRs nötig sind. Was die 
ISRs jeweils machen, sollte in einem Kommentar verewigt werden. Was 
eigentlich für alle Funktionen gilt.

[Teo (Gast)]

Franny schrieb:
> Das Programm funktioniert auch recht gut.
> Allerdings bin ich noch recht neu in der Programmierung von uC und
> dementsprechend sieht auch mein Code aus.

Ah, dir sind also bereits ein paar Baustellen bekannt!
Da gibts nix zu optimieren, fange einfach noch mal von vorne an... und 
such dir geeignete Literatur!

[Walter T. (nicolas)]

Teo schrieb:
> und such dir geeignete Literatur!

Achja. Die klassischen Nullaussagen.

[MaWin (Gast)]

Das ist schon mal besser als das was man oft hier sieht.
Nur die Tastenauswertung per Interrupt wird durch prellende Tasten 
gestört. Schreib die um.

https://dse-faq.elektronik-kompendium.de/dse-faq.htm#F.29.1

Ich selbst würde weniger in die Interruptroutinen tun  und dafür den 
Stoppzeitzähler und Digitaufbereitungscode in die while(1) 
Hauptschleife, im Interrupt nur die Timerticks zählen, und käme mit 1 
Timer für multiplexing und tick zählen aus.

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

Franny schrieb:
> Bei erneutem Drücken des Tasters wird die Uhr gestoppt.
Du hast den Taster im Ernst einfach so an einen Interruptpin 
angeschlossen? Dann solltest du dir die Sache mit dem "Prellen" von 
mechanischen Schaltern mal genauer anschauen.

> Habt ihr vielleicht Tipps, ob ich hier ein paar Sachen zusammenfassen
> bzw. effizienter schreiben könnte?
Ich würde 90% des ganzen Klimbims in der Mainloop machen. Und in der 
Timer-ISR nur einen ms-Timer (oder 10ms oder 50ms) hochzählen und darauf 
reagieren. Im selben Timer könnte dann auch die Multiplexerei 
abgehandelt werden. Und auch das Entprellen des Tasters.

[Stefan F. (Gast)]

Mir ist der Dokumentations-Stil besonders aufgefallen.

Ich empfehle sprechende Namen für Variablen und Funktionen. Der Name 
soll anzeigen, was die Variable bedeutet. Du hast viel Platz, niemand 
wird sich für Abkürzungen bedanken.

Was ist "damn"? Das heißt auf deutsch "verdammt"! Was ist verdammt?

Ganz oben sollte stehen, um welchen µC es geht.

// Für einen ATMega8 mit 8MHz Quarz

Deine Kommentare sollten nicht erklären was du tust (denn das ist 
offensichtlich, sondern warum. DDR sagt mir schon, dass du die 
Datenrichtung einstellt. Aber ich kann nicht erkennen, welchem Zweck die 
Pins dienen. Wenn sie etwas an/aus schalten, dann gebe an was da
dran hängt und, welcher Pegel an oder aus ist. Manchmal bedeutet ja auch 
LOW=an.

DDRC |= 0x1E; // Transistoren für die digits der 7-Segment Anzeige, HIGH=an

Für die Interrupt-Konfiguration:

// Interrupt bei fallender Flanke an INT0 (= PD2, Start-Taster, LOW=gedrückt)
MCUCR |= (1<<ISC01);
GICR |= (1<<INT0);

Für den Timer:

// Timer 0 dient dem Multiplexing der digits von der 7-Segment Anzeige
TCCR0 |= (1<<CS00) | (1<<CS01);  // Prescaler 64 = 125 kHz Takt
TIMSK |= (1<<TOIE0);             // Erlaube Timer Interrupts
// Der Timer 0 teilt die Takte durch 256 -> ruft die ISR mit ca. 488 Hz auf

Gleiches gilt für die anderen Timer.

Ich habe keine Ahnung, ob meine Kommentare richtig sind. Das ist alles 
nur geraten. Genau dieses Raten sollte nicht nötig sein.

> if (x1 == 15/6)      // Für annähernden Sekundentakt

Wenn du Integer Zahlen dividierst, kommt eine Integer Zahl ohne 
Nachkommastellen heraus. Das ergibt in deinem Fall einen großen 
Rundungsfehler. 15/6 ist 2 (nicht 2.5).

> // Wertzerlegung

Welcher Wert wird in was zerlegt und wofür ist das gut? Schreibe es hin!

Ich sehe viele unnötige Leerzeilen, die den Lesefluss behindern. Füge 
Leerzeilen nur zwischen Blöcken ein, die zusammen gehören. Aber nicht 
zwischen einzelnen Zeilen und Klammern.

[fop (Gast)]

Sieht doch schon ganz gut aus für den Anfang.

Franny schrieb:
>

if (x1 == 15/6)        //Für annähernden Sekundentakt

Hier lauert nur ein Haken. Die Rechnung wird komplett 
nachkommastellenfrei ausgeführt. X1 wird also mit 2 verglichen. Macht 
-20% Abweichung.

[fop (Gast)]

Und zum Beginn des Programms kannst Du ruhig alle Bits der 
Datenrichtungsregister überschreiben, anstatt nur die von Dir benötigten 
zu ändern. Ist für den µC sogar minimal weniger Arbeit.

[H.Joachim S. (crazyhorse)]

Franny schrieb:
> uint16_t damn [10] = {(0x44), (0x7D), (0xA4), (0x34), (0x19), (0x16),
> (0x06), (0x7C), (0x04), (0x14)};

Das soll wohl der Siebensegmentcode sein? Warum 16bit? Und warum 
überhaupt ne Variable? Hol sie direkt aus dem flash, denn da liegen sie 
sowieso und müssen nicht im RAM sein. Und lesbarer schreiben.

Warum läuft digit von 1..4? Der Rest der Welt nimmt normalerweise 0..3.
Aber für den Anfang gar nicht so schlecht.

[Wolfgang (Gast)]

Stefan ⛄ F. schrieb:
> Ich sehe viele unnötige Leerzeilen, die den Lesefluss behindern. Füge
> Leerzeilen nur zwischen Blöcken ein, die zusammen gehören. Aber nicht
> zwischen einzelnen Zeilen und Klammern.

Das sieht mehr nach einem Problem der Forensoftware aus, sobald die 
eingefügte Zeilennummer 3-stellig wird.

Ob

Franny schrieb:

 if (x2 == 0)
   {
     x1 = 0;
   }

nicht übersichtlicher als

if (x2 == 0) x1 = 0;

oder

if (x2 == 0)
  x1 = 0;

geschrieben werden kann, steht auf einem anderen Blatt.

[HildeK (Gast)]

Wolfgang schrieb:
> Das sieht mehr nach einem Problem der Forensoftware aus, sobald die
> eingefügte Zeilennummer 3-stellig wird.

Scheint so zu sein. Glücklicherweise nicht, wenn man den File anhängt 
und ihn über die Codeansicht betrachtet. Das wäre bei einer 
dreistelligen Zeilenzahl auch anzuraten. Da werden dann nur die 
Zeilennummern auf zwei Stellen abgeschnitten. Was ist schon perfekt? 😉