Dicker Fehler in der RP6I2CmasterTWI.h der RP6Lib + Bugfix

[RolfD]

@Slyd die Idee mit dem "zwar received aber noch nicht verarbeitet" werde ich genauer prüfen, zur Zeit hab ich den Eindruck das die Zeichen wirklich verloren gehen - das lässt sich aber genau rausfinden. Danke für den Tip.

@Betatesters, ein kleines update in RP6I2CTWI.h. ich hatte es schon oben editiert aber hier noch mal:
Bitte
#define I2CTWI_isBusy() ((TWCR & (1<<TWINT)))
durch
#define I2CTWI_isBusy() (bit_is_set(TWI_statusReg,STR_isr))
ersetzen.
Hintergrund ist, das TWINT Bit spielt für die Übertragung eines kompetten "Satzbaus" im TWI (Start Daten Stop) keine Rolle, eine Aussage darüber ob die TWI frei ist gibt hingegen das STR_isr in TWI_statusReg (abhängig von der Stop condition) , folglich muss das geändert werden. Die (blockierenden) Lowlevelfunktionen warten eh auf das STR_isr aber um festzustellen ob ein Task Rechenzeit braucht - wie in dem Master testprogramm - sollte es natürlich die richtigen Bedingungen abfragen.
Das TWINT spielt in der ganzen Übertragung übrigends garkeine Rolle mehr da wir nicht mehr das TWI als Registerset pollen sondern wirklich nur auf Interrupts reagieren und in den Lowlevelfunktionen die TWI Übertragung als "Satz" sehen. Damit werden u.a. unzulässig fehlende Stops unterbunden. Ein deutliches Zeichen das es vorran geht. Im Slavemode kann ein eintreffendes Byte die ISR übrigends auch blockieren so das verhindert wird, das ein Mastertask aktiv wird wärend Daten reinkommen... da arbeite ich aber grade dran. Das wird auch das counten/vergleichen in Dirks Slave Programm erleichtern bzw. die dort aufgeworfenen Fragen klären helfen.
LG RolfD

[Fabian E.]

So, ich habe dann jetzt mal auch die Base auf die neue Library umgestellt und das hat sehr viele Fehler produziert.

• Es waren einige Änderungen am Code nötig (Umbennungen)
• Mein Verbidnungsselbsttest zeigt das gleiche, wie das manuelle Setzen der RP6 LEDs:
  etwa 80% der Befehle kommen nicht richtig an. Das ist etwas seltsam...
  Die Befehle kommen irgendwie dann beim nächsten Befehl erst an...
  Beispiel: Ich setzte beim RP6 LED1 --> nichts passiert. Ich setzte danach LED2 --> LED1 ist dann an und LED2 aus.
  So geht das immer weiter...
  Mit dem normalen Baseprogramm ohne deine Lib geht alles normal.
• Mein I2C Stresstest gibt mit der abgeänderten Base von 2400 Versuchen 2400 Fehler aus.
  Auch in diesem Fall werden die LED zwar gesetzt, aber scheinbar zu spät.
• Das mit dem Ringbuffer auf dem Slave hätte ich jetzt spontan auch vorgeschlagen...
  Wie groß der sein muss wäre dann zwar etwas Trial&Error, aber eigentlich müsste es damit besser klappen.

[RolfD]

Hallo Fabian,
die Umbenennungen dürften nur I2CTWI_readRegisters und I2CTWI_readableRegisters betreffen, und dazu kann ich wenig. Wenn Du andere Umbenennungen hattest, sag mir bitte welche. Siehe auch:

* fix: I2CTWI_readRegisters -> I2CTWI_readableRegisters / double use of name

Ich hatte nur die Möglichkeit, das Array von I2CTWI_readRegisters im Slavecode in I2CTWI_readableRegisters umzutaufen oder ich hätte der Mastercode Funktion I2CTWI_readRegisters einen anderen Namen geben müssen... ich hab mich für erstes entschieden... müsst ihr nun mit leben
Natürlich muss man im eigenen Slave code auch die Registers umbenennen aber das macht der PN mit 3 Tasten ... quasi... allerdings sollte das besser dokumentiert sein, geb ich zu. Ich hatte es aber weiter oben auch schon mal gesagt...

