[Rumgucker]

Ok... dann noch eine Kritik.

if...else....Anweisungen am Ende einer Routine ("ChargeBattery") sind schlecht, weil soweohl der if-Teil als auch der else-teil zur gemeinsamen Ende-Adresse springen. Aber da steht nur return. Die meisten Compiler raffen esnicht, dass sie ja eigentlich am Ende der if-Klammer gleich return ausführen könnten und nicht erst auf den return springen müssen.

Ich würde sowas in C erzwingen:

if(VChargeInOut>MainsThreshold) {
blabla;
return;
}
EXTon = 0;

Dann kann die else-Anweisung auch gleich ganz wegfallen, weil wenn er in das if reingelaufen ist, dann kommt er sowieso nicht mehr beim else an. Dadurch verkürzt sich das C-Programm und wird lesbarer.

Aber wie gesagt: das geht nur, wenn if...else am Ende einer Prozedur/Funktion stehen.

[voltwide]

jepp, danke!

[Rumgucker]

Noch eine winzige Kritik.

Eine Prozedur/Funktion mit leerer Argumentenklammer gibts eigentlich gar nicht mehr. Viele Compiler warnen dann. Es ist sauber (und folgerichtig), wenn man eine leere Argumentenklammer mit "void" füllt, wenn man denn keine Argumente übergeben will. Das gleiche tust Du ja auch, um Prozeduren (also Funktionen ohne Return) zu deklarieren.

[Rumgucker]

Und noch eine allerletzte winzige Kritik...


"for( ; ; )" ist zwar ok, aber mutig. "for" ist eine sehr mächtige Schleifenangabe. Es kann gut sein, dass der Compiler Anweisungs-, Vergleich- und Manipulationsteil nicht vollständig wegoptimiert.

Sicherer ist "while(1)". Das ist sauber udn besteht selbst für den Compiler unmissverständlich aus einer Endlosschleife.

[voltwide]

Zitat:Original geschrieben von Rumgucker

Noch eine winzige Kritik.

Eine Prozedur/Funktion mit leerer Argumentenklammer gibts eigentlich gar nicht mehr. Viele Compiler warnen dann. Es ist sauber (und folgerichtig), wenn man eine leere Argumentenklammer mit "void" füllt, wenn man denn keine Argumente übergeben will. Das gleiche tust Du ja auch, um Prozeduren (also Funktionen ohne Return) zu deklarieren.

Heißt das auch, dass man die leere Klammer ganz weglassen kann?

[Rumgucker]

Zitat:Original geschrieben von voltwide
Heißt das auch, dass man die leere Klammer ganz weglassen kann?

nein.

Es muss zum beispiel heißen "void prozedurname(void) {....}"

[voltwide]

ok, vielen Dank!

[Rumgucker]

Zitat:Original geschrieben von voltwide
man könnte vielleicht auch diese flags in ein bit-Feld packen, das ist dann schon der Feinschliff.

Genauso mach ich das immer.

Aber das bringt Nachteile, weil der Compiler dann wieder maskieren muss.

Ich mach das aber trotzdem, um deutlich zu machen, dass es nur Bits sind. Dann komm ich gar nicht auf die Idee, da Zahlenwerte reinzuschreiben.

[voltwide]

Zitat:Original geschrieben von Rumgucker

Noch eine winzige Kritik.

Eine Prozedur/Funktion mit leerer Argumentenklammer gibts eigentlich gar nicht mehr. Viele Compiler warnen dann. Es ist sauber (und folgerichtig), wenn man eine leere Argumentenklammer mit "void" füllt, wenn man denn keine Argumente übergeben will. Das gleiche tust Du ja auch, um Prozeduren (also Funktionen ohne Return) zu deklarieren.

Das verhält sich bei mir gerade anders herum.
ich bekomme Fehlermeldungen in dem Moment, wo ich die leeren Klammern mit void ausfülle.
Und zwar unterschiedlich für includete und selbst geschriebene Funktionen.
Schaut man in die zugehörigen header-files, sind dort nur leere Klammernpaare zu finden.
Folgerichtig habe ich nun auch aus meinen eigenen Funktionen die voids aus den Klammern entfernt.
Der compiler (avr-gcc) meckert nicht.
Alles läuft wie bisher.

[Rumgucker]

Wir verstehen uns miss.

Beim Aufruf einer Prozedur/Funktion bleibt die Klammer natürlich leer.

Nur bei der Deklaration gehört ein void rein.

Hier eine Funktionsdeklaration, die gcc fehlerfrei frisst

int main(void) { return(0); }

[voltwide]

Aha, dann ist es klar

