Code in Ordnung/strukturiert/uebersichtlich?

[Dominik G. (moondryl)]

Hallo,
nachdem ich derzeit von Arduino zum direkten Programmieren von µC 
wechsle, habe ich es nach einiger Zeit geschafft, die DS1307/DS3232 zum 
Laufen zu bekommen.

Nun stelle ich mir aber einige Fragen bezüglich des Codes.

(1)Habe ich "vernünftig" programmiert?
(2)Was kann/sollte ich anders machen?
(3)Ist der Code übersichtlich und für andere lesbar?
(4)Habe ich mit dem Auslagern von Code in Funktionen vielleicht etwas 
übertrieben?

Danke schonmal an alle, die sich die Mühe machen, einmal drüber zu 
schauen!

#define F_CPU 16000000UL
#include <avr/io.h>
#include <util/delay.h>
#include <uart.h>
#include <avr/interrupt.h>
#include <i2cmaster.h>

#define UART_BAUD_RATE 9600

#define DS1307  0xD0

#define RTC_MODE 0 // Change to 1 for 12h // Change to 0 for 24h


uint8_t DS1307_read(uint8_t address, uint8_t *data)
{
  i2c_start_wait(DS1307+I2C_WRITE);
  i2c_write(address);          //Register setzen
  
  i2c_rep_start(DS1307+I2C_READ);
  *data = i2c_readNak();        //Byte lesen und ausgeben
  
  i2c_stop();
}

uint8_t DS1307_write(uint8_t address, uint8_t data)
{
  i2c_start_wait(DS1307+I2C_WRITE);
  i2c_write(address);          //Register setzen
  
  i2c_write(data);          //Byte schreiben
  
  i2c_stop();  
}

void DS1307_init()
{
  uint8_t buffer;
    
  //CH Bit gesetzt?
  DS1307_read(0x00, &buffer);
  
  if ((buffer & 0x80)) // Falls CH Bit gesetzt ==> löschen
  {
    DS1307_write(0x00, (buffer & 0x7F));
    uart_puts("CH Bit gelöscht\n");
  }
  
  // Change 12h- 24h-Mode
  DS1307_read(0x02, &buffer);
  if (RTC_MODE == 1){ 
    DS1307_write(0x02, (buffer | 0x40)); // AM/PM ==> 0x40
    uart_puts("DS1307 running in 12h Mode");
    uart_puts("\n");
  }  
  else {
    DS1307_write(0x02, buffer & 0xBF); // 24h ==> 0xBF
    uart_puts("DS1307 running in 24h Mode");
    uart_puts("\n\n");
  }
}

//DS1307 Struktur
struct  {
  
  char seconds[3];
  char minutes[3];
  char hours[3];
  unsigned char day;
  char date[3];
  char month[3];
  char year[3];
  char temperature[10];
  
}ds1307;

