Mein erstes Projekt

[Michael R. (mray)]

Ich habe die Programierung meines ersten Projektes(fürs Erste) 
abgeschlossen. Kurz zum Projekt: Es ist ein Logger für unsere 
Katzenklappe, der aufzeichnet, wann die Katze das Haus verlässt bzw. 
betritt. Die Betätigung der Klappe wird von einem Reed-Kontakt 
registriert.
Des weiteren läuft eine Uhr mit und ich habe zwei Taster für das Setup 
(Einstellung der Uhrzeit uns Status der Katze (drin oder draußen)) oder 
für den Abruf der geloggten Ereignisse. Zur Zeit werden 20 Ereignisse in 
einem Array gespeichert, das werde ich noch auf eine Liste umstellen.
Ich habe die Schaltung auf einem Steckbrett aufgebaut und es 
funktioniert alles, jetzt beginne ich mit dem Löten auf eine Platine
Ich habe den Quellcode ( außer lcd-rotines.c und lcd-routines.h) 
beigefügt und bitte einige alte Hasen, mal über den Code zu schauen, und 
mir Anregungen für Verbesserungen zu geben

Tia

MRay

[Wastl F. (rescuetigerchen)]

Hi,
ist die Katzenklappe an einen Transponder gekoppelt? Sonst ist "Zicke on 
tour" und du hast aber trotzdem Besuch im Haus. Was ist mit Wind? Kann 
der die Klappe aufmachen? Und wie ist es mit "Katze guckt raus, will 
dann aber doch lieber an deiner Lieblingspflanze drinnen knabbern"? 
Ansonsten eine feine Sache, wenn das Mistvieh gerade wieder mal nicht 
auffindbar ist. Dann weiß man wenigstens, daß man nicht suchen braucht, 
wenn "Zicke on tour" ist.
Gruß,
wastl

[Code (Gast)]

Michael Rochel schrieb:
> mal über den Code zu schauen,

volatile unsigned int millisekunden;
volatile unsigned int sekunde;
volatile unsigned int minute;
volatile unsigned int stunde;
volatile unsigned int AktLog;
volatile unsigned int Data[20][4];
volatile unsigned int SecsFromStart;
volatile unsigned int LastAction;
volatile unsigned int LastLog;
volatile unsigned int CatState;

Mit Vorgabewerten initialisieren...

[Karl H. (kbuchegg)]

Datentypen!

Auf einem µC willst du fast nie einfach nur 'unsigned int', sondern du 
willst ein bischen bessere Kontrolle über die Bitanzahl.

Gerade ein AVR tut sich bei 8-Bit Arithmetik um einiges leichter, als 
wie wenn du ihn auf 16 Bit oder gar noch höherern Bitzahlen fuhrwerkeln 
lässt.

Das ist jetzt in deiner Anwendung sicherlich nicht das große Problem, 
aber es geht ums Prinzip.

Sekunden, Minuten, Stunden, das alles kann problemlos in 8 Bit 
untergebracht werden. Ein uint8_t reicht daher locker aus und es muss 
kein unsigned int sein.

[Michael R. (mray)]

Hallo,

nein an einen Transponder ist die Klappe nicht gekoppelt. Es ist 
natürlich eine Fehlermöglichkeit, bisher hatten wir aber nie eine fremde 
Katze im Haus.
Der Wind schafft es nicht, die Klappe zu öffnen.
Wenn die Katze nur rausguckt ist das ein Problem. Ich habe auch schon 
darüber nachgedacht, mir ist noch keine Lösung eingefallen. Ich habe an 
zwei ReedkontaktE gedacht, dann kann ich registrieren, ob die Klappe 
weit geöffnet wurde, aber das Kann natürlich auch beim Rausgucken 
passieren. Für Anregungen bin ich hier dankbar.
Erwähnen muss ich noch, dass ich auch elektronisch ein Anfänger bin, 
also elektrojnisch komplizierte Lösungen werde ich nicht umsetzen 
können.

[Karl H. (kbuchegg)]

Da du höchst wahrscheinlich genug Flash hast, so dass du dir ein bischen 
Komfort gönnen kannst.

Deine ShowTime Funktion lässt sich so schreiben

