Programmierstil so für Mikrocontroller OK? [C]

[Christian H. (christian_h849)]

Hallo Zusammen,

da ich aus der C# und Anwendungsentwicklung komme und mich in die C Welt 
mit Mikrocontroller ebenfalls zurechtfinden möchte, bitte ich Euch auf 
meinen Code zu sehen, ob ich vom Grundansatz her auf dem richtigen Weg 
bin bzw. Ihr eure Augen verdreht.

Ich möchte mir nicht von Anfang an Murks einprägen :-).

Zur Info ich habe folgendes "Material":
- Atmel Studio 7
- STK600 mit Atmega1284-PU @ 16MHz

Main.c:

#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include "Lampe.h"
#include "TimerTrigger.h"

#define LED0 &PORTB, &DDRB, PINB0, LOWACTIVE
#define LED1 &PORTB, &DDRB, PINB1, LOWACTIVE
#define LED2 &PORTB, &DDRB, PINB2, LOWACTIVE
#define LED3 &PORTB, &DDRB, PINB3, LOWACTIVE

ISR(TIMER1_COMPA_vect) //20ms Timer
{
  Timer_InterruptAction();
}

int main(void)
{
  Timer_InitTimer1with20ms();

  sei(); //Aktivere Interrupts



  while (1)
  {
    Timer_CheckState();
    
    LampeSwitch(LED3, (1 & (Timer_ToggleState>>Timer_200ms)));
    LampeSwitch(LED2, (1 & (Timer_ToggleState>>Timer_1000ms)));



    Timer_ClearState();
  }
}

Lampe.h:

#ifndef LAMPE_H_
#define LAMPE_H_

#include <avr/io.h>


#define LOWACTIVE 0
#define HIGHACTIVE 1

#define ON 1
#define OFF 0

void LampeSwitch(volatile uint8_t *port,volatile uint8_t *ddr, uint8_t pin, uint8_t lowOrHighActive, uint8_t OnOff);

#endif /* LAMPE_H_ */

Lampe.c:

 #include "Lampe.h"

 void LampeSwitch(volatile uint8_t *port,volatile uint8_t *ddr, uint8_t pin, uint8_t lowOrHighActive, uint8_t OnOff){
   
   *ddr |= (1<<pin);

   if ((OnOff && lowOrHighActive) || (!OnOff && !lowOrHighActive))
   {
     *port |= (1<<pin);
   }
   else
   {
     *port &= ~(1<<pin);
   }
 };

TimerTrigger.h:

#ifndef TIMERTRIGGER_H_
#define TIMERTRIGGER_H_

#include <avr/io.h>

#define Timer_20ms 0
#define Timer_100ms 1
#define Timer_200ms 2
#define Timer_500ms 3
#define Timer_1000ms 4

char Timer_PulseState; //Abrufen der einzelnen Pulse pro Zyklus.
char Timer_ToggleState; //Abrufen des toogle Statuses.

extern Timer_InitTimer1with20ms(); //Wird aufgerufen wenn das Programm initialisiert wird (einmalig). Interrupts müssen noch explizit über sei() aktiviert werden.
extern Timer_InterruptAction(); //In Interrupt Routine integrierern.
extern Timer_CheckState(); //Muss bei jedem Zyklus am Anfang aufgerufen werden.
extern Timer_ClearState(); //Muss am ende von jedem Zyklus aufgerufen werden.


#endif /* TIMERTRIGGER_H_ */

TimerTrigger.c:

#include "TimerTrigger.h"

volatile uint32_t timerIntCounter;
uint32_t lastTimerIntCounter;


Timer_InitTimer1with20ms(){
  TCCR1B|=(1<<WGM12);//CTC-Mode
  TCCR1B|=(1<<CS12); //PreScaler = 256
  OCR1A|= (20*(F_CPU/1000/256)-1); //20sec -> 1249 Schritte bei Prescalor von 256
  TIMSK1 |=(1<<OCIE1A);//Interupt für MatchA aktivieren
}

