Anfänger sucht Fehler in vereinfachtem C-Code

[R. B. (britzi)]

Hallo zusammen,

ich möchte meinen C-Code so einfach gestalten, wie möglich, so dass ich 
nur noch Stichwörter für Aktionen nutzen muss.

In folgendem Beispiel funktioniert alles, bis auf den Taster.
Ich habe schon verschiedene Funktionen getestet und weiß nicht, warum er 
nicht funktioniert.

Was ist falsch an meinem Code bzw. wie kann ich ihn korrigieren?


Vielen Dank für die Hilfe :-)

#include <avr/io.h>

#define LED1 PB0
#define KEY1 PINB2 

#define LED1_BIT (1<<LED1)
#define KEY1_BIT (1<<KEY1) 

#define LED1_on() PORTB |= LED1_BIT
#define LED1_off() PORTB &= ~LED1_BIT
#define LED1_toggle() PORTB ^= LED1_BIT

#define Test_Key1() (PINB &= KEY1_BIT)
#define Test_Key1_0() (PINB &= ~KEY1_BIT)


int main(void)
{ 
  DDRB = (1 << DDB0) | (1 << DDB1); 
  PORTB |= (1<<PB2)| (1<<PB3) ;    
while (1)
{  
  if (Test_Key1_0()) { 
    LED1_off();} 
  else{
    LED1_on(); 
}
  }
return 0;
}

[Lamy (Gast)]

PINB &= ~KEY1_BIT => PINB = PINB  & (~KEY1_BIT)

ist eine Zuweisung und keine Abfrage.

[Lamy (Gast)]

#define Test_Key1_0() (PINB & (~KEY1_BIT))

[Stefan E. (sternst)]

Lamy schrieb:
> #define Test_Key1_0() (PINB & (~KEY1_BIT))

Nö, auch nicht. Das testet jetzt, ob irgendein Bit, ausgenommen 
KEY1_BIT, 1 ist.

[Roger (Gast)]

R. B. schrieb:
> ich möchte meinen C-Code so einfach gestalten, wie möglich, so dass ich
> nur noch Stichwörter für Aktionen nutzen muss.

Halte ich für völlig unsinnig. Aber wenn du es gerne ganz einfach hast 
dann pack doch einfach deinen gesamten Code in ein Define.

[mar IO (Gast)]

R. B. schrieb:
> #define LED1_BIT (1<<LED1)
> #define KEY1_BIT (1<<KEY1)
>
> #define LED1_on() PORTB |= LED1_BIT
> ...
> #define Test_Key1_0() (PINB &= ~KEY1_BIT)

Wie Taster und LED in einem???

[Peter II (Gast)]

mar IO schrieb:
> Wie Taster und LED in einem???

beleuchteter Taster eventuell.

[Stefan E. (sternst)]

mar IO schrieb:
> R. B. schrieb:
>> #define LED1_BIT (1<<LED1)
>> #define KEY1_BIT (1<<KEY1)
>>
>> #define LED1_on() PORTB |= LED1_BIT
>> ...
>> #define Test_Key1_0() (PINB &= ~KEY1_BIT)
>
> Wie Taster und LED in einem???

Wieso "in einem"?

#define LED1 PB0
#define KEY1 PINB2

[Lamy (Gast)]

#define Test_Key1() (PINB & KEY1_BIT)

Testen auf 1:
 => Erg muss dann != 0

Testen auf 0:
 => Erg muss dann  = 0

[R. B. (britzi)]

Ich habe den Fehler erkannt.
Ihr habt recht, die Zuweisung ist natürlich Blödsinn.

Dies ist mein richtiger Erstkontakt mit AVRs und in C bin ich auch 
Einsteiger.
Ich komme gerade leider nicht auf die richtige Lösung.



Roger schrieb:
> R. B. schrieb:
>> ich möchte meinen C-Code so einfach gestalten, wie möglich, so dass ich
>> nur noch Stichwörter für Aktionen nutzen muss.
>
> Halte ich für völlig unsinnig. Aber wenn du es gerne ganz einfach hast
> dann pack doch einfach deinen gesamten Code in ein Define.

Wieso ist es unsinnig, wenn man die Lesbarkeit deutlich erhöht?