void ShowTime(unsigned int showstunde, unsigned int showminute, unsigned int showsekunde, unsigned int ShowState)
{
  char OutString[30]; 

  lcd_clear();

  if (ShowState == 0)
    lcd_string( CAT_IN );
  else
    lcd_string( CAT_OUT );

  sprintf( OutString, "%02u:%02u:02u", showstunde, showminute, showsekunde );

  lcd_setcursor( 0, 2 );
  lcd_string( OutString );
}

(Dein { - } Klammerungsschema find ich unübersichtlich. Aber man muss 
dir zu Gute halten, dass du es konsequent durchgezogen hast)

[Karl H. (kbuchegg)]

Karl Heinz Buchegger schrieb:

> (Dein { - } Klammerungsschema find ich unübersichtlich. Aber man muss
> dir zu Gute halten, dass du es konsequent durchgezogen hast)


Ooops. Ich nehms zurück. Also die Sache mit dem konsequent. Das sind 
einige Patzer in der Einrückung, die sich auch mit der ewigen 
Tabulator-Leerzeichen Problematik nicht erklären lassen.

[Karl H. (kbuchegg)]

Seh ich das richtig, dass du keine Tastenentprellung eingebaut hast?
Da wirst du in ein paar Monaten dann so richtig deine Freude an den 
Tasten haben :-)

[Michael R. (mray)]

Hallo Karl Heinz,

mit der sprintf-Funktion habe ich mich noch nicht beschäftigt, Deinen 
Vorschlag werde ich jedoch umsetzen, er macht das ganze deutlich 
übersichtlicher.
Klammerungsschema: Meinst Du so:

if(...)
{
  command1;
  command2;
}

statt
if(...)
  {
  command1;
  command2;
  }

[Karl H. (kbuchegg)]

Konsequent sein!

  while(PINC & 1<<3)
    {
    }
    while (!(PINC & 1<<3));

Wieso hat das erste while eine völlig andere Form als das 2-te?

Das ist jetzt zugegebenermassen auch ein wenig Ansichtsache, aber ist 
dir bewusst, dass in

    while (!(PINC & 1<<3));

der ; technisch gesehen die vom while abhängige Anweisung darstellt? Es 
handelt sich im die leere Anweisung, die nur aus einem ; besteht. Alle 
Compound-Statements, zu denen auch ein while gehört, werden NICHT durch 
ein ; abgeschlossen!

Wenn man daher konsequent ist und konsequent durchzieht, dass die 
abhängige Anweisung bei einem while (oder bei einem if) immer in der 
nächsten Zeile steht, dann lautet die sich daraus ergebende Schreibweise

    while (!(PINC & 1<<3))
      ;

sie unterscheidet sich im Grund in der Schreibweise durch nichts von

    while (!(PINC & 1<<3))
      i++;

nur eben, dass es sich um eine leere Anweisung anstelle von i++ handelt.

Ich ziehe diese Schreibweise auch aus dem Grund vor, weil hier absolut 
klar ist, dass ich bewusst eine leere Anweisung benutze und der ; am 
Zeilenende nicht einfach nur ein Tippfehler war.

[Karl H. (kbuchegg)]

Michael Rochel schrieb:
> Hallo Karl Heinz,
>
> mit der sprintf-Funktion habe ich mich noch nicht beschäftigt,

dann solltest du das tun. Mit den Formatierungszeichen kann man sich 
sehr oft das Leben enorm erleichtern. sprintf kostet zwar etwas an Code 
bze- Laufzeit, wenn das aber keine Rolle spielt muss ich mich ja nicht 
unnötig plagen.

> Klammerungsschema: Meinst Du so:
>
> if(...)
> {
>   command1;
>   command2;
> }
>
> statt
> if(...)
>   {
>   command1;
>   command2;
>   }

Find ich übersichtlicher, weil die Klammern da besser rausstehen. Ich 
kenne zwar deine KLammereinrückung aus der Literatur, ich hab aber noch 
nie einen Programmierer getroffen, der sie auch tatsächlich verwendet.

[Michael R. (mray)]

@ Karl-Heinz

nein Tastenentprellung habe ich nicht drin. Ich hatte gehofft, da ich 
nach jedem Tastendruck warte, bis er gelöst ist, würde das reichen. Auf 
meinem Steckbrett hat es soweit funktioniert. Ist es so, dass die 
Probleme erst nach einiger Zeit, wenn die Taster älter sind, auftauchen?

[Karl H. (kbuchegg)]

Michael Rochel schrieb:
> @ Karl-Heinz
>
> nein Tastenentprellung habe ich nicht drin. Ich hatte gehofft, da ich
> nach jedem Tastendruck warte, bis er gelöst ist, würde das reichen.