void DS1307_readTime()
{
  uint8_t buffer;
  uint8_t frac;
  
  // read & write Seconds
  DS1307_read(0x00, &buffer);
  ds1307.seconds[0] = (( buffer & 0x7F ) >> 4 ) + '0';
  ds1307.seconds[1] = (( buffer & 0x0F )) + '0';
  ds1307.seconds[2] = '\0';
  
  // read & write Minutes
  DS1307_read(0x01, &buffer);
  ds1307.minutes[0] = (( buffer & 0x7F ) >> 4 ) + '0';
  ds1307.minutes[1] = (( buffer & 0x0F )) + '0';
  ds1307.minutes[2] = '\0';
  
  // read & write Hours
  DS1307_read(0x02, &buffer);
  ds1307.hours[0] = (( buffer & 0x3F ) >> 4 ) + '0';
  ds1307.hours[1] = (( buffer & 0x0F )) + '0';
  ds1307.hours[2] = '\0';
  
  // read & write Day
  DS1307_read(0x03, &buffer);
  buffer &= 0x07;
  
  switch (buffer){
    case 1: ds1307.day = "Monday\0";
        break;
    case 2: ds1307.day = "Tuesday\0";
        break;
    case 3: ds1307.day = "Wednesday\0";
        break;
    case 4: ds1307.day = "Thursday\0";
        break;
    case 5: ds1307.day = "Friday\0";
        break;
    case 6: ds1307.day = "Saturday\0";
        break;
    case 7: ds1307.day = "Sunday\0";
  }
  
  // read & write Date
  DS1307_read(0x04, &buffer);
  ds1307.date[0] = (( buffer & 0x3F ) >> 4 ) + '0';
  ds1307.date[1] = (( buffer & 0x0F )) + '0';
  ds1307.date[2] = '\0';
  
  // read & write Month
  DS1307_read(0x05, &buffer);
  ds1307.month[0] = (( buffer & 0x10 ) >> 4 ) + '0';
  ds1307.month[1] = (( buffer & 0x0F )) + '0';
  ds1307.month[2] = '\0';
  
  // read & write Year
  DS1307_read(0x06, &buffer);
  ds1307.year[0] = (( buffer & 0xF0 ) >> 4 ) + '0';
  ds1307.year[1] = (( buffer & 0x0F )) + '0';
  ds1307.year[2] = '\0';  
  
  // read & write Temperature
  DS1307_read(0x11, &buffer);
  if (buffer & 0x80) // negative Temperatur
  {
    buffer = ~buffer;
    buffer++;
    DS1307_read(0x12, &frac);
    frac = ((frac & 0xC0) >> 6) * 25;
    sprintf(ds1307.temperature, "-%dC°", buffer, frac);
  }
  else // positive Temperatur
  {
    buffer &=0x7F;
    DS1307_read(0x12, &frac);
    frac = ((frac & 0xC0) >> 6) * 25;
    sprintf(ds1307.temperature, "+%d.%dC°", buffer, frac);
  }  
}

void DS1307_Output()
{
  DS1307_readTime();
  uart_puts(ds1307.day);
  uart_puts("\n");
  uart_puts(ds1307.date);
  uart_puts(".");
  uart_puts(ds1307.month);
  uart_puts(".");
  uart_puts(ds1307.year);
  uart_puts("\n");
  uart_puts(ds1307.hours);
  uart_puts(":");
  uart_puts(ds1307.minutes);
  uart_puts(":");
  uart_puts(ds1307.seconds);
  uart_puts("\n");
  uart_puts("Temperature:");
  uart_puts(ds1307.temperature);
  uart_puts( "\n\n" );
}

/*********************************
      MAIN SETUP
**********************************/
int main(void)
{
  sei();
  DDRB = (1 << PB0);
  
  uart_init( UART_BAUD_SELECT(UART_BAUD_RATE,F_CPU) ); // uart initialisieren
  i2c_init(); // i2c initialisieren
  DS1307_init(); // DS1307 initialisieren  

  
  /*********************************
        MAIN LOOP
  **********************************/
    while(1)
    {
        PORTB ^= ( 1 << PB0 );  // Toggle PB0 
    
    DS1307_Output();
        _delay_ms(1005); 
    }
}

[Karl H. (kbuchegg)]

> ds1307.day = "Monday\0";


Du möchtest im C Buch deiner Wahl noch mal das (gar nicht so kleine) 
Kapitel über Stringverarbeitung in C durchlesen.

[Karl H. (kbuchegg)]

Diese Struktur

struct  {
  
  char seconds[3];
  char minutes[3];
  char hours[3];
  unsigned char day;
  char date[3];
  char month[3];
  char year[3];
  char temperature[10];
  
}ds1307;