Timer_InterruptAction(){
  timerIntCounter = ((timerIntCounter & 0xFFFFFFF0) | ((0x0F & timerIntCounter) + 1)); //1-4bit für 100ms Trigger
  timerIntCounter = ((timerIntCounter & 0xFFFFFF0F) | (((0x0F & (timerIntCounter>>4)) + 1))<<4); //5-8bit für 200ms Trigger
  timerIntCounter = ((timerIntCounter & 0xFFFF00FF) | (((timerIntCounter>>8) + 1)<<8)); //4-2bit für 500ms Trigger
  timerIntCounter = ((timerIntCounter & 0xFF00FFFF) | (((timerIntCounter>>16) + 1)<<16)); //8-5bit für 1000ms Trigger
}

Timer_CheckState()
{
  if (lastTimerIntCounter == timerIntCounter) //verlasse Routine
  { return; }
  
  Timer_PulseState |= (1<<Timer_20ms);
  Timer_ToggleState ^= (1<<Timer_20ms);


  if ((0x0F & timerIntCounter) >= 5)//100ms
  {
    Timer_PulseState |= (1<<Timer_100ms); 
    Timer_ToggleState ^= (1<<Timer_100ms);
    timerIntCounter &= 0xFFFFFFF0;
  }

  if ((0x0F & (timerIntCounter>>4)) >= 10) //200ms
  {
    Timer_PulseState |= (1<<Timer_200ms);
    Timer_ToggleState ^= (1<<Timer_200ms);
    timerIntCounter &= 0xFFFFFF0F;
  }

  if ((0xFF & (timerIntCounter>>8)) >= 25) //500ms
  {
    Timer_PulseState |= (1<<Timer_500ms);
    Timer_ToggleState ^= (1<<Timer_500ms);
    timerIntCounter &= 0xFFFF00FF;
  }

  if ((0xFF & (timerIntCounter>>16)) >= 50) //1000ms
  {
    Timer_PulseState |= (1<<Timer_1000ms);
    Timer_ToggleState ^= (1<<Timer_1000ms);
    timerIntCounter &= 0xFF00FFFF;
  }
  
  lastTimerIntCounter = timerIntCounter;
}

Timer_ClearState()
{
  Timer_PulseState=0;
}

Ich hoffe das erschlägt jetzt nicht :). Es geht mir jetzt nicht um jedes 
kleine Detail. Sondern mehr darum ob dieser Abstrahierungsgrad 
vertretbar ist.

Freundliche Grüße
Christian

[THOR (Gast)]

Als jemand der auch aus der C# Ecke kommt aber schon sehr lange C auf 
AVRs programmiert:

Das sieht deutlich besser aus als 99% von dem was sonst hier im Forum 
aufschlägt. Hab mir allerdings jetzt nicht jede Zeile genauestens 
angeguckt.

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Christian H. schrieb:

   if (lastTimerIntCounter == timerIntCounter) //verlasse Routine
      { return; }
   // sonst: tu was

Ich würde zumindest das andersrum machen und so nur einen Aussprungpunkt 
aus der Routine haben:

   if (lastTimerIntCounter != timerIntCounter) {
      // tu was
      lastTimerIntCounter = timerIntCounter;
   }

Und nimm statt dieser Zu-Tode-Shifterei-Und-Zusammenpackerei hier als 
Zähler einfach ein paar einzelne Bytes oder Worte. So braucht die ISR je 
nach µC ewig (zumindest bei einem AVR, der keinen Barrelshifter hat):

Timer_InterruptAction(){
  timerIntCounter = ((timerIntCounter & 0xFFFFFFF0) | ((0x0F & timerIntCounter) + 1)); //1-4bit für 100ms Trigger
  timerIntCounter = ((timerIntCounter & 0xFFFFFF0F) | (((0x0F & (timerIntCounter>>4)) + 1))<<4); //5-8bit für 200ms Trigger
  timerIntCounter = ((timerIntCounter & 0xFFFF00FF) | (((timerIntCounter>>8) + 1)<<8)); //4-2bit für 500ms Trigger
  timerIntCounter = ((timerIntCounter & 0xFF00FFFF) | (((timerIntCounter>>16) + 1)<<16)); //8-5bit für 1000ms Trigger
}