Das mit dem Ringbuffer... nun die Register im Slavemode sind quasi ein Ringbuffer nur das der Pointer eben nicht im Kreis läuft sondern bei Verbindungsbegin üblicher Weise gesetzt wird (wie der Adresspointer im EEPROM). Wird über die Register hinaus gelesen, schickt der Slave bisher ein Nack zum Master. Allerdings hab ich das noch nicht 100% debugt ob das aufs Byte genau stimmt. Ich kann natürlich auch bei Ende der Register den Zähler einfach zurück auf Anfang stellen was einem Ringbuffer gleich komt. Die Frage ist nur: Nutzt das was? Das überlesen/überschreiben hinter die Registergrenzen ist in jedem Fall ein klarer Verstoß gegen Konventionen und vor allem ein Fehler in der Nutzung des Treibers.. nicht des Treibers selbst, daher bisher NACK. Der alte Treiber hat gnadenlos Zugriffe hinter die Registergrenzen zugelassen... was der z.B. mit dem Stackheap dann anstellt... das möcht ich garnicht mehr so genau wissen Der Master sollte allerdings auch mitkriegen wenn die Registergrenze erreicht ist und entsprechend reagieren (Fehlermeldung). Siehe auch:

* fix: Register/Buffer overruns

Das Problem mit dem Stresstest kann ich nicht beurteilen so lange ich den Quellcode dazu nicht habe. Würd mich aber wie Dirks Programm interssieren.

Zum Problem mit den verzögerten Übertragungen, ich werde die Lib die Tage umfangreich testen, ich halte es auch hier für möglich das der Fehler nicht im TWI Treiber zu suchen ist. Aber auch hier muss ich sagen: Abwarten... ich werde mich um alles kümmern was hier an Problemen aufschlägt.

Bevor Du an der Remotrol Software Anpassungen machst, sollten wir also zunächst dein Stesstest ans rennen kriegen, das ist ggf. auch übersichtlicher. Wenn Dein Stresstest jedes mal systematisch gegen die Registergrenze klatscht, wäre es z.B. kein Wunder wenn der 2400 Fehler ausgibt. Vielleicht gebe ich das NACK aber auch ein Byte zu früh. Zeig mir am besten mal die Quellen für den Stresstest.
LG Rolf

[Fabian E.]

Genau die Umbenennungen meinte ich, war auch nicht als Kritik oder so gedacht, sondern nur für andere Leute die das hier lesen sollten.

Ringbuffer...:So wie ich das jetzt verstanden habe, geht es doch darum, dass die Daten ankommen aber eben nicht verarbeitet werden, weil schon wieder ÜBERSCHRIEBEN...
Wenn man aber einfach alle ankommenden Daten irgendwie in einen Ringbuffer speichert geht zumindest nichts verloren.
Wie man diese Daten dann wieder sinnvoll auswertet, ist etwas anderes.

Du sagst was vom umbennenen der Register? Wie genau meinst du das?

Quellcode zum Stresstest kommt nachher noch.

Was meinst du mit, dass "der der Fehler nicht im TWI Treiber zu suchen ist"? Wo denn sonst? Mit der alten Lib klappt das perfekt

Und Remotrol ändere ich natürlich nicht, ich habe mir einen Testbuild dafür gemacht.

[Fabian E.]

So hier Code des Tests, der sehr viele Fehler bringt:

void TestI2CLEDs(void)
{
	TestStart("I2CLED Test"); //Unwichtig, dient nur der Kommunikation mit Remotrol
	uint8_t testResult = 1;
	
	uint8_t runningLight = 1;
	uint16_t cnt;
	uint16_t errorcount = 0;
	for(cnt = 0; cnt < 2400; cnt++) //"400 Durchläufe
	{
		writeIntegerLength(cnt,DEC,3);  //Aktuelle Werte ausgeben
		writeChar(':');
		writeIntegerLength(runningLight,DEC,3);
		writeChar(',');
		writeChar(' ');

		setRP6LEDs(runningLight);   //RP6 LEDs über I2C setzen
		//mSleep(50);                     //Hierran scheitert es prinzipiell, es wäre aber sehr schön, wenn man solche Delays nicht brauchen würde
		I2CTWI_transmitByte(I2C_RP6_BASE_ADR, 29);       //Aktuelle LEDs des RP6 wieder auslesen
		uint8_t result = I2CTWI_readByte(I2C_RP6_BASE_ADR);
		if(result != runningLight) 
		{
			writeString_P("\nTWI TEST FEHLER!\n");
			writeString_P("Value: ");
			writeInteger(runningLight,DEC);
			writeString_P(" - Result: ");
			writeInteger(result,DEC);
			writeString_P("\r\n");
			errorcount++;
			testResult = 0;
		}
		runningLight <<= 1; 
		if(runningLight > 32) 
			runningLight = 1;
	
		if((cnt+1) % 6 == 0) writeChar('\n');
		
		if(cnt % 20 == 0)//Unwichtig, dient nur der Kommunikation mit Remotrol
		{
			SerialHeartBeat();
		}
		
		//mSleep(100);
	}
	if(!testResult)
	{
		writeString_P("Anzahl Fehler: ");
		writeInteger(errorcount, DEC);
		writeString_P("\r\n");
	}
	setRP6LEDs(0);
	
	TestEnd(testResult);//Unwichtig, dient nur der Kommunikation mit Remotrol
}