ist natürlich dafür ausgelegt, dass sie für Ausgabezwecke benutzt wird. 
Das kann sinnvoll sein, muss aber nicht unbedingt. (Ist die Jahreszahl 
wirklich nur 2 stellig?). Was mir daran nicht schmeckt ist, dass du als 
'Systemprogrammierer' mir als 'Anwendungsprogrammierer' vorschreibst, 
wie ich die EInzelteile auszugeben habe. Wenn ich keine englischen 
Tagbezeichnungen haben will, dann habe ich damit zu kämpfen, dass du mir 
die aufs Auge gedrückt hast. SO gesehen wir mir ein unsigned char 
(diesmal allerdings wirklich im Sinne einer Zahl und nicht in der 
falschen Stringform wie du ihn benutzt hast) lieber. Denn aus Zahlen von 
0 bis 6 meine italienischen Tagbezeichnungen abzuleiten ist trivial. Aus 
den von dir vorgegebenen Tagtexten das zu tun, ist schon mehr Aufwand. 
Selbiges für die Monate, bzw. alle anderen Dinge. Ich will vielleicht in 
MEINER Ausgabe keine führenden 0-en haben, krieg sie aber von dir in den 
Strings aufs augegedrückt. Um sie loszuwerden muss ich einigen Aufwand 
treiben, den ich nicht machen müsste, wenn du die Tagnummer einfach nur 
als Zahl in der Struktur hättest, die ich nach meinem Gusto so ausgeben 
kann, wie ich das in meiner Anwendung für richtig halte.

Merke: Ein Systemprogrammierer stellt in erster Linie Funktionalität zur 
Verfügung. Dabei sollte er sich nicht davon leiten lassen, was er in 
seiner momentanen Applikation als sinnvoll erachtet, sondern er lässt 
sich davon leiten, was losgelöst von irgendeiner Applikation am meisten 
Sinn macht. Sinn im Sinne von: in einer speziellen Applikation lässt 
sich aus der zur Verfügung gestellten Funktionalität auf einfache Art 
und Weise die dort gebrauchte Funktionalität ableiten.



An der äusseren Form des Codes gibt es erst mal nicht allzuviel zu 
bemängeln. Ein paar Kleinigkeiten vielleicht möglicherweise, aber nichts 
gravierendes. Die wichtigste Forderung 'konsistent sein', ist erfüllt 
und alles weitere schreibt dir dann sowieso dein Arbeitgeber vor.

[Tom K. (ez81)]

uint8_t DS1307_read(uint8_t address, uint8_t *data)
scheint nichts zu returnen, warum nicht
void DS1307_read(uint8_t address, uint8_t *data)
oder
uint8_t DS1307_read(uint8_t address)
?

Achtung,der Rest ist Geschmackssache, ich mag weder globale Variablen 
noch lange Funktionen:

Ich persönlich würde ds1307 nicht global, sondern in main anlegen und 
mit DS1307_readTime(&ds1307) befüllen.

Und statt z.B.

// read & write Seconds
  DS1307_read(0x00, &buffer);
  ds1307.seconds[0] = (( buffer & 0x7F ) >> 4 ) + '0';
  ds1307.seconds[1] = (( buffer & 0x0F )) + '0';
  ds1307.seconds[2] = '\0';

sowas schreiben:

DS1307_read(DS1307_SECONDS, &buffer);
convert_to_seconds(buffer, &ds1307.seconds);

[Würg Jonsch (Gast)]

Also mit Makros kann man das ganze schon noch ein bisschen aufhübschen.

[Würg Jonsch (Gast)]

z.B. das hier:

DS1307_read(0x04, &buffer);
  ds1307.date[0] = (( buffer & 0x3F ) >> 4 ) + '0';
  ds1307.date[1] = (( buffer & 0x0F )) + '0';
  ds1307.date[2] = '\0';

[Dominik G. (moondryl)]

Wow, vielen Dank für eure Mühen!
Ich fange einmal oben an:

>Du möchtest im C Buch deiner Wahl noch mal das (gar nicht so kleine)
>Kapitel über Stringverarbeitung in C durchlesen.