[mar IO (Gast)]

Stefan Ernst schrieb:
> Wieso "in einem"?
> #define LED1 PB0
> #define KEY1 PINB2

Stimmt, da bin ich durcheinander gekommen. Ich hab's zwar gesehen, aber 
iwie. ignoriert. Da stand halt bei beiden eine 1 hinten...

[R. B. (britzi)]

Lamy schrieb:
> #define Test_Key1() (PINB & KEY1_BIT)
>
> Testen auf 1:
>  => Erg muss dann != 0
>
> Testen auf 0:
>  => Erg muss dann  = 0



Vielen Dank für die Lösung, es funktioniert :-)

[Roger (Gast)]

R. B. schrieb:
> Wieso ist es unsinnig, wenn man die Lesbarkeit deutlich erhöht?

Die kannst du genauso mit entsprechend benannten Funktionen erhöhen.

[R. B. (britzi)]

Roger schrieb:
> R. B. schrieb:
>> Wieso ist es unsinnig, wenn man die Lesbarkeit deutlich erhöht?
>
> Die kannst du genauso mit entsprechend benannten Funktionen erhöhen.


Ja ok, ich denke du hast Recht.
Aber ist es nicht gehopst wie gesprungen?

[Dr. Sommer (Gast)]

Wenn du C Code vereinfachen/eleganter machen willst - wirf C weg und 
nimm C++, damit geht das noch viel besser. Geeignete Implementation 
angenommen, z.B. so etwa:

Pin led1Pin (1 /* 1 = Port B */, 0);
Pin key1Pin (1 /* 1 = Port B */, 2);

int main () {
  led1Pin.setDirection (Direction::OUTPUT);
  key1Pin.setDirection (Direction::INPUT);

while (1)
{  
  led1Pin.setOutput (!key1Pin.getInput ());
  }
return 0;
}

[Peter II (Gast)]

Dr. Sommer schrieb:
> Wenn du C Code vereinfachen/eleganter machen willst - wirf C weg und
> nimm C++, damit geht das noch viel besser. Geeignete Implementation
> angenommen, z.B. so etwa:Pin led1Pin (1 /* 1 = Port B */, 0);
> Pin key1Pin (1 /* 1 = Port B */, 2);
>
> int main () {
>   led1Pin.setDirection (Direction::OUTPUT);
>   key1Pin.setDirection (Direction::INPUT);
>
> while (1)
> {
>   led1Pin.setOutput (!key1Pin.getInput ());
>   }
> return 0;
> }

und in C ist es ja so viel anders

struct Pin led1Pin = {1 /* 1 = Port B */, 0}; 
struct Pin key1Pin = {1 /* 1 = Port B */, 2}; 

int main () { 
  setDirection(led1Pin, DIR_OUTPUT); 
  setDirection(key1Pin, DIR_INPUT); 

  while (1) 
  { 
    setOutput(led1Pin, !getInput(key1Pin) ); 
  } 
return 0; 
}

[R. B. (britzi)]

Dr. Sommer schrieb:
> Wenn du C Code vereinfachen/eleganter machen willst - wirf C weg und
> nimm C++, damit geht das noch viel besser.[/c]


Hatte ich auch überlegt, aber dann müsste ich noch C++ lernen und gerade 
im Bereich AVR gibt es für C++ im Gegensatz zu C nahezu keine 
Beispiele/Bücher/Tutorials oder andere Hilfen.

Von daher weiß ich nicht, was besser ist.
Wobei für mein Studium muss ich mich jetzt eh ein bisschen mit C++ 
beschäftigen.

[Dr. Sommer (Gast)]

Peter II schrieb:
> und in C ist es ja so viel anders
Okay in dem Fall stimmts... Wobei du hier nicht in den Genuss des 
Typesafen enums kommst ;-)

[Dr. Sommer (Gast)]

R. B. schrieb:
> Hatte ich auch überlegt, aber dann müsste ich noch C++ lernen und gerade
> im Bereich AVR gibt es für C++ im Gegensatz zu C nahezu keine
> Beispiele/Bücher/Tutorials oder andere Hilfen.
Das macht nix, der AVR-relevante Teil ist gleich zu C, und der C++ Teil 
ist gleich wie am PC, man muss nur ein bisschen mit der 
Speicherverwaltung aufpassen.