Das Problem an sich ist klar... Er überrent halt dauerhaft die Hardware.
Es wäre aber prinzipiell sehr schön, wenn man die statischen Delays hier weglassen könnte.
Dann wäre die Lib perfekt

[RolfD]

Genau die Umbenennungen meinte ich, war auch nicht als Kritik oder so gedacht, sondern nur für andere Leute die das hier lesen sollten.

Ne als Kritik fasse ich das auch nicht auf, es gab ja einen guten Grund das umzubennen.... ich mach sowas nicht aus Langeweile

Ringbuffer...:So wie ich das jetzt verstanden habe, geht es doch darum, dass die Daten ankommen aber eben nicht verarbeitet werden, weil schon wieder ÜBERSCHRIEBEN...

Das hatte SlyD gemutmaßt.. ja.. Tatsache ist aber, das die Empfangenen Daten noch in der ISR in das Registerarray ... also quasi der "Ringbuffer" geschrieben werden... weshalb ich auf seinen Post auch sagte: "zur Zeit hab ich den Eindruck das die Zeichen wirklich verloren gehen - das lässt sich aber genau rausfinden."
Es können _eigentlich_ keine Zeichen in der ISR verloren gehen - es sei denn, es verhindert was die Ausführung der ISR. Wenn die ISR aber geblockt wird nutzen auch 100 buffer nix weil bereits ein Buffer genutzt wird... die TWI Hardware müsste ein Hardwarebuffer haben.. wie ein UART... is aber nich

Es wäre aber auch möglich, das der Kontrolltask ein empfangenes Zeichen schlicht und einfach nicht mitbekommt .. das Programm von Dirk muss ich mir darauf hin noch mal ansehen. Es wäre zumindest eine Möglichkeit die SlyD auch ansprach.

Du sagst was vom umbennenen der Register? Wie genau meinst du das?

Siehe oben ->
* fix: I2CTWI_readRegisters -> I2CTWI_readableRegisters / double use of name
Mit "Register" meine ich hier die Slaveregister der Base Software. Sollte jetzt klar sein.

Quellcode zum Stresstest kommt nachher noch.

Super, danke Dir.

Was meinst du mit, dass "der der Fehler nicht im TWI Treiber zu suchen ist"? Wo denn sonst? Mit der alten Lib klappt das perfekt
Und Remotrol ändere ich natürlich nicht, ich habe mir einen Testbuild dafür gemacht.

Ich formuliere mal vorsichtig... vielleicht ist Dir ein eigener Fehler mit der ALTEN Lib nur nicht aufgefallen? *grins ... perfekt ist relativ... nein.. noch ist es zu früh um andere Software zu debuggen, lass uns erst mal klar definierte Bedingungen wie Testprogramme usw. zum laufen bringen.

LG RolfD

[Fabian E.]

Was meinst du mit, dass "der der Fehler nicht im TWI Treiber zu suchen ist"? Wo denn sonst? Mit der alten Lib klappt das perfekt
Und Remotrol ändere ich natürlich nicht, ich habe mir einen Testbuild dafür gemacht.

Ich formuliere mal vorsichtig... vielleicht ist Dir ein eigener Fehler mit der ALTEN Lib nur nicht aufgefallen? *grins ... perfekt ist relativ... nein.. noch ist es zu früh um andere Software zu debuggen, lass uns erst mal klar definierte Bedingungen wie Testprogramme usw. zum laufen bringen.

Das kann natürlich sein, aber wenn ich einfach nur das alte Hexfile-flashe klappt alles so wie ich das will. Da da trotzdem Fehler sein können, die ich einfach nicht sehe ist klar =)