Tastenprellen spielt sich im Millisekundenbereich ab. Deine Schleifen 
können das so nicht ausgleichen. Die werden viel zu schnell 
abgearbeitet.

> Auf
> meinem Steckbrett hat es soweit funktioniert. Ist es so, dass die
> Probleme erst nach einiger Zeit, wenn die Taster älter sind, auftauchen?

Genau. Tastenprellen entsteht, wenn die Kontakte die aufeinander 
gedrückt werden, abgenutzt sind bzw. die einstmals strammen 
Kontaktfedern anfangen auszuleiern.

[Karl H. (kbuchegg)]

      if (CatState == 0)
          {
          CatState = 1;
          }
    else
          {
      CatState = 0;
      }

kleiner Trick.
Der Sinn dieser Sequenz ist es CatState von 0 auf 1 bzw. umgekehrt zu 
schalten.

   CatState = 1 - CatState;

macht genau das gleiche.

[Karl H. (kbuchegg)]

      if (minute == 60) {minute = 0;}

Autsch. Böses Faul gegen die Formatierrichtlinien.

[Karl H. (kbuchegg)]

Das ist jetzt "Jammern auf hohem Niveau"

ISR (TIMER2_COMP_vect)
{
  millisekunden++;
  
  if(millisekunden == 1000)
  {
    sekunde++;
  SecsFromStart++;
    millisekunden = 0;
  
    if(sekunde == 60)
    {
      minute++;
      sekunde = 0;
    }
    if(minute == 60)
    {
      stunde++;
      minute = 0;
    }
    if(stunde == 24)
    {
      stunde = 0;
    }
  }
}

Ob die Minuten 60 erreicht haben oder nicht, brauchst du nur testen, 
wenn sich die Minuten auch tatsächlich erhöht haben

ISR (TIMER2_COMP_vect)
{
  millisekunden++;
  
  if(millisekunden == 1000)
  {
    millisekunden = 0;
    SecsFromStart++;
    sekunde++;

    if(sekunde == 60)
    {
      sekunde = 0;
      minute++;

      if(minute == 60)
      {
        minute = 0;
        stunde++;

        if(stunde == 24)
          stunde = 0;
      }
    }
  }
}

(Interessant, dass du hier ein anderes { } Einrückungsschema benutzt 
hast :-))

[Code (Gast)]

Karl Heinz Buchegger schrieb:
> sekunde++;
> SecsFromStart++;
> millisekunden = 0;

Dann weisen wir doch auch gleich noch auf die inkonsequenten Wahl der 
Bezeichnernamen hin. ;)

[Karl H. (kbuchegg)]

    LastLog = SecsFromStart;
    Data[AktLog][0] = stunde;
    Data[AktLog][1] = minute;
    Data[AktLog][2] = sekunde;
  Data[AktLog][3] = CatState;
    AktLog++;
    }

Hoppla.
Wer prüft, ob das Array droht überzulaufen?
Das ist kein Schönheitsfehler mehr, über den man diskutieren kann. Das 
ist ein echter Showstopper.

[Karl H. (kbuchegg)]

OK.
Möglicherweise bist du noch nicht soweit. Aber ein paar Strukturen 
hätten dem Programm gut getan.

zum einen gibt es da Zeiten in Form von Stunden Minuten Sekunden

struct Zeit
{
  uint8_t Stunde;
  uint8_t Minute;
  uint8_t Sekunde;
};

zum anderen gibt es das Ereigniss, charakterisiert durch die Art des 
Ereignisses (Katze geht/kommt) und den zugehörigen Zeitpunkt

#define CAT_NO_DATA 2
#define CAT_IS_OUT  1
#define CAT_IS_IN   0

struct Aktion
{
  uint8_t     Typ;    // CAT_NO_DATA, CAT_IS_OUT oder CAT_IS_IN
  struct Zeit Wann;
};

und es gibt ein Array in dem Ereignisse gespeichert werden

#define NR_LOG_DATA  20
struct Aktion LogData[NR_LOG_DATA];

alles zusammen:

const char *CAT_IN = "Zicke ist drin";
const char *CAT_OUT = "Zicke auf Tour";

struct Zeit
{
  uint8_t Stunde;
  uint8_t Minute;
  uint8_t Sekunde;
};

#define CAT_IS_OUT  1
#define CAT_IS_IN   0

