Probleme mit slbstgeschriebener C-Funktion

[Marco G. (grmg2010)]

Moin,

für einen ATMEGA644 ich habe eine eigene Funktion geschrieben. Die 
Funktion soll beim Systemstart überprüfen, ob ein EEPROM hinterlegter 
Wert plausibel ist. Konkreter soll überprüft werden, ob die Ausgänge des 
Device geschaltet haben sollen oder nicht. Dazu ist eine RTC mit 
verbaut.

Wenn das Device nicht geschaltet hat, ist die Auswertung korrekt. Soll 
das Device jedoch an sein, geht er in den Fehlerzustand. Jetzt rage ich 
mich weshalb. Der Code ist bis auf die Vergleiche gleich. Auch stimmen 
die übergebenen Parameter(gebe ich auf einen LCD aus)
Hier mal der Code:

uint8_t CheckSwitchState(uint8_t State, uint8_t TimeOn, uint8_t TimeOff, uint8_t TimeOnWe, uint8_t TimeOffWe)
{
  uint8_t Minute, Hour, ValueOn, ValueOff, WDay;
  char BufferM[10];
  
  Hour = RetHour();
  Minute = RetMinute();
  WDay = RetWDay();
  
  itoa(Hour, BufferM, 10);
  lcd_gotoxy(8,1);
  lcd_puts(BufferM);
  itoa(TimeOn, BufferM, 10);
  lcd_gotoxy(0,1);
  lcd_puts(BufferM);
  itoa(TimeOff, BufferM, 10);
  lcd_gotoxy(5,1);
  lcd_puts(BufferM);

  if (WDay == 6 || WDay == 7)
  {
    /*Device on*/
    if (Hour >= TimeOnWe && Hour < TimeOffWe && State == 1)
    {
      ValueOn = 1;
    }
    else
    {
      ValueOn = 0;
    }
    
    /*Device off*/
    if (Hour < TimeOnWe && Hour > TimeOffWe && State == 0)
    {
      ValueOff = 1;
    }
    else
    {
      ValueOff = 0;
    }
  } 
  else
  {    
    /*Device on*/
    if (Hour >= TimeOn && Hour <= TimeOff && State == 1)
    {
      lcd_gotoxy(0,3);
      lcd_puts("on1");
      ValueOn = 1;
    }
    else
    {
      lcd_gotoxy(0,3);
      lcd_puts("on2");
      ValueOn = 0;
    }
    
    /*Device off*/
    if (Hour <= TimeOn && Hour >= TimeOff && State == 0)
    {
      lcd_gotoxy(10,3);
      lcd_puts("off1");
      ValueOff = 1;
    }
    else
    {
      lcd_gotoxy(10,3);
      lcd_puts("off2");
      ValueOff = 0;
    }
  }
  

  
  if ((ValueOff == 1 && ValueOn == 0) || (ValueOff == 0 && ValueOn == 1))
  {
    return 1;
  } 
  else
  {
    led_bl(0);
    led_rt(0);
    return 0;
  }
  
}

Die Variable State soll hierbei den aus dem EEPROM ausgelesenen 
eigentlichen Status verarbeiten.

[Jonas B. (jibi)]

Moin,

ich geb mal meinen Senf dazu ab:

Funktionsnamen: "CheckSwitchState" Welcher Switch? Welcher Status?
Warum 2 Variablen für einen Zustand? ValueOn, ValueOff

Warum nicht Device.isOpen als Struktur gebaut? Ach da ist viel zu viel 
im argen...

GRuß J

[PittyJ (Gast)]

Anscheinend sind doch alle Werte auch auf dem LCD?
Warum vergleichst du nicht die Werte mit den IF-Entscheidungen?

[Marco G. (grmg2010)]

PittyJ schrieb:
> Anscheinend sind doch alle Werte auch auf dem LCD?
> Warum vergleichst du nicht die Werte mit den IF-Entscheidungen?

Das stimmt, und daher bin ich verwundert dass der Else-Zweig aufgerufen 
wird. Nach den dargestellten werten sollte die Bedingung wahr sein, 
nicht falsch.

Als Beispiel: OnHour ist 14, Hour ist 14, OffHour ist 6 und State ist 1
Dann stünde doch dar:

if (14 >= 14 && 14 <= 6 && 1 == 1)
    {
      lcd_gotoxy(0,3);
      lcd_puts("on1");
      ValueOn = 1;
    }
    else
    {
      lcd_gotoxy(0,3);
      lcd_puts("on2");
      ValueOn = 0;
    }

Edit: Ok das hat es nochmal gebracht es so aufzuschreiben. Da war ein 
Denkfehler drin.Es muss wie folgt lauten