Aber der Fehler mit der neuen Lib ist so offensichtlich, das hängt auf jedenfall irgendwie zusammen
Ich tausche ja immer nur die Base aus. Entweder die mehr oder weniger originale I2C-Slave aus den Beispielen oder eben genau das nur mit der neuen Lib.

Achja, so zur allgemeinen Information: Die M128 unterstützt einfach mal schnell auch kein Multimaster. Soll aber wohl relativ leicht umzustellen sein, laut dem Entwickler.

[SlyD]

@Fabian:
Jo korrekt mit dem Ringbuffer könnte man das Überschreiben durch den Master vermeiden...
@RolfD: ... der Schreibpointer darf natürlich NICHT vom Master kontrolliert werden sondern muss vom Slave verwaltet werden.

Ein Ringbuffer hat einen Schreib- und einen Lesepointer. Der Schreibpointer wird ausschließlich in der ISR gesetzt, der Lesepointer ausschließlich in der Anwendung (in Library Funktionen).

Um den Code einfach zu gestalten könnte man z.B. einen Ringbuffer mit einem 2D Array anlegen (oder 1D Array und jeweils entsprechend die Indizes um +n Bytes erhöhen).
Z.B. 16 Einträge für Schreibzugriffe vom Master mit jeweils max. 8 Bytes - das reicht für die üblichen Anwendungen wohl aus.


> Registerarray ... also quasi der "Ringbuffer" geschrieben werden...

Nene das hat gar nix mit einem Ringbuffer zu tun - s. Funktionsweise oben bzw. schau Dir den UART Code an.

Der ganze Inhalt des Arrays ist bei beginn JEDES schreibzugriffes sofort ungültig wenn man es für die Übertragung von zusammenhängenden Kommandos(!) verwendet.
Man kann natürlich für bestimmte Funktionen einfach fest sagen Array[23] ist für die LEDs, Array[42] ist für Geschwindigkeit links o.ä. dann braucht man keine Befehle wie beim derzeitigen Slaveprogramm und kanns direkt setzen.
Geht übrigens auch beides in Kombination mit dem bisherigen Slave Code.
Ganz geschickt könnte mans kombinieren indem man sagt die ersten 8 Bytes werden mit dem Ringbuffer für Kommandos gepuffert und die anderen kann man direkt wie normale Register beschreiben - ohne Buffer.


Nochmal anders gesagt falls das hier noch ein Missverständnis sein sollte:
Bislang ist in der Lib ja nur ein Haufen "Register" vorgesehen. Wie bei normalen I2C Slaves auch. Das die zufällig in einem gemeinsamen Array liegen hat nur den Grund, dass es so effizienter ist das in der ISR zu beschreiben. Wenn man den Haufen Register als Buffer auffassen möchte hat der eben nur die Tiefe 1 - kein Ring kein gar nix.

Daher sind die Pausen nach Befehlen notwendig damit der Slave Zeit hat drauf zu reagieren. Die kann man natürlich auch sinnvoll für andere Dinge verwenden nicht nur mSleep... wird z.B. bei den Beispielen mit den Ultraschallsensoren gemacht die brauchen ja z.B. typ. 65ms für ein Ping Kommando.


MfG,
SlyD

[RolfD]

@Fabian Ok was du meinst ist also die Sequenz:

I2CTWI_transmitByte(I2C_RP6_BASE_ADR, 29); //Aktuelle LEDs des RP6 wieder auslesen
uint8_t result = I2CTWI_readByte(I2C_RP6_BASE_ADR);

Also Pointer im Registersatz auf das 28'te byte (Arrays beginnen bei 0) setzen und nächtes Byte (29) lesen der LEDs? Das knallt? hm...
Mal abgesehen davon, das man das 1te Byte auf Position 0 so nie adressieren kann sollte das eigentlich gehen. TWI definiert:
#define I2CTWI_SLAVE_WRITE_REGISTERS 16
#define I2CTWI_SLAVE_READ_REGISTERS 48
... also genug Platz um nicht an die Registergrenze zu klatschen... hm hm..

Kannst du mal deine setRP6LEDs funktion noch posten? Dann bau ich mir das mal hier nach.... Danke.

@SlyD
Ich glaube wir Missverstehen uns...

@RolfD: ... der Schreibpointer darf natürlich NICHT vom Master kontrolliert werden sondern muss vom Slave verwaltet werden.