R. B. schrieb:
> Von daher weiß ich nicht, was besser ist.
> Wobei für mein Studium muss ich mich jetzt eh ein bisschen mit C++
> beschäftigen.
C++ zu lernen ist auf jeden Fall eine gute Idee, man lernt eine Menge 
abstrakter Konzepte, und darf danach Java-Leute auslachen ;-)

[Karl H. (kbuchegg)]

R. B. schrieb:

> Wieso ist es unsinnig, wenn man die Lesbarkeit deutlich erhöht?

Es gibt oft einen Punkt, an dem sich die Lesbarkeit nicht mehr erhöht, 
sondern verschlechtert, weil du schon zu viele Details 'versteckt' hast.

spätestens wenn du anfängst, 5 verschiedene Makros zu erfinden, die alle 
eine 'Aktion' als Namenszusatz haben, solltest du dir überlegen, ob du 
den Punkt nicht schon überschritten hast.
Schau dir auch immer die Verwendung im Code an, ob der dann nicht schon 
zu dicht ist. Überleg dir auch, unabhängig davon ob und wie du das 
erreichen kannst, was für dich übersichtlicher Code wäre.

Meine Idealvorstellung würde zb so aussehen

int main(void)
{ 
  DDRB = (1 << DDB0) | (1 << DDB1); 
  PORTB |= (1<<PB2)| (1<<PB3) ;    

  while (1)
  {  
    if (Is_Key_Pressed( KEY_RED ) { 
      TurnOff( LED_RED );
    else
      TurnOn( LED_RED );

    if (Is_Key_Pressed( KEY_BLUE ) { 
      TurnOff( LED_BLUE );
    else
      TurnOn( LED_BLUE );
  }

  return 0;
}

(ignorier mal das Vorgeplänkel mit DDR)

So würde für mich Code aussehen, der leicht zu erfassen ist.
Und jetzt überleg ich mir, mit welchen Funktionen und/oder Makros ich 
auf dieses Ziel zusteuere.

D.h. ich fang hinten an. Beim Ergebnis. Das soll auf der einen Seite 
leicht zu lesen und zu erfassen sein, auf der anderen Seite aber die 
Sache auch nicht aufblähen. Mit 2 Funktionalitäten, der ich jeweils die 
Led vorgeben kann, kann ich auch 50 LED beackern, ohne mir da im 
Programm eine Makro-Orgie einzuhandeln, weil ich für jede LED einen 
Haufen Spezialmakros für ein/aus brauche.
Dasselbe für die Tasten.
Immer auch daran denken, wie etwas verallgemeinert. Eine Systematik

   Test_Key1_0()

verallgemeinert schlecht, ohne dass ich Millionen von Makros anlegen 
muss.

[R. B. (britzi)]

Karl Heinz Buchegger schrieb:
> Es gibt oft einen Punkt, an dem sich die Lesbarkeit nicht mehr erhöht,
> sondern verschlechtert, weil du schon zu viele Details 'versteckt' hast.
>
> Meine Idealvorstellung würde zb so aussehen...



Stimmt, das sehe ich vollkommen ein.
Auf den ersten Blick scheint mir deine Idealvorstellung nicht viel 
besser zu sein, aber dies liegt nur daran, dass ich nicht weiß, wie du 
die Funktionen definieren würdest und ich wieder auf meine Markos 
zurückgreifen würde.

Wenn du mir das alles als vollständiges Beispiel schreiben würdest, 
verspreche ich, mir genau diesen Stil anzugewöhnen ;)

Das wäre echt nett :-)

Vielen Dank!

[Ralf G. (ralg)]

R. B. schrieb:
> Auf den ersten Blick scheint mir deine Idealvorstellung nicht viel
> besser zu sein, aber dies liegt nur daran, dass ich nicht weiß, wie du
> die Funktionen definieren würdest und ich wieder auf meine Markos
> zurückgreifen würde.

Warum Makro? Über so einen typischen Makro-Fallstrick bist du doch 
gerade gestolpert! Eine richtige Funktion. Schön übersichtlich. Und 
wenn's nur eine Zeile ist. Der Compiler macht da möglicherweise genau 
dasselbe Ergebnis daraus. Darin ist der ausgebildet ;-)

Edit: Vielleicht musst du ja mal was erweitern?
Funktion: Eine Zeile dazu.
Makro: ... (ob das noch übersichtlich ist?)

[R. B. (britzi)]

Ja, ich habs verstanden ;-)