(Ok, ich geb zu, der Compiler kann das optimieren, aber trotzdem ist das 
Zusammenpacken und Auseinandernehmen unleserlicher...)

Zudem wäre es sinnvoll, z.B. den 100ms-5er-Zähler nicht einfach 
zurückzusetzen, sondern nach Setzen des Flags einfach 5 abzuziehen. Denn 
wenn der Zähler aus irgendwelchen Gründen zwischenzeitlich schon bei 8 
angekommen wäre, dann hast du mit deiner Methode 60ms "Verlust", ich mit 
der Subtraktion nur einen höheren Jitter.

[Peter II (Gast)]

ich würde mal sagen gut gedacht, aber ja nach Compiler und Option nicht 
optimal.

ISR(TIMER1_COMPA_vect) //20ms Timer
{
  Timer_InterruptAction();
}

ein Funktionsaufruf in der ISR führ dazu das alles Register gesichert 
werden müssen -> langsam. Mit Link-Time-Optimierung könnte es besser 
werden.

auch das Schalten der LEDs mit dem LEDs makro find ich nicht wirklich 
schön. müsste man mal genau schauen was der Compiler daraus macht.

Auch das setzen vom DDR bei jedem Schalten vom Ausgang ist nicht 
sinnvoll.

Bei µC würde ich mehr als Optimierung achten, auch wenn es nicht immer 
notwendig ist.

[Christian H. (christian_h849)]

Hi Zusammen,

wow... vielen Dank für die schnellen Antworten (ich dachte schon ich 
kann mich erstmal in´s Bett legen bevor es weiter geht :-)


Lothar M. schrieb:
> Christian H. schrieb:
>

>    if (lastTimerIntCounter == timerIntCounter) //verlasse Routine
>       { return; }
>    // sonst: tu was
>

> Ich würde zumindest das andersrum...


Ich habe das jetzt so umgesetzt:

Timer_CheckState()
{
  if (Timer_InterruptCounter) //wenn die 20ms ISR den Counter erhöht hat.
  {    

    counter_100ms += Timer_InterruptCounter;
    counter_200ms += Timer_InterruptCounter;
    counter_500ms += Timer_InterruptCounter;
    counter_1000ms += Timer_InterruptCounter;

    Timer_InterruptCounter = 0;

    Timer_PulseState |= (1<<Timer_20ms);
    Timer_ToggleState ^= (1<<Timer_20ms);


    if (counter_100ms >= 5)//100ms
    {
      Timer_PulseState |= (1<<Timer_100ms);
      Timer_ToggleState ^= (1<<Timer_100ms);
      counter_100ms -= 5;
    }

    if (counter_200ms >= 10) //200ms
    {
      Timer_PulseState |= (1<<Timer_200ms);
      Timer_ToggleState ^= (1<<Timer_200ms);
      counter_200ms -= 10;
    }

    if (counter_500ms >= 25) //500ms
    {
      Timer_PulseState |= (1<<Timer_500ms);
      Timer_ToggleState ^= (1<<Timer_500ms);
      counter_500ms -= 25;
    }

    if (counter_1000ms >= 50) //1000ms
    {
      Timer_PulseState |= (1<<Timer_1000ms);
      Timer_ToggleState ^= (1<<Timer_1000ms);
      counter_1000ms -= 50;
    }

  }
}

die Variable "Timer_InterruptCounter" habe ich jetzt in den 
TimerTrigger.h verschoben und zähle diese direkt aus der main.c 
innerhalb der ISR hoch.

TimerTrigger.h:

...
volatile uint8_t Timer_InterruptCounter; // muss in ISR des Timer eingebunden werden: Timer_InterruptCounter += 1;
...

main.c:

ISR(TIMER1_COMPA_vect) //20ms Timer
{
  Timer_InterruptCounter+=1;
}

Peter II schrieb:
> auch das Schalten der LEDs mit dem LEDs makro find ich nicht wirklich
> schön. müsste man mal genau schauen was der Compiler daraus macht.
>
> Auch das setzen vom DDR bei jedem Schalten vom Ausgang ist nicht
> sinnvoll.

Ist es sinnvoll einen Ausgang/Eingang mit einem struct zu definieren?

