C [AVR][Atmega328p] Frage zum code

[Quinix1]

Hallo,
ich schreibe einen code mit Interrupt und Timer. Eine LED(PortB) soll dauerhaft zum blinken gebracht werden, und die anderen LEDs(PortC) sollen an sein, solange einer der 3 Taster gedrückt wird(PINB).

Was im Moment passiert:
-LEDs(PortC) blinken synchron mit LED(PortB) solange der Taster gedrück istt.
-Wenn ich den Taster loslasse, während LED(PortB) an ist, dann bleiben LEDs(PortC) an(ansonsten gehen sie aus)

Was ich nicht verstehe:
Warum blinken die LEDs(PortC) auch?
Und warum gehen die LEDs(PortC) nicht aus, wenn ich den Taster loslasse während LED(PortB) an ist?

Danke

#include <avr/io.h>
#include <avr/interrupt.h>

void warten(void) {
	while ( !(TIFR1 & (1 << OCF1A ) ) ) // Auf "Wecker" warten
	;
	TIFR1 |= (1 << OCF1A ); // Interruptflag löschen
}

int main(void) {

	OCR1A = 31250-1;

	TCCR1B |= (1 << WGM12); //turn on CTC mode
	TCCR1B |= (1 << CS12) | (1 << CS10);	//Prescalemode 101 -> 1024 Takt

	DDRB = 0x20; //kontrolleuchte aktivieren
	DDRC = 0x3f; //alle LED aktivieren
	
	PCICR = 0x3f; //Pin Change Interrupt Control Register 
	PCMSK0 =0x3f; //taster aktivieren

	sei();

	while(1) {
		PORTB ^= 0x20;	//kontroll LED
		warten();		//blinken
	}
}

ISR(PCINT0_vect){
	   
		if (PINB & 0b00000001) 	//if rechte Taste gedrückt
			PORTC ^= 0b00000011;	//2 rechte LED	
		
		if (PINB & 0b00000100)	//if  mittlere Taste gedrückt
			PORTC ^= 0b00001100;	//2 mittlere LED
		
		if (PINB & 0b00010000)		//if  linke Taste gedrückt
			PORTC ^= 0b00110000;	//2 linke LED
		
		if (PINB & 0b00000000)		//if keine Taste gedrüvckt
			PORTC ^= 0b00000000;
}

 

[Vanilla2001]

Ich vermute, das blinken deiner Kontroll-LED erzeugt ebenfalls einen PCINT, ohne auf die PCMSK zu gucken.

Desweiteren solltest du das setzen der entsprechenden Bits nicht immer per XOR machen.

Was soll das

if (PINB & 0b00000000)

bringen?

 

[Quinix1]

Was soll das

if (PINB & 0b00000000)

bringen?

Stimmt. brauch ich ja gar nicht


Aber deine Erklärung, wegen dem Blinken kann ich nicht wirklich nachvollziehen. Warum passiert das?

 

[Vanilla2001]

Deine DEBUG-LED hängt so wie ich es begreife an PortB5, das entspricht PCINT5.

Mit

PCMSK0 =0x3f; //taster aktivieren

hast du PCINT5 auch als Quelle aktiviert.

Ergänzung (5. Mai 2017)

Gewöhn Dir z.B. so eine Schreibweise an:

PCMSK0 = ( 1 << PCINT0 ) | ( 1 << PCINT2 ) | ( 1 << PCINT4 );

Da siehst du das schneller und aktivierst auch nur die Quellen, die du wirklich haben willst. Ohne PCINT1/PCINT3/PCINT5.

 

[Quinix1]

Irgendwas stimmt noch nicht. Bekomme ich noch nen Tipp?

#include <avr/io.h>
#include <avr/interrupt.h>

void warten(void) {
	while ( !(TIFR1 & (1 << OCF1A ) ) ) // Auf "Wecker" warten
	;
	TIFR1 |= (1 << OCF1A ); // Interruptflag löschen
}