Meinst du ich solle lieber String.h einbinden oder das "\0" am Ende? 
Oder ist die Zuweisung an sich das Problem?

>Ist die Jahreszahl wirklich nur 2 stellig?
Nein, da hast du recht; das muss ich dann noch ändern.

>Was mir daran nicht schmeckt ist, dass du als
>'Systemprogrammierer' mir als 'Anwendungsprogrammierer' vorschreibst,
>wie ich die EInzelteile auszugeben habe. [...]Um sie loszuwerden muss ich 
>einigen Aufwand treiben, den ich nicht machen müsste, wenn du die Tagnummer 
>einfach nur als Zahl in der Struktur hättest, die ich nach meinem Gusto so 
>ausgeben kann, wie ich das in meiner Anwendung für richtig halte.

Wenn ich eine Lib zur Verfügung stellen wollte, kann ich dich sehr gut 
nachvollziehen. Und für weitere Projekte ist es wohl sinnvoll, dass auch 
gleich so umzusetzen.
Ich sollte also alles so allgemein halten wie möglich, und diese Daten 
dann in die Struktur laden. Die Auswertung der Sekunden, Jahre, 
Wochentage etc. erledige ich dann in der while(1)-Schleife. Dort kann 
dann auch jeder die Daten so interpretieren, wie er möchte. Habe ich 
dich richtig verstanden?

>An der äusseren Form des Codes gibt es erst mal nicht allzuviel zu
>bemängeln. Ein paar Kleinigkeiten vielleicht möglicherweise, aber nichts
>gravierendes. Die wichtigste Forderung 'konsistent sein', ist erfüllt
>und alles weitere schreibt dir dann sowieso dein Arbeitgeber vor.

Das es nicht der größte Murks ist, ist schonmal erfreulich zu hören.
Ist aber alles Hobby. ;)

>uint8_t DS1307_read(uint8_t address, uint8_t *data)
>scheint nichts zu returnen, warum nicht
>void DS1307_read(uint8_t address, uint8_t *data)
>oder
>uint8_t DS1307_read(uint8_t address)
>?

Da hast du Recht. Also sollte ich entweder die Funktion als "void" 
bezeichnen oder ich könnte die I2C-Ausgabe zurückgeben, um zu sehen, ob 
das Lesen/Schreiben überhaupt geklappt hat.

>sowas schreiben:
>DS1307_read(DS1307_SECONDS, &buffer);
>convert_to_seconds(buffer, &ds1307.seconds);

Dann hätte ich aber zu jedem Wert nochmal eine Umwandlungsfunktion. Aber 
das ließe sich ja mit einer allgemeinen BCDtoDEC-Funktion umgehen. Dann 
hätte man nur noch eine Umwandlungsfunktion.

>Also mit Makros kann man das ganze schon noch ein bisschen aufhübschen.

Meinst du damit die Bitmasken und Register?
Also z.B.

 #define SECONDS 0x00

Nochmal vielen Dank!

[Patrick D. (oldbug)]

Ein bisschen "Meckern auf hohem Niveau" :-)

#define F_CPU 16000000UL

Das gehört in die Kommandozeile für den avr-gcc. Also irgendwo in Deinem 
Makefile oder den Projekteinstellungen mit -DF_CPU=..., damit das auch 
wirklich alle Sourcefiles mitbekommen!

#define RTC_MODE 0 // Change to 1 for 12h // Change to 0 for 24h

Besser:

#define RTC_FORMAT_12H  1
#define RTC_FORMAT_24H  0

#define RTC_MODE RTC_FORMAT_24H

Das bringt den Code zum Sprechen.
Die Warteschleife wäre dann eins der nächsten Dinge: guck Dir mal ein 
paar Beispiele zu Timern an. Eine gute Library dazu bietet Jörg Wunsch 
auf Seiner Homepage an. "AVR one shot timer" o.ä....

Das ganze macht aber schon einen ordentlichen Eindruck für "Dein erstes 
Projekt" :-)