typedef struct{
  volatile uint8_t * port;
  volatile uint8_t * ddr;
  uint8_t pin;
  uint8_t lowOrHigh;
} Output;

und dann mit spezifischen Funktionen arbeiten z.B.:
- Initialisieren(Output value)
- Schalten(Output value)
- ...


Gruß Christian

[Peter II (Gast)]

Christian H. schrieb:
> Ist es sinnvoll einen Ausgang/Eingang mit einem struct zu definieren?

würde ich auch nicht machen.

Ich habe mir nur ein paar Makros in der Art geschrieben.

#define LED1_PORT PORTD
#define LED1_PIN PIN1
#define LED1_INVERT 0

der Aufruf dann mit ein paar Makros 

SET_OUTPUT( LED1 );

SET_ON( LED1 );
SET_OFF( LED1 );

kann man zwar mit C++ auch über Templates machen, das sehe ich aber kaum 
ein Vorteil.

[Christian H. (christian_h849)]

Hallo Peter,

danke für Deine Antwort.
Wie finden in Deinem Code die Methoden SET_OUTPUT, SET_ON und SET_OFF 
die Referenz zu den #define´s?
Du übergibst soviel ich sehen kann ja nur LED1 und nicht die einzelnen 
LED_PORT usw...

Könntest Du mir bitte den Code von den Methoden aufzeigen?

Gruß Christian

[A. S. (Gast)]

Also ehrlich gesagt, finde ich Deinen Code viel zu unnötig abstrahiert.

a) Als Zeitbasis ist 1ms perfekt, 10ms oder 100ms ebenso, aber 5ms??
--> Timer_InterruptCounter+=2; oder +=20; //im 20ms Interrupt
Das der Kommentar an jedem Timer notwendig ist, zeigt, dass der Code 
nicht gut ist.


b) die Bitschiebereien sind völlig unnötig. Deren Beherrschung wird zwar 
vorausgesetzt, deren Verwendung hingegen zeugt von wenig Struktur.
Entweder mache Dir Bit-Strukturen (mit struct. im Code z.B. 
Timer_PulseState.timer200 = 1;)
Oder mache Dir Funktionen/Makros zum Bitsetzen etc., z.B. SETB(addr, 
Bit)

c) Nutze Timer mit je einem festen Wert bei insgesamt einem 
Free-Running-Counter. Also z.B. msTick+=20 statt 
"Timer_InterruptCounter". Der counter_1000ms wird dann beim ersten Start 
auf (msTick+1000) gesetzt und in einer "Differenz"(ganz wichtig!) 
ausgewertet, z.B.
[c]
if((msTick-counter_1000ms) > 0) {counter_1000+=1000; .... ;}

d) Versuche Deinen Code wirklich Stück für Stück kompakter und 
verständlicher zu machen. Die LEDs beispielsweise sind ein Anfang, aber 
die #defines Dafür sind ein Monster. Sie so allgemein zu halten scheint 
"flexibel", ist aber absolut keine Hardware-Abstraktion, da genau der 
Controller mit der Beschaltung vorausgesetzt wird.

[PittyJ (Gast)]

Meine Tipps
Auf auf Mikrocontrollern gibt es C++. Nimm statt structs eine Klasse und 
alles ist gut.
Verzichte auf Defines und Macros. Für Defines gibt es feine Sachen wie
static const int HighActive=0;
Aud für Makros normale Funktionen nehmen. Dann kann der Compiler damit 
arbeiten und es gibt keine Seiteneffekte.
Und Zeilen länger als die Bildschirmbreit geht gat nicht. Der Code muss 
mit einem Blick erfassbar sein, und man muss nicht erst nach rechts 
scrollen.