Der Schreibpointer wie auch der Lesepointer MUSS vom Master kontrolliert werden damit der Slave weis welches Register gemeint ist. Da die ISR bereits die Daten direkt in die Register schreibt, nutzt ein extra "RingBuffer" (dessen Pointer wie in einer UART natürlich allein der Slave zu verwalten hätte) nichts. Mir ist der Unterschied zwischen einer Pipe und einem Array schon klar...

Du verwendest als Beispiel selbst in "case TWI_SRX_ADR_DATA_ACK:"

current_register = TWDR;

und damit setzt du den Schreib/Lesezeiger im Registerset beim folgenden Access. Das mit den Buffern habe ich garnicht verändert sondern benutze da zu 100% dein Code Der Code von RP6Base_I2CSlave.c wie auch vom alten TWI benutzt soweit ich das sehen kann auch keine unabhängigen Buffer oder Pointer für Schreiben und Lesen. Das war vielleicht mal geplant aber umgesetzt ist es in der alte TWI steuerung nicht. Vielleicht kommt das mal noch auf meine Todo Liste, mal sehen.

Würde ich (bzw. Du damals) die Daten nicht sofort in die Slaveregister schreiben, wäre ein Ringbuffer natürlich das Mittel der Wahl... keine Frage, und dann stimmt natürlich alles was Du gesagt hast. Wir reden aber beim TWI immer noch über Registeraccess und NICHT über UART/ISO-OSI

Allerdings sehe ich grade, das ich evtl. die Initialiierung von current_register = 0; in case TWI_STX_DATA_NACK: verschlampt habe... wobei als nächstes ein START kommen muss und da wird sie dann initialisiert... *seufz .. ich muss da noch mal durch...

LG Rolf

[SlyD]

Der Schreibpointer wie auch der Lesepointer MUSS vom Master kontrolliert werden damit der Slave weis welches Register gemeint ist.

Lötzinn :P
Schau Dir erstmal an wie ein Ringbuffer funktioniert (google).

Der Master adressiert zwar welche Register er schreiben will, das hat mit dem Ringbuffer aber NIX zu tun. Die "Register" sind nur die Daten die in den Ringbuffer reinkommen. Ein Eintrag = Zustand ALLER Register. Daher macht das auch nur für einen kleinen Teil aller Register sinn - z.b. die ersten 8 der Master muss dann immer bei Register 0 anfangen damit das richtig initialisiert ist und keine alten Werte enthält - wird ja in den Beispielen sowieso schon so ähnlich gemacht.
Wird die Übertragung beendet, ist das ein neuer Eintrag im Buffer, der Slave zählt dann den Schreibpointer hoch.
Schreibt der Master mehr Bytes als 8 oder adressiert er direkt die Bytes >7 dann wird nix gebuffert und direkt in die entsprechenden Register geschrieben.

Der Buffer wird also selbstverständlich vom Slave gesteuert - der Schreibpointer wird halt in der ISR gesetzt wenn neue Daten ankommen hat aber nix damit zu tun welches Register der Master schreibt.
Adressiert der Master z.B. Byte 0 bis 7 wird der Schreibpointer vom Slave Code in der ISR um 1 erhöt (neuer Eintrag).
daten[schreibpointer_vom_ringbuffer][Registernummer_vom_Master]

daten[0][0] = Eintrag 0 im Buffer, Register Nummer 0
daten[0][1] = Eintrag 0 im Buffer, Register Nummer 1
daten[0][2] = Eintrag 0 im Buffer, Register Nummer 2
...
daten[1][0] = Eintrag 1 im Buffer, Register Nummer 0
...

Die "Register" sind die Daten im Buffer das könnte aber auch was ganz anderes sein.


Der LESEpointer muss sowieso vom Slave gesteuert werden - nur der weiss ja wo er gerade mit der Abarbeitung der ganzen angekommenen Daten ist.

Klar soweit?


> Du verwendest als Beispiel selbst in ...

grrrrrr ich sage doch nicht was der fast 4 Jahre alte Code macht ich sage wie man es richtig machen MÜSSTE sollte hätte könnte wenn man einen Ringbuffer haben hätte wollen...
Ein Ringbuffer ist weder in Deinem noch in meinem Code drin

MfG,
SlyD

EDIT:
Ich glaub Du hast Deinen Beitrag grad nochmal editiert während ich das hier geschrieben hab? Naja egal.