Dicker Fehler in der RP6I2CmasterTWI.h der RP6Lib + Bugfix
Ich bin dabei die TWI-Funktion für Master/Salve und Multimaster umzuschreiben und dabei ist mir ein dicker Fehler in den RP6Libs aufgefallen. Er führt zu einem instabilen TWI System.
Es betrifft in der Datei RP6I2CmasterTWI.h die Zeile:
#define I2CTWI_isBusy() ((TWCR & (1<<TWIE)))
Dort wird durch Abfrage des TWIE Bit im TWCR geprüft ob die TWI Hardware frei ist bevor ein weiterer TWI-Befehl abgesetzt wird. Das TWIE Bit ist aber laut Datenblatt "nur" das TWI Interrupt Enable Bit. Die Abfrage ist recht unsinnig. Der Entwickler hat - weil er so laufend die Hardware überrannte - an allen möglichen und unmöglichen Stellen des Code Delays eingefügt. Durch die statischen Delays wird das Risiko vermidert, die Hardware zu überrennen aber es macht große Timingprobleme auf dem TWI Bus weshalb dieser instabil, langsam und manchmal arg unwillig ist.
Das ganze ist einfach zu fixen in dem man oben die Zeile durch folgende ersetzt:
#define I2CTWI_isBusy() ((TWCR & (1<<TWINT)))
Hier wird nun korrekt das TWINT Bit geprüft. Das Datenblatt sagt dazu:
"TWINT: TWI Interrupt Flag ; This bit is set by hardware when the TWI has finished its current job and expects application software response."
Weiterhin kann man dann auch alle Aufrufe von I2CTWI_delay in der RP6I2CmasterTWI.c entfernen. Als Folge läuft die TWI Hardware deutlich stabiler und schneller. Wer viel auf dem TWI Bus zu tun hat wird das schätzen wissen. Im Treiber sind noch mehr Bugs aber der erscheint mir wichtig genug um vorab eine Meldung zu machen.
Mehr dann vielleicht wenn meine Lib bzw. die Änderungen fertig sind.
EDIT: Also das mit dem "Delays entfernen" klappt nicht so ganz wie gedacht, bei der Aussage zum TWIE/TWINT bleibe ich aber.
LG RolfD
Liste der Anhänge anzeigen (Anzahl: 1)
Jetzt antworte ich mir schon auf meine eigenen Posts... tsts :)
Und dann das..
"Deine Nachricht darf nicht mehr als 20000 Zeichen enthalten."
:)
Aber es gibt ein guten Grund... für die Betatester hab ich was zu tun.
Ich betone BETATESTER... der Code ist noch nicht reif, das man ihn auf die Menschheit und insbesondere arglose RP6 Nutzer loslassen kann :)
Es folgen eine twi_target.h, eine include Datei um ein paar Dinge zu vereinfachen, hauptsächlich Präprozessordirektiven die es erlauben den Quelltext abhängig vom Zielsystem zu bauen. Sie muss also entsprechend geändert werden, da sie aber im Projektverzeichnis liegt und jedes Projekt eine eigene haben darf, spart man sich bissel arbeit. Dort gibts auch ein debug Flag welches die Ausgabe der Zustände am i2c Bis und Datenbytes ausgibt. Bei vielen Daten streikt allerdings das Konsolenterminal des Loaders...
Dann eine RP6I2CTWI.h, in der Hauptsache ein Verbund aus den beiden alten includes + paar Änderungen.
Dann die RP6I2CTWI.c als eingentliche Lib. Die alten Libs werden nicht includiert.
Meine Testumgebung ist ein RP6 mit einem modifizierten Slave Programm + einer i2c pcf8583 sowie einem aufgestecktem M32. Die M32 spielt mit dem Programm Example_08_I2CMaster Master, die PCF8583 spielt Slave für das Baseboard, man kann aber auch ein anderes i2c Device nehmen... lcd, ein SRF oder sonst was... Ich lasse die Base ein wenig in den Registern und RAM der Uhr spielen. Die Base als Slave (für die M32) ist also zugleich auch Master für das PCF8583. Ich katte noch keine Zeit mir ein richtiges Stress und Benchmarks system aufzubauen, dazu fehlt mir vor allem ein zweiter USB Adapter und ohne den ist debuggen wirklich im trüben fischen...
Alles in allem läuft das so auch so wie gedacht. Allerdings treten noch Probleme (wahrscheinlich Timing) auf und daher sage ich ganz deutlich: Das ist alles wirklich noch beta! Allerdings gibts in der Lib auch keine nennenswerten statischen Delays.
Die Änderungen im einzelnen zu beschreiben und zu begründen würde ein Buch füllen, ihr werdet euch also ggf. den Code anschauen müssen. Vielleicht spuckt "diff" was sinnvolles aus... keine Ahnung. Er ist zu 99% komapibel mit der alten Lib, es gibt allerding kleine Änderungen.
__I2CTWI_initMaster gibts nicht mehr und ein paar Variabelen die scheinbar gedacht waren um den ISR zu monitoren sind statisch gesetzt (waus auch zu problemen führen kann, da such ich noch, hat aber keine Priorität grade... Der Mastermode ist nur Master (und wenn man das in twi_target.h auch einträgt spart man einiges an Platz), der Slave Mode kann beides, Slave und Master. Sonst ist das meiste ähnlich zu beachten wie in den ursprünglichen Libs. Meinem Gefühl nach ist die Lib deutlich stabiler als die alte. Das würde ich aber gern auch von euch erfahren.
Bin für Fragen, Vorschläge, Verbesserungen usw. immer offen. Ich hoffe auf Feedback.
EDIT1: Im makefile nicht vergessen:
#SRC += $(RP6_LIB_PATH)/RP6common/RP6I2CslaveTWI.c
#SRC += $(RP6_LIB_PATH)/RP6common/RP6I2CmasterTWI.c
SRC += $(RP6_LIB_PATH)/RP6common/RP6I2CTWI.c
EDIT2:
Es spricht nichts dagegen, z.B. mit eigenen i2c Funktionen auf dem i2c Bus zu arbeiten, allerdings sollte man dann für die Zeit Interrupts unterbinden und die i2c Register (TWSR) überwachen. In jedem Fall muss man nach einem übertragenem Datenpaket eine Stop Condition setzen. Viele Slaves vertragen zwar den repeated Start ... also Start,Daten,Start,Daten,Start,Daten, usw., allerdings wird der Bus so nie frei und andere Master bekommen ggf. keine Gelegenheit zum senden. Wichtig für mehrere Master auf einem Bus ist also Start,Daten,Stop,Start,Daten,Stop usw... Es wäre auch ein Repeated Start durch Start,Daten,Start,Daten,Stop denkbar.. wenn man verhindern will das andere Master einem in die Devices schreiben wärend man noch im Zugriff ist. Aber ein Stop am Ende ist quasi Pflicht. Sinnvoller Weise müsste man ein RepStart nach einer gewissen Zeit unterbinden z.B. auch um nicht anfällig für Signalstörungen auf dem Bus zu sein.
Schreibt man eigene Funktionen und will die ISR nutzen, sollte man vor einem Start im TWCR mit bitset(TWI_statusReg,STR_isr); die ISR "reservieren" und nach einem Stop mit bitclr(TWI_statusReg,STR_isr); freigeben. bitset und bitclr sind gängige Macros:
#define bitset(var, bit) ((var) |= (1 << (bit)))
#define bitclr(var, bit) ((var) &= (unsigned)~(1 << (bit)))
Repstarts kann die Lib als Master derzeit noch nicht... bzw konnte sie glaube ich auch noch nie, bin ich mir jetzt nicht sicher. Steht aber auf meiner Todo. Sinnvoll wäre dies z.B bei Funktionen die in einem Rutsch ein Pointer setzen und dann lesen - wie in RAMs/EEPROMs üblich.
LG Rolf
Liste der Anhänge anzeigen (Anzahl: 1)
@SlyD oh.. auch nicht schlecht... da bin ich mit dem Prozessordatenblatt und den vielen halb implementierten Sources die im Web rumgeistern (incl. Atmels Sourcecode) ja noch gut dran :) Im Web zu googlen lohnt sich für mich nicht mehr.. da finde ich inzwischen meine eigenen Beiträge hier :) Interssanter Weise finden sich die besten Infos immer noch hier im RN-Wissen.
Hallo,
damit euch nicht langweilig wird, hab ich für die Betatester was.
Die Geschichte ist noch nicht stabil aber zumindest sollten sich Master und Slave unterhalten können. (bei mir tuen sie das :) )
Zunächst das modifizierte Testprogramm von Dirk:
Die Base/Slave, ich poste nur die main
Code:
int main(void)
{
initRobotBase();
setLEDs(0b111111);
mSleep(500);
setLEDs(0b000000);
I2CTWI_initSlave(RP6BASE_I2C_SLAVE_ADR);
while (true)
{
/* asks TWI for waiting Jobs, textblock may be a "task_slave" in rp6 c notation?
**
*/ if (I2CTWI_needAction()) {
/*
** yessssss ... doit!
*/
// get data from slave receiver mode register
controlbyte = I2CTWI_writeRegisters[0]; // Read control byte
I2CTWI_writeRegisters[0] = 0; // and reset it (!!!)
lowbyte = I2CTWI_writeRegisters[1]; // Read lowbyte
highbyte = I2CTWI_writeRegisters[2]; // Read highbyte
// copy data to slave sender mode register
I2CTWI_readableRegisters[0] = controlbyte; //copy controlbyte
I2CTWI_readableRegisters[1] = lowbyte; // copy lowbyte
I2CTWI_readableRegisters[2] = highbyte; // copy highbyte
// this is controlled by I2CTWI_needAction()
// if (controlbyte == CMD_SHOW_DATA) { //
cnt = ((highbyte << 8) | lowbyte); // Restore 16-bit value
#ifdef TEXT_OUTPUT
writeString_P("Received: "); // and show it
writeInteger(cnt, DEC);
#endif
if (!cnt) error_cnt = 0; // Reset error counter
else error_cnt++; // Error counter + 1
if (cnt != error_cnt) {
writeString_P(" ERROR!\n");
error_cnt = cnt;
}
#ifdef TEXT_OUTPUT
else writeChar('\n');
#endif
controlbyte = 0;
// }
/* signals Job as "done" and free the receiver for next Data
**
*/ I2CTWI_doneAction();
/*
** done!
*/
}
/*
** blink like fireworks, attract girls by danci'n circles or lying around an drink ... gear oil
*/
}
return 0;
}
Dann der Master auf der M32
Code:
int main(void)
{
initRP6Control();
initLCD();
I2CTWI_initSlave(12); // Initialize the TWI Module for Master operation
// with 100kHz SCL Frequency
// Register the event handlers:
I2CTWI_setTransmissionErrorHandler(I2C_transmissionError);
setLEDs(0b1111);
mSleep(500);
setLEDs(0b0000);
while(true)
{
if (!I2CTWI_isBusy()) {
cnt++; // TEST: COUNTER!!!
if (cnt > 255) cnt = 0;
buffer[0] = 0;
buffer[1] = CMD_SHOW_DATA;
buffer[2] = cnt;
buffer[3] = (cnt >> 8);
I2CTWI_transmitBytes(RP6BASE_I2C_SLAVE_ADR, buffer, 4);
mSleep(1000); //the slave works...
buffer[0] = 0; //reset buffer
buffer[1] = 0;
buffer[2] = 0;
buffer[3] = 0;
I2CTWI_readRegisters(RP6BASE_I2C_SLAVE_ADR, 0, buffer, 4);
if (cnt == ((buffer[2] << 8) | buffer[1])) {
#ifdef TEXT_OUTPUT
setCursorPosLCD(0, 0);
writeStringLCD_P("I2C has moved:"); // What was sent?
setCursorPosLCD(1, 0);
writeIntegerLCD(cnt, DEC);
#endif
} else {
setCursorPosLCD(0, 0);
writeStringLCD_P("Error!"); // What was sent?
}
//if (cnt==3) while(true); //countertrap
}
}
return 0;
}
Es hat sich in soweit etwas verändert, das der Master die Daten auf den Slave schreibt, dieser kopiert die Daten vom Schreib- ins Leseregister und prüft, eine Zeit lang später (hier 1 Sek. weil ich am debuggen bin) liest der Master die Daten zurück um sie mit dem Original zu vergleichen. Sinnvoll wäre hier ein Statusbyte einzuplanen das der Master abfragen kann. Dafür müsste der Master aber ins Leseregister schreiben da der slave nicht mitbekommt ob der Master liest. Ließe sich aber irgendwie anders einbauen. Ich vermute aber das man in Dirks Programm locker auf 1ms runter gehen kann, der Slave copiert ja nur bissel in der Gegend rum bis die Daten bereit sind. Hier wie in der Schreibfunktion zu nacken macht aber keinen Sinn da es den Master lesend blockiert. Das ist mit einem Statusbyte besser gelöst
Es sind also alle 4 Modi mit Slave Receive/Transmit und Master Receive/Transmit beteiligt. Die ISR sperrt nach einem schreibenden Durchgang auf den Slave weitere Schreibversuche durch NACKen. Erst wenn der Slave fertig ist schaltet er die ISR frei. I2CTWI_needAction() und insbesondere I2CTWI_doneAction() erledigen das.
Eine Abfrage von Statusbytes in den Registern erledigt sich damit, allerdings muss damit jede Schreibaktion vom Slave quitiert werden sonst wartet sich der Master einen Wolf.
Die aktuelle twi_target.h sieht bei mir so aus (hier von der Base):
Code:
/**/
#ifndef TARGET_H //Include once
#define TARGET_H
/* this define Hardware to selecting the right includes
** use one of them, never both
*/
#define M32
//#define BASE
/* this define TWI Modus to stip obsolete Slave code in Mastermode
** use one of them, never both
*/
//#define TWIMASTER
#define TWISLAVE
/*
** use LOWLEVEL I2C Functions without interupts too
*/
//#define TWILOW
/* this TWI define Speedsetup at 100 or 400 KHz Standart
** use one of them
*/
#define TWISPEED 100
/*
** use Generic calls, not supported yet
*/
//#define GENADR
/*
** if set, Base can use I2CLCD like LCD on M32
*/
//#define I2C_LCD
/*
** internal use, hide warnings
*/
#define UNUSED(x) ((void)(x)) //help optimizing
#define DEBUG
#include "RP6I2CTWI.h"
#ifdef BASE
#include <RP6RobotBase.h>
#ifdef DEBUG
#include <RP6RobotBaseLib.h>
#endif
#ifdef I2C_LCD
#include <RP6RobotBaseLib.h>
#include <lcd.h>
#define writeLCDCommand writeI2CLCDCommand
#define initLCD initI2CLCD
#define clearLCD clearI2CLCD
#define clearPosLCD clearPosI2CLCD
#define writeCharLCD writeCharI2CLCD
#define writeStringLCD_P writeStringI2CLCD_P
#define writeStringLengthLCD writeStringLengthI2CLCD
#define writeStringLCD writeStringI2CLCD
#define writeNStringLCD_P writeNStringI2CLCD_P
#define _showScreenLCD_P _showScreenI2CLCD_P
#define writeIntegerLCD writeIntegerI2CLCD
#define writeIntegerLengthLCD writeIntegerLengthI2CLCD
#define setCursorPosLCD setCursorPosI2CLCD
#define setlightLCD setlightI2CLCD
#endif
#else
#ifdef M32
#include <RP6Control.h>
#ifdef DEBUG
#include <RP6ControlLib.h>
#endif
#ifdef I2C_LCD
#include <RP6ControlLib.h>
#include <lcd.h>
#endif
#endif
#endif
#endif
Ihr werdet nicht alles brauchen, allerdings sind die Optionen im oberen Bereich schon interessant da sie ggf. Code einsparen helfen. Achtung, DEBUG ist Aktiv... unten stricke ich mir u.a. das i2c-lcd für die Base als normals M32 Display zurecht.... macht sich gut wenn man die gleichen Programme auf der Base UND der M32 testen will.
In der TWI hat sich viel getan, u.a. habe ich einige Variablen abgeschaltet, die versuchten die ISR zu monitoren. Sowas z.B.
/* don't use, obsolte, try I2CTWI_isBusy() instead
volatile uint8_t I2CTWI_genCallCMD;
volatile uint8_t I2CTWI_dataWasRead = 1;
volatile uint8_t I2CTWI_dataReadFromReg = 1;
volatile uint8_t I2CTWI_readBusy = 0;
volatile uint8_t I2CTWI_writeBusy = 0;
*/
Die Funktionalität (war eigentlich keine) ist mit den Action Befehlen und isBusy besser gelöst, der C Preprozessor hilft hier beim anpassen älterer Projekte ungemein ohne das man viel umschreiben muss. Allerdings funktioniert das jetzt so nicht mehr so einfach mit dem Slave Programm für die Base. Aber zum testen reicht Dirks Programm und Fabian wird den Slave sicher anpassen können. Sonst mach ich das irgendwann. Ich sehe jedenfalls nicht ein, Code zu pflegen der obsolet ist, nur weil er da ist. So viel Ram und Flash hat der RP6 auch nicht...
Evtl. mache ich das NACKen nach schreiben auf den Slave optional, derzeit kann man sich gut mit dem bedingungslosen Befehl I2CTWI_doneAction(); in der äusseren While Loop helfen wenn der Slave möglichst nicht nacken soll - allerdings werden dann halt die Register überschrieben. Die gencall Geschichte ist z.Z. komplett abgeschaltet, da konnte bisher eh nur ein Byte mit empfangen werden. Da hab ich noch was in der Todo stehen :) für irgendwann mal...
Das ganze System hat immer noch viele Macken, es ist noch nicht auf Multimaster umgestellt aber man kann aus dem Slavemode herraus zumindest Masterfunktionen ansprechen. Ein paar Macken sind leider systembedingt - z.B. werden zusammenhängende Befehlsketten wie Setzen einer Registeradresse und anschließendes Register lesen in 2 Zyklen ausgeführt was nicht nötig wäre, was aber z.B. dazu führt, das die ISR für kurze Zeiten "frei" ist obwohl sie eigentlich mitten in einer Transaktion steckt. Ein folgender Befehl kann hier die noch arbeitende ISR stören. Das scheint auch für Buserrors zu sorgen. Sowas ist unschön! Ob ich das beheben kann, weis ich noch nicht. Es stört mich auch, das in einer ansich Interruotgesteuerten Umgebung blockende Befehle stecken, das mindert den Sinn einer ISR doch gewaltig. Vermutlich stimmen auch einige Reaktionen auf die TWI-State Engine noch nicht, was wohl auch zu Buserrors führt, da bin ich aber dran... alles in allem also noch sehr Beta. Aber wieder mal ein Stück weiter hoffe ich.
Mein nächstes Ziel ist zum einen die Geschichte stabiler zu machen, aus irgendeinem Grund bleibt der Transport hängen, zum anderen möchte ich die Programme von Dirk zusammen legen, zu einem "Masterslave" so das sich Master und Slave des jeweiligen Boards abwechseln und gegenseitig Daten zuwerfen und prüfen. Also bidirektionaler/intermitierender Master/Slave Betrieb auf Base und M32.
Mir erschließt sich noch nicht, welch tieferer Sinn sich dahinter verbirgt, das man z.B. mit Readregisters nur Speicherplätze ab 1 im Buffer lesen, aber mit anderen Funktionen ab 0 beschreiben kann. Sowas werde ich wohl auch ändern wenns keine großen Einsprüche gibt.
Benutzt man die Geschichte nur im Mastermode mit nur einem Prozessor (per twi_target einstellen), kann man den Slavecode komplett einsparen, benutzt man auch den Slave, hat man beide Modi zur Auswahl. Die default Werte für
#define I2CTWI_SLAVE_WRITE_REGISTERS 16
#define I2CTWI_SLAVE_READ_REGISTERS 48
sind etwas zu hoch gewählt, hier würden im normalen Slaveprogramm auch 8/32 reichen wenn man ein paar Byte sparen will. Ich habs aber mal so gelassen.
EDIT:
Und wieder eine kleine Änderung in der RP6I2CTWI.c
Um die Buffer gehören Präprozessorabfragen zum TWISLAVE, keine funktionelle Änderung aber es spart Platz im Mastermode. In der nächsten Fassung ist das mit drin. In der .h war es schon eingetragen.
#ifdef TWISLAVE
uint8_t I2CTWI_readableRegisters[I2CTWI_SLAVE_READ_REGISTERS];
volatile uint8_t I2CTWI_writeRegisters[I2CTWI_SLAVE_WRITE_REGISTERS];
#endif
Dann noch was zum NACKen und Buffergrenzen überfahren, es wäre z.Z. wohl denkbar, das der Master hinter die Buffergrenze liest was der Slave nacken müsste worauf hin der Master abbrechen soll. Ob das klappt weis ich nicht, muss ich noch testen. Ich überlege sowieso, alternativ 1 R+W oder 2 R/W Registersets per twi_target.h anzubieten, das macht den Slave universeller. Und demnächst wirds zusätzlich ein Satz ISR-lose Funktionen geben, ähnlich der von Peter Fleury oder der AVR-LIBC aber angepasst an die Abläufe und Vorgaben der aktuellen TWI Steuerung.
EDIT: Hm.. ich glaube da stimmt was noch nicht... der Master hängt sich auf laut LEDs.. aber morgen wirds wieder ne Beta Version geben denk ich mal. NACK als Option ist drin, die Lowlevelfunktionen sind schon drin, jede Menge Code cleanup über #ifdef...
LG Rolf