void LampeSwitch(volatile uint8_t *port,
                 volatile uint8_t *ddr,
                 uint8_t           pin,
                 uint8_t           lowOrHighActive,
                 uint8_t           OnOff
                 )
{

Ist übersichlicher, man kan die Parameter viel besser erkennen.

[Peter II (Gast)]

Christian H. schrieb:
> Hallo Peter,
>
> danke für Deine Antwort.
> Wie finden in Deinem Code die Methoden SET_OUTPUT, SET_ON und SET_OFF
> die Referenz zu den #define´s?
> Du übergibst soviel ich sehen kann ja nur LED1 und nicht die einzelnen
> LED_PORT usw...
>
> Könntest Du mir bitte den Code von den Methoden aufzeigen?

nicht schön aber funktioniert:

#define _SET_INPUT_2( PORT, PIN ) DDR ## PORT &= ~(1<<PIN)
#define _SET_INPUT_1( PORT, PIN ) _SET_INPUT_2( PORT, PIN)
#define SET_INPUT( NAME ) _SET_INPUT_1( NAME ## _PORT, NAME ## _PIN)

#define _SET_OUTPUT_2( PORT, PIN ) DDR ## PORT |= (1<<PIN)
#define _SET_OUTPUT_1( PORT, PIN ) _SET_OUTPUT_2( PORT, PIN)
#define SET_OUTPUT( NAME ) _SET_OUTPUT_1( NAME ## _PORT, NAME ## _PIN)

#define SET_ON_2( _PORT, PIN, INV ) if ( INV != 0 ) { PORT ## _PORT &= ~(1<<PIN); } else { PORT ## _PORT |= (1<<PIN); }
#define SET_ON_1( PORT, PIN, INV ) SET_ON_2( PORT, PIN, INV )
#define SET_ON( NAME ) SET_ON_1( NAME ## _PORT, NAME ## _PIN, NAME ## _INVERT )

#define SET_OFF_2( _PORT, PIN, INV ) if ( INV == 0 ) { PORT ## _PORT &= ~(1<<PIN); } else { PORT ## _PORT |= (1<<PIN); }
#define SET_OFF_1( PORT, PIN, INV ) SET_OFF_2( PORT, PIN, INV )
#define SET_OFF( NAME ) SET_OFF_1( NAME ## _PORT, NAME ## _PIN, NAME ## _INVERT ) 

#define SET_PULLUP_ON_2( _PORT, PIN, INV ) { PUE ## _PORT |= (1<<PIN); }
#define SET_PULLUP_ON_1( PORT, PIN, INV ) SET_PULLUP_ON_2( PORT, PIN, INV )
#define SET_PULLUP_ON( NAME ) SET_PULLUP_ON_1( NAME ## _PORT, NAME ## _PIN, NAME ## _INVERT )

#define SET_PULLUP_OFF_2( _PORT, PIN, INV ) { PORT ## _PORT &= ~(1<<PIN); }
#define SET_PULLUP_OFF_1( PORT, PIN, INV ) SET_PULLUP_OFF_2( PORT, PIN, INV )
#define SET_PULLUP_OFF( NAME ) SET_PULLUP_OFF_1( NAME ## _PORT, NAME ## _PIN, NAME ## _INVERT )

#define IS_ON_2( _PORT, _PIN, INV ) (INV?(!(PIN ## _PORT & (1<<_PIN))):(PIN ## _PORT & (1<<_PIN)) )
#define IS_ON_1( PORT, PIN, INV ) IS_ON_2( PORT, PIN, INV )
#define IS_ON( NAME ) IS_ON_1 ( NAME ## _PORT, NAME ## _PIN, NAME ## _INVERT )

[Felix F. (wiesel8)]

Jaja, hier wirst du auf 10 Leute 12 verschiedene Antworten bekommen. Der 
eine will weniger Abstraktion, der andere kommt gleich mit 
C++-Klassen...

Prinzipiell sieht der Code ganz gut aus, aber:
Kein Denglisch, bleib einfach überall bei Englisch

Sowas hab ich noch nie gesehen und verwendet auch keiner

#define LED3 &PORTB, &DDRB, PINB3, LOWACTIVE

Besser

#define LED3_PORT DDRB

In Interrupts die Befehle möglichst direkt abarbeiten und kurz halten.

Prinzipiell ist es eher üblich, einen 1ms "SysTick" zu verwenden. Hier 
kannst du einen Counter hochzählen lassen und dann auswerten. Dann muss 
man für 500ms auch nicht auf 25 prüfen.

Abstraktion kommt drauf an, was du machen willst. Willst du denn Code 
wieder verwenden (auf einem anderen Prozessor), dann macht das Sinn. Ist 
der Code nur für diesen einen Controller? Spar dir die Mühe.

Sowas kann man durchaus machen. Erhöht die Lesbarkeit und die 
Portabilität, aber ein paar Assembler-Freaks hier rotieren gerade auf 
ihrem Bürostuhl :)