[Thomas E. (thomase)]

int main(void)
{
  sei();
  DDRB = (1 << PB0);
  
  uart_init( UART_BAUD_SELECT(UART_BAUD_RATE,F_CPU) ); // uart initialisieren
  i2c_init(); // i2c initialisieren
  DS1307_init(); // DS1307 initialisieren  

  
  /*********************************
        MAIN LOOP
  **********************************/
    while(1)
    {
        PORTB ^= ( 1 << PB0 );  // Toggle PB0 
    
    DS1307_Output();
        _delay_ms(1005); 
    }
}

Patrick Dohmen schrieb:
> Ein bisschen "Meckern auf hohem Niveau" :-)

Da schliesse ich mich mal an.
Wobei eine Sache kritisch werden kann. Sinn und Zweck einer globalen 
Interruptfreigabe ist, erst sämtliche Einstellungen vorzunehmen und erst 
wenn alle wissen, wer sie sind, ihnen zu erlauben, das Programm zu 
unterbrechen.
Also sei(); kommt grundsätzlich als letztes.

Da die Funktionsnamen zur Initialisierung selbsterklärend sind, kann man 
sich das Kommentieren sparen.
1 + 1 = 2  // Berechnen von 1 + 1

_delay_ms(1005);
Das ist natürlich völlig Panne. Aber da arbeitest du sicher schon dran.

mfg.

[imonbln (Gast)]

Noch ein wenig Jammern auf Hohen Nivau:

Dominik Gebhardt schrieb:
> if (RTC_MODE == 1){
>     DS1307_write(0x02, (buffer | 0x40)); // AM/PM ==> 0x40
>     uart_puts("DS1307 running in 12h Mode");
>     uart_puts("\n");
>   }
>   else {
>     DS1307_write(0x02, buffer & 0xBF); // 24h ==> 0xBF
>     uart_puts("DS1307 running in 24h Mode");
>     uart_puts("\n\n");
>   }

Das if ist Komisch, RTC_MODE ist ein Makro, welches immer 0 oder 1 ist.
d.h hier sagst du den Kompiler er soll zwei Konstante werte 
Vergeleichen.
In der Praxis wird er dieses if weg Optimieren, da beide teile des 
vergleich Konstant sind. Nur du wirst dich wundern warum dein assembler 
Code an der stelle "Komisch" aussieht und kein vergleich macht.
Besser wäre wenn du an der stelle das präprozessor if nutz.
Dann sieht der Kompiler immer nur den eine von beiden möglichkeiten und 
andere Entwickler erkennen auch das es hier um Optionen geht welche zum 
zeitpunkt der Kompilieren gesetz werden.

#if RTC_MODE == 1
  DS1307_write(0x02, (buffer | 0x40)); // AM/PM ==> 0x40
  uart_puts("DS1307 running in 12h Mode");
  uart_puts("\n")
#else
  DS1307_write(0x02, buffer & 0xBF); // 24h ==> 0xBF
  uart_puts("DS1307 running in 24h Mode");
  uart_puts("\n\n");
#endif

[Mikel M. (mikelm)]

imonbln schrieb:
> #if RTC_MODE == 1
>   DS1307_write(0x02, (buffer | 0x40)); // AM/PM ==> 0x40
>   uart_puts("DS1307 running in 12h Mode");
>   uart_puts("\n")
> #else
>   DS1307_write(0x02, buffer & 0xBF); // 24h ==> 0xBF
>   uart_puts("DS1307 running in 24h Mode");
>   uart_puts("\n\n");
> #endif
>
>

Wenn schon auf 0 abfragen, um die philsophie !=0 ist true beizubehalten
funktionieren tut zwar beides.
 Konsequent wäre auf 0 uns 1 abzufragen und wenn was anderes auftritt 
mir Fehler anzubrechen.