Funktionen sind einfacher und schöner.

Ich habe gerade mal probiert eine Funktion zu schreiben, die ein Makro 
nutzt und dann ein Bit setzen soll.

Leider funktioniert es nicht und ich weiß mal wieder nicht, woran es 
liegt.

Dies ist kein vollständiger C-Code, sondern nur die Stellen, die zu 
meinem Problem gehören. Sonst ist der Code wie weiter oben ;)

#define LED1 PB0,PORTB

int Set_On (int Anschluss, int Port)
  {Port |= (1<<Anschluss);
    }
int main(void)
{ 
Set_On(LED1); 
return 0;
}

Wo ist hier der Fehler?

Vielen Dank :-)

[Karl H. (kbuchegg)]

R. B. schrieb:

> int Set_On (int Anschluss, int Port)
>   {Port |= (1<<Anschluss);
>     }

Ein Port Register kannst du nicht so erreichen

void Set_On (uint8_t Anschluss, volatile uint8_t* Port)
{
  *Port |= (1<<Anschluss);
}

> Wo ist hier der Fehler?

Auch wenn ich Funktionen grundsätzlich unterstütze. Wenn du die Namen 
von µC Registern übergeben musst, sind Makros an dieser Stelle 
ausnahmsweise besser.

[R. B. (britzi)]

Karl Heinz Buchegger schrieb:
> void Set_On (uint8_t Anschluss, volatile uint8_t* Port)
> {
>   *Port |= (1<<Anschluss);
> }
>
> Auch wenn ich Funktionen grundsätzlich unterstütze. Wenn du die Namen
> von µC Registern übergeben musst, sind Makros an dieser Stelle
> ausnahmsweise besser.


Vielen Dank :-)

Ich habe das Sternchen hinter dem unit_8 mal and Port gerückt, aber 
dennoch bekomme ich Fehler beim Aufruf der Funktion, wenn ich sie mit 
LED1 aufrufe:

AVR.cpp:45: error: invalid conversion from 'volatile unsigned char' to 
'volatile uint8_t*'
1>  AVR.cpp:45: error:   initializing argument 2 of 'void 
Set_On(uint8_t, volatile uint8_t*)'


Woran liegt das?

[Läubi .. (laeubi)]

R. B. schrieb:
> Woran liegt das?

Steht doch da! Zum Programmieren gehört auch Abstraktionsvermögen, und 
damit sollte man ein Beispiel wohl auf den konkreten Fall abbilden 
können. Sachen per C&P sowie T&E zu übernehmen ist bestenfalls 
frustrierend aber kein Programmieren.

[R. B. (britzi)]

Läubi .. schrieb:
> R. B. schrieb:
>> Woran liegt das?
>
> Steht doch da! Zum Programmieren gehört auch Abstraktionsvermögen, und
> damit sollte man ein Beispiel wohl auf den konkreten Fall abbilden
> können. Sachen per C&P sowie T&E zu übernehmen ist bestenfalls
> frustrierend aber kein Programmieren.

Sorry,

aber für einen Anfänger, der noch nie was mit Informatik am Hut hatte, 
ist es ab und zu nicht gerade einfach Fehlermeldungen zu interpretieren 
und zum Thema C&P finde ich es ab und zu besser erstmal C&P zu machen, 
weil man so eine richtige Struktur lernt, während man sonst, wie ich es 
vorher gemacht habe, z.B. anfängt erstmal 1000 Makros zu schreiben.

[Läubi .. (laeubi)]

R. B. schrieb:
> ist es ab und zu nicht gerade einfach Fehlermeldungen zu interpretieren

Na dann ist jetzt der Zeitpunkt es zu lernen, was verstehst du den an 
der Meldung nicht?

R. B. schrieb:
> AVR.cpp