typedef struct{
  volatile uint8_t * port;
  volatile uint8_t * ddr;
  uint8_t pin;
  uint8_t lowOrHigh;
} Output;

mfg

[Rufus Τ. F. (rufus)]

Christian H. schrieb:
> die Variable "Timer_InterruptCounter" habe ich jetzt in den
> TimerTrigger.h verschoben

Das ist Käse. Da gehören Variaben nie hin.

[Peter D. (peda)]

Christian H. schrieb:
> Ist es sinnvoll einen Ausgang/Eingang mit einem struct zu
> definieren?typedef struct{
>   volatile uint8_t * port;
>   volatile uint8_t * ddr;
>   uint8_t pin;
>   uint8_t lowOrHigh;
> } Output;

Damit würden für jeden einzelnen Pin 6 Byte SRAM belegt und der ist bei 
den 8-Bittern nicht so üppig.
Ich hab mir daher für die Pinzugriffe Macros definiert.
Hier ein Beispiel:
Beitrag "Ooch nöö, jetzt ist der Thread weg"

Und die sbit.h:
https://www.mikrocontroller.net/attachment/highlight/254049

[Einer K. (Gast)]

Felix F. schrieb:
> Sowas hab ich noch nie gesehen und verwendet auch keiner#define LED3
> &PORTB, &DDRB, PINB3, LOWACTIVE

Naja....
Vielleicht nicht genau so....
Aber doch sehr ähnlich!

[Jörg W. (dl8dtl) (Moderator)]

Felix F. schrieb:
> Sowas hab ich noch nie gesehen und verwendet auch keiner #define LED3
> &PORTB, &DDRB, PINB3, LOWACTIVE

Nur, weil du es noch nicht gesehen hast, heißt ja nicht, dass es
keinen Sinn haben kann. ;-)

> Besser #define LED3_PORT DDRB

Ist nicht dasselbe.  Das behandelt nämlich nur den Ausgabeport,
nicht aber die anderen Dinge.

Allerdings kann man so einen Makro nur sinnvoll da benutzen, wo
er all seine Argumente hinein substituieren kann.  In dieser Hinsicht
wiederum kann man sich natürlich über den Programmierstil streiten,
wenn ein Makro mehrere (Funktions-)Argumente gleich auf einmal liefert.
Das ist auf den ersten Blick verwirrend und nicht ganz so toller Stil,
aber dafür ein guter Kompromiss zwischen Lesbarkeit und Effizienz.

PINB3 hätte ich aber besser PB3 genannt.

[Felix F. (wiesel8)]

Jörg W. schrieb:
> Felix F. schrieb:
>> Sowas hab ich noch nie gesehen und verwendet auch keiner #define LED3
>> &PORTB, &DDRB, PINB3, LOWACTIVE
>
> Nur, weil du es noch nicht gesehen hast, heißt ja nicht, dass es
> keinen Sinn haben kann. ;-)
Ich lasse mich gerne eines Besseren belehren. Wo macht so etwas denn 
Sinn? Und bitte verlinke mir doch ein entsprechendes Projekt, damit ich 
es mal im Einsatz sehe :)


>> Besser #define LED3_PORT DDRB
>
> Ist nicht dasselbe.  Das behandelt nämlich nur den Ausgabeport,
> nicht aber die anderen Dinge.
Das sollte auch nur eine Anregung sein, wie er es Sinnvoll aufspalten 
kann.

> PINB3 hätte ich aber besser PB3 genannt.
Wenn ich mich nicht irre, ist PINB3 ein Makro/Define von Atmel selbst.

mfg

[Einer K. (Gast)]

Jörg W. schrieb:
> aber dafür ein guter Kompromiss zwischen Lesbarkeit und Effizienz.

Bei mir sieht das mittlerweile meist so aus:

#include <FastPin.h>

#define LED PINDEF(B,5)  // PortB Bit5, die LED auf dem UNO 

void setup() 
{
  setOutput(LED);
}

void loop() 
{
  togglePin(LED);
  delay(500);
}

Die *.h im Anhang ist für die Arduino Welt, sollte aber unverändert mit 
AS uvm. laufen.

[Jörg W. (dl8dtl) (Moderator)]

Felix F. schrieb:

>> Nur, weil du es noch nicht gesehen hast, heißt ja nicht, dass es
>> keinen Sinn haben kann. ;-)

> Ich lasse mich gerne eines Besseren belehren. Wo macht so etwas denn
> Sinn?

#define LED_INIT_(port,ddr,pin,polarity) do { ddr |= (1<<(pin)); } while(0)
#define LED_INIT(x) LED_INIT_(x)
#define LED_SET_(port,ddr,pin,polarity) do { if(polarity) port |= (1<<(pin)); else port &= ~(1<<(pin)); } while(0)
#define LED_SET(x) LED_SET_(x)

...
   LED_INIT(LED3);
   LED_SET(LED3);

Das ist jetzt nur mal schnell aus dem Stegreif zusammengeschustert,
sollte aber verdeutlichen, warum es gar nicht so daneben ist, wenn
man alle Dinge für LED3 in einem Makro unterbringen kann.

>> PINB3 hätte ich aber besser PB3 genannt.

> Wenn ich mich nicht irre, ist PINB3 ein Makro/Define von Atmel selbst.

Naja, von Atmel nicht direkt, sondern aus der avr-libc.  Aber das
"PIN" darin deutet darauf hin, dass es sich dabei logisch um die
Pinbezeichnung bezüglich "PINB" handelt.  Analog gibt es dann auch
noch PORTB3 für PORTB und DDRB3 für DDRB.  PB3 dagegen ist eine Art
generische Pinnummer.

Ist natürlich weitgehend nebensächlich, da die Pinnummern in allen
Registern natürlich stets übereinstimmen.  Man kann genausogut gleich
"3" schreiben.

[W.S. (Gast)]

Christian H. schrieb:
> bzw. Ihr eure Augen verdreht.
>
> Ich möchte mir nicht von Anfang an Murks einprägen :-).

OK, das verstehe ich.

Also, dann fangen wir mal an mit der Manöverkritik:

1. Sowas wie

#define LED0 &PORTB, &DDRB, PINB0, LOWACTIVE

ist gar keine Abstraktion, es ist auch nicht wirklich nützlich. Deine 
Absicht scheint es zu sein, in den höheren Programmschichten nicht mit 
Pins, Ports und sowas herumfummeln zu müssen. Das ist der richtige 
Gedanke, aber du hast ihn falsch umgesetzt.

Schreibe dir lieber einen Lowlevel-Treiber in separater Quelldatei, 
deren .h garnichts vom Innenleben des Treibers verrät, außer dem 
tatsächlichen Setzen der LED, also etwa so:

extern void LED0_ein (void);
extern void LED0_aus (void);
// allenfalls eine Rückmeldung:
extern bool LED0_ist_an (void);

Wie das treiberintern gelöst ist, soll den Rest der Welt nichts angehen. 
Und falls du dein Produkt mal auf eine andere Architektur umsetzt, 
brauchst du nur die Innereien des Treibers anzupassen - der ganze Rest 
kann so bleiben, denn er ist hardwareunabhängig.

2. zum Interrupt:
ISR(TIMER1_COMPA_vect) //20ms Timer
{
  Timer_InterruptAction();
}

Gibt es einen Grund, den Interrupt nicht in der ISR zu erledigen, 
sondern dazu eine andere Funktion aufzurufen? Nein, gibt es nicht. Außer 
du beabsichtigst, eben diese Funktion auch mal von woanders aufzurufen, 
was ein schlimmer Denk- bzw. Entwurfsfehler ist. Also laß sowas lieber 
und geh nicht auf's Eis tanzen.

3. Funktionalitäten

void LampeSwitch (volatile uint8_t *port,
                  volatile uint8_t *ddr, 
                  uint8_t pin, 
                  uint8_t lowOrHighActive, 
                  uint8_t OnOff);