#if RTC_MODE == 0
  DS1307_write(0x02, buffer & 0xBF); // 24h ==> 0xBF
  uart_puts("DS1307 running in 24h Mode");
  uart_puts("\n\n");
#else
  DS1307_write(0x02, (buffer | 0x40)); // AM/PM ==> 0x40
  uart_puts("DS1307 running in 12h Mode");
  uart_puts("\n")
#endif

Generell finde ich den Code gerade mal ausreichend für nen Anfänger evt 
noch befriedigend.
 Es fehlen jede Menge Kommentare, das fängt am Anfang an, es steht weder 
das was das für ein Programm ist, noch was es macht, es fehlt der Autor, 
es fehlt de Historie.
 Auch sollte jede Funktion optisch leicht erkennbar sein, finden ist die 
Devise und nicht suchen.
 Ein struct mitten im Code zu definieren geht für mich gar nicht. Auch 
gehört hierhinter jeder Zeile ein Kommentar zu dem Parameter. z.B.
  char temperature[10]; Was kommt da rein? Da muß man erst den Source 
durchsuchen um das rauszufinden. Auch bei den anderen das sie mit '/0' 
abgeschlossen werden müssen fehlt an der Stelle.
 Bei den kleinen Code kann man sich das zwar schnell zusammensuchen, 
aber oft wächst er und wird damit dann unübersichtlich. Spätestens dann 
freut man sich wenn man weis was da reingehört.

>  DS1307_read(0x01, &buffer);
 statt 0x01 baut man sich ein sprechendes "define", das verhindert 
Fehler. und erleichtern die Portabilität und erhöht die Lesbarkeit.

[Steffen (Gast)]

Hallo Dominik,

ich finde den Quellcode gut lesbar. Ein gutes Stück besser als der 
Durchschnitt, was ich sonst (auch, oder gerade im professionellen 
Bereich) gesehen habe. Verbessern kann man immer was... auch bei 
Superprogrammierung findet man meist immer noch was. Bei Dir könnte man 
bspw. die lokalen Daten besser kapseln: Gib DS1307_init() einen 
Parameter mit, und lass den Client antscheiden, ob er lieber 
RTC_FORMAT_12H/24H verwendet.

Grüße, Steffen

[Zeiten (Gast)]

struct  {

  char seconds[3];
  char minutes[3];
  char hours[3];
  unsigned char day;
  char date[3];
  char month[3];
  char year[3];
  char temperature[10];

}ds1307;

Ein time_t (in GMT) ist IMHO besser zu handhaben als diese Struktur, auf 
einen kleinen uc muss das aber nicht unbedingt stimmen.

Schau dir mal die Funktionen asctime() und ctime() an.

[Mikel M. (mikelm)]

Dominik Gebhardt schrieb:
> uint8_t DS1307_read(uint8_t address, uint8_t *data)
> {
>   i2c_start_wait(DS1307+I2C_WRITE);
>   i2c_write(address);          //Register setzen
>
>   i2c_rep_start(DS1307+I2C_READ);
>   *data = i2c_readNak();        //Byte lesen und ausgeben
>
>   i2c_stop();
> }

Was hier noch hingehört sind 2 Sachen:
1) da die Funktion nicht void ist, muß sie auf jeden Fall einen Wert 
zurückliefern.
2) muß data auf NULL angefragt werden.

also etwa so:
/*
** DS1308 über I2C auslesen
** Adress muß im Bereich 0..x sein
** *pData enthält gelesenen Wert wenn gültig
*/
uint8_t DS1307_read(uint8_t Address, uint8_t *pData)
{uint8_t tmp;
  i2c_start_wait(DS1307+I2C_WRITE);
// hier evt noch ne Abfrage ob die adresse gültig ist
  i2c_write(Address);             //Register setzen

  i2c_rep_start(DS1307+I2C_READ);
  tmp= i2c_readNak();                 //Byte lesen
  if ( pData ) *pData = i2c_readNak();
  i2c_stop();
  return tmp;
}