int main(void) {

	OCR1A = 31250-1;	//2 sekunden

	TCCR1B |= (1 << WGM12); //turn on CTC mode
	TCCR1B |= (1 << CS12) | (1 << CS10);	//Prescalemode 101 -> 1024 Takt

	DDRB = ( 1 << PCINT5 ); //kontrolleuchte aktivieren
	DDRC = 0x3f; //alle LED aktivieren
	
	PCICR   = (1<<PCIE0); //Pin Change Interrupt Control Register
	PCMSK0 |= ( 1 << PCINT0 ) | ( 1 << PCINT2 ) | ( 1 << PCINT4 ); //taster aktivieren

	sei();

	while(1) {
		PORTB ^= ( 1 << PCINT5 );	//kontroll LED
		warten();					//blinken
	}
}

ISR(PCINT0_vect){
	
	if (PINB & ( 1 << PCINT0 )) 	//if rechte Taste gedrückt
	PORTC ^= 0b00000011;			//2 rechte LED (ADC0D,ADC1D)
	
	if (PINB & ( 1 << PCINT2 ))		//if  mittlere Taste gedrückt
	PORTC ^= 0b00001100;			//2 mittlere LED (ADC2D,ADC3D)
	
	if (PINB &( 1 << PCINT4 ))		//if  linke Taste gedrückt
	PORTC ^= 0b00110000;			//2 linke LED (ADC4D,ADC5D)
}

 

[Vanilla2001]

Zwei Tipps.

Ein Taster hat zwei Zustände. Low oder High.

Was genau passiert bei dieser Zeile und wann?

PORTC ^= 0b00000011;

Den Gedanken hattest du schon. Hast ihn nur noch nicht richtig umgesetzt. Deswegen war die letzte Zeile in deiner ISR die du zwar gelöscht hast aber vermutlich vergessen hast warum du sie hattest.

 

[TaxiDriver12]

edit

 

[Quinix1]

So jetzt müsste es richtig sein:

#include <avr/io.h>
#include <avr/interrupt.h>

void warten(void) {
	while ( !(TIFR1 & (1 << OCF1A ) ) ) // Auf "Wecker" warten
	;
	TIFR1 |= (1 << OCF1A ); // Interruptflag löschen
}

int main(void) {

	OCR1A = 31250-1;	//2 sekunden

	TCCR1B |= (1 << WGM12); //turn on CTC mode
	TCCR1B |= (1 << CS12) | (1 << CS10);	//Prescalemode 101 -> 1024 Takt

	DDRB = ( 1 << PCINT5 ); //kontrolleuchte aktivieren
	DDRC = 0x3f; //alle LED aktivieren
	
	PCICR = 0x3f; //Pin Change Interrupt Control Register
	PCMSK0 = ( 1 << PCINT0 ) | ( 1 << PCINT2 ) | ( 1 << PCINT4 ); //taster aktivieren

	sei();

	while(1) {
		PORTB ^= ( 1 << PCINT5 );	//kontroll LED
		warten();		//blinken
	}
}