Wieso endet dein C-Code überhaupt auf cpp = i.A. Dateiendung für C++

[R. B. (britzi)]

Läubi .. schrieb:
> Na dann ist jetzt der Zeitpunkt es zu lernen, was verstehst du den an
> der Meldung nicht?
>
> R. B. schrieb:
>> AVR.cpp
>
> Wieso endet dein C-Code überhaupt auf cpp = i.A. Dateiendung für C++


Ok,
also wenn ich unit8_t zu unsigned char ändere kommt derselbe Fehler nur 
dass es heißt invalid conversion from 'volatile unsigned char' to 
'volatile unsigned char*.

Es muss also was mit dem Sternchen für die Übergabe als Referenz zu tun 
haben. Aber warum ich das mit dem Sternchen hier nicht darf, verstehe 
ich nicht.

Und zu dem Initialisierungsfehler habe ich ehrlich gesagt keine Idee, 
wie er zustande kommt oder wie ich ihn beheben kann.


Und warum das auf .cpp endet und nicht auf .c liegt daran, dass ich nach 
einer Anleitung im Internet Visual Studio so konfiguriert habe, dass ich 
es für AVRs nutzen kann und da war es eben so beschrieben. Da C ja eine 
Untermenge von C++ ist, dürfte das ja nichts machen.

[Läubi .. (laeubi)]

R. B. schrieb:
> s muss also was mit dem Sternchen für die Übergabe als
> Referenz zu tun haben

Dann schau doch mal hier: 
Beitrag "Re: AVR-GCC "C" Port als Variablenwert ablegen und nutzen - ich komm einfach nicht drauf"

R. B. schrieb:
> Da C ja eine Untermenge von C++ ist

Das stimmt so auch nicht: 
http://stackoverflow.com/questions/1201593/c-subset-of-c-where-not-examples

[R. B. (britzi)]

Läubi .. schrieb:
> Dann schau doch mal hier:
> Beitrag "Re: AVR-GCC "C" Port als Variablenwert ablegen und nutzen - ich komm einfach nicht drauf"


Ok, vielen Dank.
Ich habe jetzt alles mögliche probiert und mir auch den anderen Beitrag 
angesehen, aber leider keinen Erfolg gehabt.

Nach wie vor möchte ich, dass ich aus dem einen Makro mit dem Bezeichner 
LED die Variablen PB0 und Port übergebe und es klappt einfach hinten und 
vorne nicht.

Bei dem Port muss ja ein & davor und wenn ich das & schon vor den Port 
im Makro schreibe, funktioniert es nicht und wenn ich das & vor das LED1 
schreibe, dann bezeichnet es ja auch das PB0 und das ist ja auch falsch.

So oder so.... ich bekomme nur Fehlermeldungen und weiß nicht, wie ich 
es lösen soll.

Und ob ich nun schreibe

void Set_On (uint8_t Anschluss, volatile uint8_t *Port)
{Port= &Port;
  *Port = (1<<Anschluss);
}

oder

void Set_On (uint8_t Anschluss, volatile uint8_t *Port)
{
 *Port |= (1<<Anschluss);
}

ist doch im Prinzip dasselbe, oder nicht?

[Stefan R. (srand)]

R. B. schrieb:
> ist doch im Prinzip dasselbe, oder nicht?

Kompiliers doch mal. Dann siehst du schon, daß Port= &Port; 
offensichtlich nicht funktionieren kann.

Wie soll Port denn zugleich von einem bestimmten Datentyp, und zugleich 
vom Typ "Pointer auf Datentyp" sein?

[R. B. (britzi)]

Stefan Rand schrieb:
> R. B. schrieb:
>> ist doch im Prinzip dasselbe, oder nicht?
>
> Kompiliers doch mal. Dann siehst du schon, daß Port= &Port;
> offensichtlich nicht funktionieren kann.
>
> Wie soll Port denn zugleich von einem bestimmten Datentyp, und zugleich
> vom Typ "Pointer auf Datentyp" sein?



Ja ok... stimmt!
Ich sag ja, ich bin blutiger Anfänger sowohl mit C als auch mit AVRs ;-)

Gibt es denn nun eine elegante Möglichkeit mein "Problem" zu lösen?