[voltwide]

https://stromrichter.org/d-amp/content/i...20120224.c
Hier ist die aktualisierte Version des AVR-codes, im Wesentlichen nur noch Kosmetik.

[Rumgucker]

Kritik?

Du hast immer noch zum Ende von ChargeBattery diese "if-else"-Sache drin. Ich empfahl, die if-Klammer mit einem return abzuschließen und Dir so die else Anweisung zu sparen.

void ChargeBattery (void) {
if (CHARGEirq) { // check battery 2x / 1sec
CHARGEirq = 0; // done
// DC input powering the charger?
DDRB &= ~(ChargeInOut|BatteryIn); // ADC-ports=input
ADMUX = ADMUXdefault |(1<<MUX1); // ADCin=ChargeInOut=pin3=PB4=ADC2, result left adj
ADCSRA = ADCSRAdefault; // start ad-conversion
while (0 == (ADCSRA & (1<<ADIF))); // irq-flag set at eof ad-conversion
ADCSRA |= (1<<ADIF); // clear irq flag
VChargeInOut = ADCH; // bit9..2 , 8bit result
// measure Battery voltage
ADMUX = ADMUXdefault |(1<<MUX1)|(1<<MUX0); // ADCin=BattMoni=pin2=PB3=ADC3
ADCSRA = ADCSRAdefault; // start ad-conversion
while (0 == (ADCSRA & (1<<ADIF))); // irq-flag set at eof ad-conversion
ADCSRA |= (1<<ADIF); // clear irq flag
VBattery = ADCH; // bit9..2 , 8bit result
ADCSRA &= ~(1<<ADEN); // done: ADC=off saves power during sleep
DDRB = DDRBdefault; // restore portmodes
// 2-point hysteretic battery charge
if (VChargeInOut>MainsThreshold) { // external DC-input?
EXTon = 1;
if (VBattery<VChargeMin) {
PORTB &= ~ChargeInOut; // active lo
CHARGEon = 1;
}
if (VBattery>VChargeMa {
PORTB |= ChargeInOut; // passive hi
CHARGEon = 0;
}
return;
}
// else // unnötig
EXTon = 0;
}
}

[Rumgucker]

Statt "#define pin1 (1<<PB5)"

kann man auch schreiben "#define pin1 _BV(PB5)"

---------------------

Bei

if (VBattery<VChargeMin) {
PORTB &= ~ChargeInOut; // active lo
CHARGEon = 1;
}
if (VBattery>VChargeMa {
PORTB |= ChargeInOut; // passive hi
CHARGEon = 0;
}

bin ich mir nicht sicher, ob Du das wirklich willst. Du hast damit künstlich ein verspätetes Schalten erreicht, weil der letzte Portzustand ja so lange erhalten bleibt, bis Du zu hoch oder zu tief geladen hast.

Es wäre IMHO nicht dumm, wenn Du bei Spannungsgleichheit den Port togglen lässt!

BTW: ein extra-Flag brauchst Du nicht. Der Port ist ein gutes Flag-Register!

----------------

Für

PORTB &= ~ChargeInOut;

bzw.

PORTB |= ChargeInOut;

also für das Setzen und Rücksetzen von Portbits würde ich mir Makros machen.

[voltwide]

Zitat:Original geschrieben von Rumgucker

Kritik?

Du hast immer noch zum Ende von ChargeBattery diese "if-else"-Sache drin. Ich empfahl, die if-Klammer mit einem return abzuschließen und Dir so die else Anweisung zu sparen.

void ChargeBattery (void) {
if (CHARGEirq) { // check battery 2x / 1sec
CHARGEirq = 0; // done
// DC input powering the charger?
DDRB &= ~(ChargeInOut|BatteryIn); // ADC-ports=input
ADMUX = ADMUXdefault |(1<<MUX1); // ADCin=ChargeInOut=pin3=PB4=ADC2, result left adj
ADCSRA = ADCSRAdefault; // start ad-conversion
while (0 == (ADCSRA & (1<<ADIF))); // irq-flag set at eof ad-conversion
ADCSRA |= (1<<ADIF); // clear irq flag
VChargeInOut = ADCH; // bit9..2 , 8bit result
// measure Battery voltage
ADMUX = ADMUXdefault |(1<<MUX1)|(1<<MUX0); // ADCin=BattMoni=pin2=PB3=ADC3
ADCSRA = ADCSRAdefault; // start ad-conversion
while (0 == (ADCSRA & (1<<ADIF))); // irq-flag set at eof ad-conversion
ADCSRA |= (1<<ADIF); // clear irq flag
VBattery = ADCH; // bit9..2 , 8bit result
ADCSRA &= ~(1<<ADEN); // done: ADC=off saves power during sleep
DDRB = DDRBdefault; // restore portmodes
// 2-point hysteretic battery charge
if (VChargeInOut>MainsThreshold) { // external DC-input?
EXTon = 1;
if (VBattery<VChargeMin) {
PORTB &= ~ChargeInOut; // active lo
CHARGEon = 1;
}
if (VBattery>VChargeMa {
PORTB |= ChargeInOut; // passive hi
CHARGEon = 0;
}
return;
}
// else // unnötig
EXTon = 0;
}
}

Du meinst also
if (!CHARGEirq) {EXTon = 0; return};
//DC input powering...usw

[voltwide]

Zitat:Original geschrieben von Rumgucker

Statt "#define pin1 (1<<PB5)"

kann man auch schreiben "#define pin1 _BV(PB5)"

Das finde ich nicht besser lesbar

[voltwide]

ich verstehe nicht was Du mit toggeln bei Spannungsgleichheit meinst.
Dies ist ein hysteretischer Ansatz:
VBat < VMin: Lader ein
VBat > VMax: Lader aus
Die Abfrage erfolgt 2/sec.
Das ist schnell genug, so dass ich mir da keine Sorgen mache dass der port zu spät upgedated wird.

Das Flag im portbit zu speichern hatte ich auch erwogen, aber aus rein formalen Erwägungen wieder verworfen. So ist es ein flag wie all die anderen, was ich besser lesbar finde.

Die Codegröße liegt bei 770byte, also muß ich noch nicht knapsen.

[Rumgucker]

Zitat:Original geschrieben von voltwide
ich verstehe nicht was Du mit toggeln bei Spannungsgleichheit meinst.
Dies ist ein hysteretischer Ansatz:
VBat < VMin: Lader ein
VBat > VMax: Lader aus

Bei "VBat == VMin" behälst Du aber den alten Lader-Zustand (on bzw. off). Dadurch schwankt die Batteriespannung unnötig stark hin und her. Du treibst dieses Spiel:

t = 0s: Vbat < Vmin -> Lader ein
t = 2s: Vbat == Vmin -> Lader ein
t = 4s: Vbat > Vmin -> Lader aus
t = 6s: Vbat == Vmin -> Lader aus
t = 8s: Vbat < Vmin -> Lader ein

Ich hatte folgenden Sonderfall für "VBat == VMin" vorgeschlagen:

t = 0s: Vbat < Vmin -> Lader ein
t = 2s: Vbat == Vmin -> Lader aus (weil vorher "ein")
t = 4s: Vbat == Vmin -> Lader ein (weil vorher "aus")
t = 6s: Vbat == Vmin -> Lader aus (weil vorher "ein")
t = 8s: Vbat < Vmin -> Lader ein
t = 10s: Vbat == Vmin -> Lader aus (weil vorher "ein")
t = 12s: Vbat > Vmin -> Lader aus

Ich erreiche damit natürlich auch ne Hysteresis. Die ensteht ja allein schon durch die 2-Sekunden-Abtastung. Aber bei der genauen Ladespannung beträgt meine minimale Ladeperiode 4 Sekunden (2s on, 2s off), während Deine minimale Ladeperiode 8 Sekunden beträgt (2s on, 2s on, 2s off, 2s off).

Du hast also eine unnötig große Ungenauigkeit eingebaut.

[voltwide]

Ich glaube Du hast mich nicht verstanden.
ich schalte den Lader ein wenn Vbatt < Vmin = 13,6V
Ich schalte den Lader aus wenn Vbatt > Vmax = 14,3V

Deine Annahme bei t=4s ist also falsch: Ich schalte nicht wieder ein bei Vbatt > Vmin

[Rumgucker]

Zitat:Original geschrieben von voltwide
Das Flag im portbit zu speichern hatte ich auch erwogen, aber aus rein formalen Erwägungen wieder verworfen.

Eben deswegen solltest Du Dich umentscheiden.

Eigenzitat: "Wenn Du zwei Variablen hast, die das gleiche bedeuten, so tu alles, um mit einer einzigen auszureichen!!!!" ;deal2

Beispiel (ohne Bezug zu Deinem Progranmm): Du hast gerade den Port gesetzt, noch nicht aber Dein extra-Flag. Wenn in dieser Zwischenzeit ein Intrerrupt eintrifft, so hat der ein inkonsistentes/mehrdeutiges System vor sich.

Solche Fehler sind unfassbar schwierig zu finden. Tickende Zeitbomben.