struct Aktion
{
  uint8_t     Typ;    // CAT_OUT oder CAT_IN
  struct Zeit Wann;
};

#define NR_LOG_DATA  20
struct Aktion LogData[NR_LOG_DATA];
uint8_t       actLogNr;

volatile unsigned int millisekunden;
volatile struct Zeit actZeit;

void ShowTime( struct Zeit* zeit )
{
  char OutString[10]; 

  sprintf( OutString, "%02u:%02u:02u", zeit->Stunde, zeit->Minute, zeit->Sekunde );
  lcd_string( OutString );
}

void ShowAktion( struct Aktion* aktion )
{
  char OutString[30]; 

  lcd_clear();

  if (ShowState == CAT_IS_IN)
    lcd_string( CAT_IN );
  else
    lcd_string( CAT_OUT );

  lcd_setcursor( 0, 2 );
  ShowZeit( &(aktion->Wann) );
}
...

Das ist zwar hier am Anfang des Programms erst mal etwas mehr Aufwand, 
dafür wird dann aber ab ungefähr dieser Stelle der restliche Code 
einfacher, besser zu lesen und auch oft kürzer. zb

die Klappe wurde betätigt, ein neuer Log-Eintrag wird angelegt

   ...

     LogData[actLogNr].Typ = CatState;
     LogData[actLogNr].Wann = actZeit;

     actLogNr++;
     if( actLogNr == NR_LOG_DATA )
       actLogNr = 0;

Das Log soll gelöscht werden:

  // LogArray löschen
  for( i = 0; i < NR_LOG_DATA; i++ )
    LogData[i].Typ = CAT_NO_DATA;
  actLogNr = 0;

es wird ausgegeben:

  for( i = 0; i < NR_LOG_DATA, i++ ) {
    if( LogData[i].Typ != CAT_NO_DATA )
      ShowAktion( &LogData[i] );
  }

[Michael R. (mray)]

@ Karl-Heinz

Vielen Dank für dDein umfassendes Feedback. Im Detail muss ich es mir 
heute Abend noch ansehen.

Die inkonsistente Klammerung und Variablenbezeichnungen sind enthalten, 
weil ich teilweise den Quelltext aus den Tutorien hier übernommen habe 
(z. B. die Interrupt-Routine).
Das Problem mit dem Array-Überlauf ist mit klar, ich stell das noch auf 
eine Liste um.
Mit den Strukturen ist natürlich auch richtig, aber ich habe seit ca. 20 
Jahren nicht mehr inb C programmiert, hier habe ich noch Nachholbedarf.

[Karl H. (kbuchegg)]

Michael Rochel schrieb:

> Das Problem mit dem Array-Überlauf ist mit klar, ich stell das noch auf
> eine Liste um.

Was versprichst du dir davon?
Der zur Verfügung stehende Speicher wird dadurch auch nicht mehr. 
Eigentlich wird er weniger, weil du mehr Verwaltungsdaten hast.

[Michael R. (mray)]

Dafür aber ohne Beschränkung der Anzahl der Logs (außer dem Speicher). 
Zusätzlich benötige ich doch nur einen Pointer pro Log und einige 
Funktionen zu Listenverwaltung. Ich bin da eher ein Fan von dynamischen 
Strukturen, meinst Du, bei Mikroprozessoren ist es besser ein Array zu 
nehmen und die Logs zu beschränken? (Dann natürlich mit Prüfung auf 
Überlauf)

[Karl H. (kbuchegg)]

Michael Rochel schrieb:
> Dafür aber ohne Beschränkung der Anzahl der Logs (außer dem Speicher).

Ähm. Das ist ein Scheinargument.
Denn du kennst deine Speichergröße. Du weißt auch wieviel Speicher du 
für statische Variablen benötigst und den Stackverbrauch bzw. Verbrauch 
für lokale Variablen kann man abschätzen.

Daraus kannst du errechnen wie groß dein Array werden darf. Mehr geht 
nicht. Denn mehr passt ganz einfach nicht in den Speicher.

> Zusätzlich benötige ich doch nur einen Pointer pro Log und einige
> Funktionen zu Listenverwaltung. Ich bin da eher ein Fan von dynamischen
> Strukturen,

Oh, ich auch.
Auf einem PC.