ISR(PCINT0_vect){
	
	if (PINB & ( 1 << PCINT0 )){						//if rechte Taste gedrückt
		PORTC |= ( 1 << ADC0D ) | ( 1 << ADC1D );			//2 rechte LED (ADC0D,ADC1D)
	}
	else{
		PORTC &= ~(1<<ADC0D);
		PORTC &= ~(1<<ADC1D);
		}
		
	else if (PINB & ( 1 << PCINT2 )){							//if  mittlere Taste gedrückt
		PORTC |= ( 1 << ADC2D ) | ( 1 << ADC3D );			//2 mittlere LED (ADC2D,ADC3D)
	}
	else{
		PORTC &= ~(1<<ADC2D);
		PORTC &= ~(1<<ADC3D);
	}
	
	else if (PINB &( 1 << PCINT4 )){							//if  linke Taste gedrückt
		PORTC |= ( 1 << ADC4D ) | ( 1 << ADC5D );			//2 linke LED (ADC4D,ADC5D)
	}
	else{
		PORTC &= ~(1<<ADC4D);
		PORTC &= ~(1<<ADC5D);
	}

 

[Vanilla2001]

Wir nähern uns der Sache.

Statt:

if (PINB & ( 1 << PCINT0 )){

würde ich eher

if (PINB & ( 1 << PINB0 )){

verwenden, da dich ja das Level des PIN0 von PortB interessiert.

Du kannst statt:

	PORTC |= ( 1 << ADC0D ) | ( 1 << ADC1D );

und

                PORTC &= ~(1<<ADC0D);
		PORTC &= ~(1<<ADC1D);

auch

 PORTC  |=     ( (1<<PINC0) | (1<< PINC1) );

und

 PORTC  &= ~ ( (1<<PINC0) | (1<<PINC1) );

verwenden. Die Funktion ist die gleiche, nur ist die zweite Version schneller zu begreifen und du siehst auch, dass du die gleiche Maske verwendest. Damit sind wir beim Thema Maske. Du könntest dann auch eine Maske definieren mit der du die LEDs steuerst, die bei den
Tasten gedrückt werden sollen. z.B.

#define BTN1_LED_MASK    ( (1<<PINC0) | (1<< PINC1) )
.
.
PORTC  |=   BTN1_LED_MASK ;
.
.
PORTC  &= ~BTN1_LED_MASK ;

Die Definition einer Maske macht man in der Regel bei 'C' in einer Header-Datei. Der Vorteil liegt daran, dass du die Konfiguration schneller ändern kannst, ohne alle Stellen im Code zu suchen.


Noch eine Sache. Überleg mal was passiert, wenn du mit deiner Code Variante zwei Tasten zur exakt gleichen Zeit drückst!

 

[kuddlmuddl]

Anfangs habe ich beim Thema mikrocontroller-Programmierung auch so gearbeitet. Aber als ich dann das erste mal die Hardware wechseln wollte/musste wurde mir klar wie nervig es ist so Plattform-spezifischen Code zu schreiben und wie sehr die lesbarkeit leidet. Stell dir vor du arbeitest danach 2 Jahre mit einem anderen Chip und willst dann etwas von deinen alten AVR Zeiten übernehmen.
Mach dir doch einen "HardwareAbstractionLayer" (=HAL) mit netten Funktionsnamen die dir helfen solche häßlichen Ausdrücke "PORTB ^= ( 1 << PCINT5 );" zu vermeiden. In dem HAL gibts dann Funktionen wie toggle, setOn, setOff, isSet, startTimer, bitOn, bitOff etc. Ich finde es immer viel netter lesbar wenn zB in deiner ISR nur steht

	if (isSet(rightButton)){
		setOn(led0d, led1d);
	}
	else{
		toggle(led0d, led1d);
		}
...

Mal davon abgesehen: Man sagt immer ISRs sollten möglichst kurz sein. Daher machen wohl viele als Member oder im schlimmsten Fall global
volatile bool isr1HasHappened = false;
in der ISR wird dann nur gemacht:
isr1HasHappened = true;
und der mainloop enthält eine Funktion checkISRFlags(); innerhalb derer dann sowas ähnliches steht:
if (isr1HasHappened)
{doIsr1Stuff(); isr1HasHappened = false;}
Falls die Interrupts schneller kommen können als die Abarbeitung in diesem Fall passiert und es dir wichtig ist, dass der Interrupt exakt so oft behandelt wird wie er auch eingetreten ist dann könntest du statt bool auch einen uintX_t verwenden, welcher statt auf true +=1 in der ISR gesetzt wird. Dadurch kann checkISRFlags() bei != 0 den Interrupt abbarbeiten. Dies wäre auch eine verbesserung zu deinem Code oben wobei deine ISR ja noch sehr schnell abgearbeitet werden wird.

 

[Quinix1]

edit

 

[kuddlmuddl]

Hab vergessen zu antworten:
Im Grunde war es das was du gepostet hattest - aber der check auf das Flag gehört imho VOR den call der processInterrupt Funktion. Sonst machst du zig tausend mal pro Sekunde den Funktionscall und springst idR wieder raus, weil das flag false ist anstatt den call garnicht erst zu machen. Aber das ist evtl auch geschmackssache.