Du knetest hier so ziemlich alles hinein, was nicht zusammengehört.

Also noch einmal: Trenne Lowlevel und Highlevel, also Peripherietreiber 
vom Algorithmus und halte die Hardwareschnittstelle einfach und auf's 
Wesentliche beschränkt. Wenn du nicht mehr als Lampe ein oder aus machen 
willst, dann gehören all die anderen Dinge nicht in den Header.Nicht 
nur, daß sowas deinen Code unnötig kompliziert macht, es ist auch 
unverständlich. Und du selber wirst dich in einem halben Jahr in deinem 
eigenen Code nicht mehr zurechtfinden.

Verstehe mal die Herangehensweise: Du versuchst, das Ganze vom 
göttlichen Standpunkt aus zu formulieren, etwa so:
void ErschaffeDieWelt ( param1, param2,...param9999999999999);
und wenn du mal bloß eine Mücke erschaffen willst, mußt du alle 
Milliarden Parameter richtig setzen, sonst kommt ne Herde Elefanten 
dabei heraus. Viel einfacher ist das mit nem Treiber, der nur eines 
kennt:
void ErschaffeNeMücke(void);
Der ist natürlich überhaupt nicht universell und man kann damit eben 
nicht eine Elefanteherde erschaffen, dafür aber ist er einfacher in der 
Benutzung, wesentlich lesbarer und auch sicherer in jeder Weise, 
insbesondere gegen Schusselfehler.

Ich skizziere hier mal ein Gegenbeispiel:

bool beleuchtet;
int  ereignis;

ereignis = GetEreignis();
if (ereignis==isTastendruck)
 { if (beleuchtet) 
     { Notbeleuchtung_ein();
       LED0_aus();
       beleuchtet = false;
     }
   else
     { LED0_ein();
       AddEreignis(isBesucherGekommen);
       Notbeleuchtung_aus();
       beleuchtet = true;
     }
   ereignis = 0;
 }

Sag jetzt nicht, die Logik so eines Programmstückes bliebe dir obskur.

W.S.

[Susi (Gast)]

> 2. zum Interrupt:
> ISR(TIMER1_COMPA_vect) //20ms Timer
> {
>   Timer_InterruptAction();
> }
>
> Gibt es einen Grund, den Interrupt nicht in der ISR zu erledigen,
> sondern dazu eine andere Funktion aufzurufen? Nein, gibt es nicht. Außer
> du beabsichtigst, eben diese Funktion auch mal von woanders aufzurufen,
> was ein schlimmer Denk- bzw. Entwurfsfehler ist. Also laß sowas lieber
> und geh nicht auf's Eis tanzen.


Ich habe mal eine Frage, bei einen Interrupt müssen die Register sowieso 
gesichert werden, warum sollte da dann mehr als ein Subroutine Aufruf 
sein und warum sollten dann Register gesichert werden?

[Peter II (Gast)]

Susi schrieb:
> Ich habe mal eine Frage, bei einen Interrupt müssen die Register sowieso
> gesichert werden, warum sollte da dann mehr als ein Subroutine Aufruf
> sein und warum sollten dann Register gesichert werden?

den code direkt in der ISR überblickt der Compiler und sichert nur die 
Register die verändert werden. Sobald eine Funktion enthalten ist, die 
er nicht inline macht, muss er alle Register sichern.

[Peter D. (peda)]

Arduino F. schrieb:
> Bei mir sieht das mittlerweile meist so aus:
> #include <FastPin.h>
> ...

Gefällt mir.

[Christian H. (christian_h849)]

Hallo Zusammen,

ich möchte mich nochmals bei ALLEN recht herzlich für die ganzen Tipps 
und Wegweiser bedanken!

@W.S. Die Ausführung hat mir gut weitergeholfen!

@Fanboy Danke für die Variante.

Gruß Christian

[Einer K. (Gast)]

Peter D. schrieb:
> Gefällt mir.
Danke für die Blumen!

Christian H. schrieb:
> @Fanboy Danke für die Variante.
Wenn es dir hilft...
Gern geschehen!