Auf einem kleinen µC macht es keinen Sinn von den vorhandenen 1K SRAM 
auch nur 5% für Verwaltungsdaten wie zb Verpointerungen von Listenknoten 
zu verbrauchen.
Zumindest solange, solange man nicht den Speicher dynamisch zur Laufzeit 
auf unterschiedliche Datentypen verteilen muss. Den Fall hast du aber 
nicht: Du hast nur einen Typ von Daten. Alle Log-Einträge sind gleich 
aufgebaut. Und du weißt wieviel SRAM dir zur Verfügung steht. Mehr 
Speicher hast du nicht.

Und da sind jetzt die Probleme, die durch Fragmentierung entstehen noch 
gar nicht mit eingerechnet. Denn es ist ein Trugschluss, wenn du denkst, 
dass du durch dynamische Allokierung auch noch das letzte Byte aus dem 
SRAM ausnutzen kannst.

[Jörg W. (dl8dtl) (Moderator)]

Karl Heinz Buchegger schrieb:
> Und da sind jetzt die Probleme, die durch Fragmentierung entstehen noch
> gar nicht mit eingerechnet.

Bei einer Liste mit nur einem Datentyp gibt es (zumindest statistisch)
keine Fragmentierungsprobleme, da die alten, verworfenen Einträge ja
garantiert immer wieder "recyclet" werden können.

Der wunde Punkt beider Lösungen ist die Abschätzung des Stackverbrauchs.
Bei der statischen Variante muss man die komplett selbst machen, bei
der dynamischen unterstützt einen malloc() zumindest rudimentär, indem
es die Anforderung ablehnt, wenn der Heap bereits bis "dicht unter"
dem Stack angelangt ist.  Trotzdem kann man natürlich auch hier
pathologische Fälle finden, in denen dieser Test nicht ausreichen
würde (aus ebendiesem Grunde ist das Maß für "dicht unter" auch zur
Laufzeit konfigurierbar).

[Karl H. (kbuchegg)]

Jörg Wunsch schrieb:

> würde (aus ebendiesem Grunde ist das Maß für "dicht unter" auch zur
> Laufzeit konfigurierbar).

Apropos:
Das hab ich mich nämlich auch gerade gefragt.
Wie konfiguriert man das? Weißt du das zufällig auswendig?

[Karl H. (kbuchegg)]

Michael Rochel schrieb:

> Zusätzlich benötige ich doch nur einen Pointer pro Log

Es ist mehr.
Denn malloc muss ja auch noch Buch führen, welchen Speicher es schon 
rausgerückt hat.

[Karl H. (kbuchegg)]

Karl Heinz Buchegger schrieb:
> Jörg Wunsch schrieb:
>
>> würde (aus ebendiesem Grunde ist das Maß für "dicht unter" auch zur
>> Laufzeit konfigurierbar).
>
> Apropos:
> Das hab ich mich nämlich auch gerade gefragt.
> Wie konfiguriert man das? Weißt du das zufällig auswendig?

Habs gefunden.
globale Variablen __malloc_heap_start, __malloc_heap_end

[Jörg W. (dl8dtl) (Moderator)]

Karl Heinz Buchegger schrieb:
> Wie konfiguriert man das? Weißt du das zufällig auswendig?

Nö, ich muss da auch in der Doku nachgucken, obwohl ich sie selbst
geschrieben habe. ;-)

http://www.nongnu.org/avr-libc/user-manual/malloc.html

``The default value of __malloc_margin is set to 32.''

Karl Heinz Buchegger schrieb:
> Denn malloc muss ja auch noch Buch führen, welchen Speicher es schon
> rausgerückt hat.

Dafür braucht es pro Block weitere 2 Byte, die dem zum Nutzer
rausgerückten Block vorangestellt werden.  Darin wird die Länge des
Blocks aufgezeichnet.  Außerdem gibt es noch global einen Zeiger, der
das aktuelle obere Ende des Heaps kennzeichnet und einen, der den
Anfang der "freelist" markiert.  Welche Blöcke gerade vergeben sind,
merkt sich malloc nicht selbst, das muss sich schon die Applikation
merken. ;-)  Wenn man mit free etwas zurückgibt, wird die Verkettung
für die freelist in dem Bereich hinterlegt, der vorher dem Nutzer zur
Verfügung gestellt worden war.  (Wenn man also auf einen bereits
freigegebenen Block nachträglich zugreift, liest man nicht mehr die
gewünschten Daten, und wenn man drauf schreibt, zerrammelt man die
freelist.)