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!
1
#define F_CPU 16000000UL
2
#include<avr/io.h>
3
#include<util/delay.h>
4
#include<uart.h>
5
#include<avr/interrupt.h>
6
#include<i2cmaster.h>
7
8
#define UART_BAUD_RATE 9600
9
10
#define DS1307 0xD0
11
12
#define RTC_MODE 0 // Change to 1 for 12h // Change to 0 for 24h
13
14
15
uint8_tDS1307_read(uint8_taddress,uint8_t*data)
16
{
17
i2c_start_wait(DS1307+I2C_WRITE);
18
i2c_write(address);//Register setzen
19
20
i2c_rep_start(DS1307+I2C_READ);
21
*data=i2c_readNak();//Byte lesen und ausgeben
22
23
i2c_stop();
24
}
25
26
uint8_tDS1307_write(uint8_taddress,uint8_tdata)
27
{
28
i2c_start_wait(DS1307+I2C_WRITE);
29
i2c_write(address);//Register setzen
30
31
i2c_write(data);//Byte schreiben
32
33
i2c_stop();
34
}
35
36
voidDS1307_init()
37
{
38
uint8_tbuffer;
39
40
//CH Bit gesetzt?
41
DS1307_read(0x00,&buffer);
42
43
if((buffer&0x80))// Falls CH Bit gesetzt ==> löschen
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.
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.
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.
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!
1
#define RTC_MODE 0 // Change to 1 for 12h // Change to 0 for 24h
Besser:
1
#define RTC_FORMAT_12H 1
2
#define RTC_FORMAT_24H 0
3
4
#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" :-)
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.
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
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.
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
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.
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;
}