Diskussion:TWI Slave mit avr-gcc - Versionsgeschichte

RedBaron am 8. August 2012 um 09:58 Uhr

2012-08-08T09:58:30Z

Uwegw: /* Kelinigkeiten */

2010-07-02T14:39:19Z

‎Kelinigkeiten

Blacky_alt: Kelinigkeiten

2010-06-05T10:22:45Z

Kelinigkeiten

Uwegw am 12. April 2010 um 09:47 Uhr

2010-04-12T09:47:59Z

Betafrank: Kleiner Vorschlag

2007-05-23T14:18:08Z

Kleiner Vorschlag

Neue Seite

← Nächstältere Version		Version vom 8. August 2012, 09:58 Uhr
Zeile 56:		Zeile 56:

	Christian</nowiki>		Christian</nowiki>
		+
		+	Hallo Uwe,
		+
		+	ich setze mich gerade mit deinem Code in "twislave" auseinander. Ich habe folgende Anmerkungen:
		+
		+
		+	Der Puffer wird mit "volatile uint8_t i2cdata[i2c_buffer_size+1];" deklariert. Warum das "+1" bei der Größe? Du fragst überall die Puffergrenze mit "...<i2c_buffer_size" ab. Damit würde "i2cdata[i2c_buffer_size]" nie angefasst werden.
		+
		+	Der Puffer wird sowohl in der .h- als auch in der .c-Datei deklariert. Die Deklaration in der .h-Datei wäre ausreichend.
		+
		+
		+
		+	'''init_twi_slave:'''
		+
		+	Hier steht "TWCR &= ~(1<<TWSTA) \| (1<<TWSTO);". Durch ~(1<<TWSTA) sind bereits alle Bits bis auf TWSTA auf 1, also auch TWSTO. "\| (1<<TWSTO)" wäre also unnötig. Meinst du etwas anderes?
		+
		+	Danach folgt "TWCR \|= (1<<TWEA) \| (1<<TWEN) \| (1<<TWIE);". Diese Bits wurden bereits in der vorhergehenden Anweisung gesetzt. Diese Zeile müsste überflüssig sein.
		+
		+	sei() in der Initialisierungsroutine ist nicht gut! Wenn mehrere Hardwarekomponenten zu initialisieren sind (z.B. USART), kann es sein, dass diese bereits im unitialisierten Zustand oder bei der Initialisierung Interrupts auslösen. sei() gehört auf jeden Fall ins Hauptprogramm. Ggf. wäre eine Kommentarzeile mit dem Hinweis auf sei() angemessen.
		+
		+
		+	'''ISR:'''
		+	...
		+	{ buffer_adr = 0; //Adresse auf Null setzen. Ist das sinnvoll? TO DO!
		+	...
		+
		+	Ich denke, dass ist nicht sinnvoll. Besser ist es, buffer_adr auf 0xFF stehen zu lassen. Das setzen auf 0 bedeutet, dass bei einer offensichtlich fehlerhaften Ansteuerung ggf. gültiger Speicherbereich überschrieben würde. Das sollte nicht sein.
		+
		+	Die folgende Codefolge taucht in ähnlicher Form mehrfach auf:
		+	{ if(buffer_adr < i2c_buffer_size)
		+	{ i2cdata[buffer_adr] = data; //Daten in Puffer schreiben
		+	}
		+	buffer_adr++; //Buffer-Adresse weiter zählen für nächsten Schreibzugriff
		+	...
		+
		+	Das bedingungslose Hochzählen des Pufferindex bedeutet, dass er bei genügend langen Übertragungssequenzen zunächst zu 0xFF (also ungültig) wird und dann zu 0x00 überläuft und wieder in gültige Indexbereiche kommt. Die Sequenz muss noch nicht einmal sehr lang sein, da man den Index durch die Addressierung kurz vor das Ende des Puffers positionieren kann. Besser wäre
		+
		+	{ if(buffer_adr < i2c_buffer_size)
		+	{ i2cdata[buffer_adr++] = data; //Daten in Puffer schreiben
		+	}
		+	...
		+
		+	Eigentlich müsste man auch noch berücksichtigen, dass wenn der Index 254 errreicht wurde, weder weiter hochgezählt werden darf (würde zu 0xFF führen) noch ein weiteres Mal Daten gespeichert werden dürfen. Einfacher ist es, man begrenzt die max. Puffergröße auf 253 Byte.
		+
		+
		+	case TW_ST_DATA_ACK: //0xB8 Slave Transmitter, Daten wurden angefordert
		+	if (buffer_adr == 0xFF) //zuvor keine Leseadresse angegeben!
		+	{ buffer_adr = 0;
		+	}
		+
		+	Auch hier: Offensichtlich ein Fehlerzustand, der zu gültigen Daten führt. Wenn man diesen Fehler macht, sucht man ziemlich lange nach der Ursache. Kurz danach wird auf Datenanforderung außerhalb des Pufferbereichs mit dem Senden von 0x00 reagiert. Dies sollte man auch diesem Fehler machen. Anweisung also am besten streichen.
		+
		+	Apropos "lange suchen"... Ich bin schon etwas älter. Da könnte der Ausdruck "totsuchen" zur Realität werden :-)
		+
		+	Viele Grüße
		+	RedBaron

@@ Zeile 29: / Zeile 29: @@
 I2C_reg_Schreibschutz in twislave.h definieren ich denke das war als array gedacht?
 I2C_reg_Schreibschutz sollte in der ini routine initialisiert werden... - dabei kann man gleich den Speicherbereich vorbeschreiben.
 Ich würde auch die Interrupt aktivierung nicht in der Ini routine machen, sondern lieber ausserhalb, denn normalerweise müssen ja noch andere Sachen vorbereitet werden.

← Nächstältere Version		Version vom 5. Juni 2010, 10:22 Uhr
Zeile 16:		Zeile 16:

	:: Ok, habe es jetzt korrigiert, aber noch nicht selbst getestet... Solle laut Datenblatt aber korrekt sein. mfg --[[Benutzer:Uwegw\|uwegw]] 11:47, 12. Apr 2010 (CEST)		:: Ok, habe es jetzt korrigiert, aber noch nicht selbst getestet... Solle laut Datenblatt aber korrekt sein. mfg --[[Benutzer:Uwegw\|uwegw]] 11:47, 12. Apr 2010 (CEST)
		+
		+	== Kelinigkeiten ==
		+
		+	Hallo,
		+
		+	ich habe den Code für ein Projekt vor 2 Jahren benutzt, jetzt habe ich nach dem Code gesucht um ein Update zu machen.
		+
		+	Die Oben genannten Probleme hatte ich damals auch - jetzt beim ersetzen des Moduls fällt mir folgendes auf:
		+
		+	- I2C_reg_Schreibschutz ist nicht definiert und kommt im Code auch nur einmal vor. Ich denke mal, dass es dafür sorgen soll, dass richtig adressiert worden ist, oder?
		+
		+	I2C_reg_Schreibschutz in twislave.h definieren ich denke das war als array gedacht?
		+	I2C_reg_Schreibschutz sollte in der ini routine initialisiert werden... - dabei kann man gleich den Speicherbereich vorbeschreiben.
		+
		+	Ich würde auch die Interrupt aktivierung nicht in der Ini routine machen, sondern lieber ausserhalb, denn normalerweise müssen ja noch andere Sachen vorbereitet werden.
		+
		+	Mein Vorschlag für die Ini routine:
		+
		+	void init_twi_slave(uint8_t adr)
		+	{
		+	uint8_t i=0;<nowiki>
		+	TWAR= adr; //Adresse setzen
		+	TWCR &= ~(1<<TWSTA)\|(1<<TWSTO);
		+	TWCR\|= (1<<TWEA) \| (1<<TWEN)\|(1<<TWIE);
		+	buffer_adr=0xFF;
		+	for(i=0;i<i2c_buffer_size;i++)
		+	{
		+	i2cdata[i]=0;
		+	I2C_reg_Schreibschutz[i]=0;
		+	}
		+	}
		+
		+
		+	Warum definierst Du i2cdata zweimal? Einmal im header reicht ....
		+
		+	Ansonsten vielen Dank, das ist sehr hilfreich...
		+
		+	Christian</nowiki>

← Nächstältere Version		Version vom 12. April 2010, 09:47 Uhr
Zeile 14:		Zeile 14:

	Gruß Frank		Gruß Frank
		+
		+	:: Ok, habe es jetzt korrigiert, aber noch nicht selbst getestet... Solle laut Datenblatt aber korrekt sein. mfg --[[Benutzer:Uwegw\|uwegw]] 11:47, 12. Apr 2010 (CEST)