if (Hour >= TimeOn && Hour >= TimeOff && State == 1)
    {
      lcd_gotoxy(0,3);
      lcd_puts("on1");
      ValueOn = 1;
    }
    else
    {
      lcd_gotoxy(0,3);
      lcd_puts("on2");
      ValueOn = 0;
    }

[Walter S. (avatar)]

Marco G. schrieb:
> Soll
> das Device jedoch an sein, geht er in den Fehlerzustand.

was heißt Fehlerzustand?

[Marco G. (grmg2010)]

Die Funktion gibt dann 0 zurück, was in der main dann weiter ausgewertet 
wird. Ich habe den Fehler aber bereits gefunden :) Ein < bzw > Verglich 
war fehlerhaft

[Cyblord -. (cyblord)]

Marco G. schrieb:
> Die Funktion gibt dann 0 zurück, was in der main dann weiter ausgewertet
> wird. Ich habe den Fehler aber bereits gefunden :) Ein < bzw > Verglich
> war fehlerhaft

Der grundlegende Fehler, also sozusagen die Wurzel des Übels, wird ja 
bereits in der Überschrift genannt: Du hast selbst eine C-Funktion 
geschrieben. Lass das einfach in Zukunft.

[wrdlbrmft (Gast)]

Cyblord -. schrieb:
> Marco G. schrieb:
>> Die Funktion gibt dann 0 zurück, was in der main dann weiter ausgewertet
>> wird. Ich habe den Fehler aber bereits gefunden :) Ein < bzw > Verglich
>> war fehlerhaft
>
> Der grundlegende Fehler, also sozusagen die Wurzel des Übels, wird ja
> bereits in der Überschrift genannt: Du hast selbst eine C-Funktion
> geschrieben. Lass das einfach in Zukunft.

Kannst du nicht einfach mal deine unendlich arrogante Fresse halten?

Scheinbar hast du noch nieeee einen Fehler gemacht und mußtest auch noch 
nie was lernen. Denn bekanntermaßen bist du ja mit dem Wissen des 
gesamten Universums geboren worden. Aber lass dir sagen, das ist nicht 
bei jedem so. Es gibt Menschen, da steigt das Wissen erst im Laufe des 
Lebens. Und bei denen ist es so, dass sie aus Fehlern lernen. Das ist 
dir fremd, da du keine Fehler machst. Aber vielleicht kannst du auch mal 
akzeptieren, daß es Menschen gibt, die verschiedene Sachen lernen 
müssen. Und da ist so ein grosskotziger Kommentar wie deiner völlig 
daneben..

[Kuschelpädagoge (Gast)]

Cyblord -. schrieb:
> Der grundlegende Fehler, also sozusagen die Wurzel des Übels, wird ja
> bereits in der Überschrift genannt: Du hast selbst eine C-Funktion
> geschrieben. Lass das einfach in Zukunft.

Ach komm. Bei der ersten selbstgeschriebenen Funktion kann schon mal was 
schief gehen.

[nicht"Gast" (Gast)]

Moin,

ich will auch mal meinen Senf dazu geben :)

du nennst die Funktion CheckSwitchState machst aber tausend andere 
sachen drin. Du solltest das besser aufteilen.


Zähle mal, wie oft du die kombination lcd_goto und lcd_puts drin hast. 
Mach das in eine Funktion, die lcd_puts_at(char* toPrint, uint posX,uint 
posY) heißen kann. Du wirst sehen, das mach den Code deutlich lesbarer.
Eine zusätzliche lcd_puti_at(uint8_t toPrint, uint posX, uint posY) ist 
auch hilfreich.


Benenne die LEDs nach ihrer Funktion, nicht nach ihrer Farbe. Was soll 
denn  led_bl(0) und led_rt(0) bedeuten, und warum werden sie hier 
(vermutlich) ausgeschaltet. Wenn man das wissen will, muss man erst in 
der Funktion suchen und dann auch noch im Schaltplan. Das ist nicht gut.

was passiert eigentlich, wenn die Startzeit größer als die Endzeit ist?

[Marco G. (grmg2010)]

Die LCD-Ausgaben waren nur fürs Debugging. Die sind normal nicht 
enthalten.

Die beiden Zeiten TimeOn und TimeOff werden in der eigentlichen Main aus 
d EEPROM geholt. Sie sind also immer gleich, solange sie dort nicht 
geändert werden.
Stimmt der ausgelesene Status mit dem vorher definierten Zeitrahmen 
überein, wird ein true zurückgegeben, wenn dies nicht der Fall ist, wird 
ein false zurück gegeben. Das wird dann in der main noch 
weiterverarbeitet.