Fehler bei der SPI Programmierung ?

[Marvin H. (memtest)]

Hallo liebes Forum,

ich wollte mal einen Playstation 2 Controller mit meinem AtMega256RFR2 
X-Pro
ansprechen.
Die Programmierung der SPI ist ja eigentlich nur ein Ding von 5 Minuten, 
aber irgendwie scheint es nicht zu funktionieren.

Ich habe die zu sendenden Komandos und die SPI Einstellungen von dieser 
Seite hier:
http://store.curiousinventor.com/guides/PS2/

Das Problem ist jedoch folgendes, ich sende dem Gamepad die benötigten 
Bytes (Bsp. die beim Polling also 5 stk.) ,aber die Antwort entspricht 
nicht der von der Seite.
Es kommt immer 0x01,0x42,0x00,0x00,0x01,0x00 zurück.
Weiterhin finde ich es sehr verwunderlich, dass es offensichtlich keine 
Rolle spielt ob ich Chip-Select verwende. Normalerweise liegt die 
CS-Leitung ja auf High und wird bei Auswahl des Slaves auf Low gezogen.
Nur bekomme ich die gleichen Antworten sogar wenn ich die CS-Leitung 
abziehe.

Aber hier erstmal mein SPI Code:

 
void SpiInit(void)
{
  SPI_DDR |= (1<<MOSI_PIN) | (1<<CLCK_PIN) | (1<<SS_PIN);  // Set MOSI, CLCK and SlaveSelect as Output
  SPI_DDR &= ~(1<<MISO_PIN);                  // Set MISO as Input
  
  SELECT_DDR |= (1<<SELECT_PIN);    // Set slave select as output
    
  SPCR |= (1<<SPE) | (1<<DORD) | (1<<MSTR) | (1<<CPOL)  | (1<<CPHA) | (1<< SPR1); // SPI enable, LSB first, SPI is Master,  Clock Polarity, Clock Phase, Prescaler 64
  
  SPSR |= (1<<SPIF); // Enable transmission complete interrupt
  
  DESELECT_CONTROLLER
}

uint8_t SpiCommand(const uint8_t *byte_to_send,uint8_t *byte_to_receive)
{
  SPDR = *byte_to_send;
  while (!(SPSR & (1<<SPIF)))
  {
    //wait for transmission complete and do nothing
  }
  *byte_to_receive = SPDR;
  
  return true;
}

In der Main habe ich einfach zwei Arrays angelegt, eins für command und 
eins für die zurückgesendeten Daten.
Zum Senden wird nun Slave-Select auf Low gezogen.
In einer for-Schleife wird dann x-mal der Befehl command aufgerufen und 
die jeweiligen Bytes aus dem Array übertragen.
Und anschließen Slave-Select wieder aug HIGH gezogen.

Vielleicht hat ja jemand ne Idee, ich habe leider jetzt keine Oszi zur 
Hand, müsste mir dann erst nächste Woche mal eins leihen.

[Jim M. (turboj)]

Der Fehler liegt im nicht geposteten Code. So wie Deine Funktionen 
aussehen würde ich den Transmission Coplete Interrupt nicht 
aktivieren.

[Karl H. (kbuchegg)]

Jim M. schrieb:
> Der Fehler liegt im nicht geposteten Code. So wie Deine Funktionen
> aussehen würde ich den Transmission Coplete Interrupt *nicht*
> aktivieren.

typischer Fall, in dem der Kommentar nicht mit dem Code übereinstimmt.
Tip: wenn du Leuten helfen willst, dann IGNORIERE alle Kommentare und 
halte dich nur an den Code. In den meisten Fällen posten hier Anfänger, 
deren Kommentare sowieso nicht hilfreich sind sondern in die Kategorie 
'völlig sinnlos kommentiert' fallen.

Ich stimme allerdings mit dir überein, dass das Problem nicht im 
geposteten Codeteil lauert.
Insbesondere macht mich stutzig, dass das zu sendende Byte per Pointer 
darauf übergeben wird. Es gibt keinen Grund warum das so sein soll, aber 
jede Menge Gründe, warum man das nicht macht.

Genausowenig wie es (momentan noch) einen Grund gibt, warum man das 
erhaltene Byte in einen Speicher schreiben sollte, den man per Pointer 
bekommt. Die Funktion liefert sowieso immer true zurück. D.h. dieser 
Return Wert sagt genau überhaupt nichts aus. Da kann man genausogut auch 
das erhaltene Byte per Returnwert zurück geben.

[Marvin H. (memtest)]

Hallo,

Ja wie ihr richtig bemerkt habt, bin ich noch recht am Anfang der 
Programmierung und daher bereit und offen für konstruktive Kritik, z.B. 
wie könnte ich eurer Meinung nach die Qualität der Kommentare 
verbessern?

Bezüglich der Parameter als Pointer ist es so, dass der Returnwert frei 
bleiben soll für spätere Fehlerausgaben, wenn z.B. die empfangenen Daten 
nicht mit dem Erwartungswert übereinstimmen, ist aber noch nicht 
implementiert, daher return = true;
Deshalb auch die Rückgabe der Daten über "call by reference".
Die Eingabe erfolgt einfach ebenfalls über Pointer da schneller als 
"call by value".
Wäre aber ebenfalls auch hier dankbar für Anregungen, warum man das 
normalerweise anders macht wie du sagtest.

Da offensichtlich keine groben Schnitzer in meinem Code vorliegen, werde 
ich mir die Kommunikation wohl einmal mit einem Oszi ansehen müssen.

Anmerkung:
Wie ihr festgestellt habt bin ich noch am Anfang, aber bereit aus meinen 
Fehlern zu lernen, es ist nicht sehr hilfreich, nur zu sagen:
 - "völlig sinnlos kommentiert"
 - "jede Menge Gründe, warum man das nicht macht"
Es wäre hilfreicher dazu ebenfalls Verbesserungsvorschläge zu liefern, 
so wirkt es lediglich wie "lass den mal machen, der hat sowieso keine 
Ahnung".

Trotzdem vielen Dank für die schnellen Antworten und ich hoffe ihr Helft 
weiterhin ;)

[Stephan (Gast)]

Hi Marvin,
ich versuche mal ein paar Ratschläge zu geben:

SPCR |= ....

SEHR Böse! Du willst hier ein Interface konfigurieren und nicht raten 
müssen was ein paar Zeilen vorher im Code stand.
Die Annahme das die Register auf einen Default stehen ist nicht gut!
Interface immer genau konfigurieren und vorher deaktivieren bitte.

und sieht das nicht etwas besser aus:

#define Spi_enable()              (SPCR |=  (1<<SPE))
#define Spi_disable()             (SPCR &= ~(1<<SPE))

Spi_disable();
SPCR = SPI_LSB_FIRST | SPI_MASTER | SPI_DATA_MODE_3 | SPI_CLKIO_BY_64;
Spi_enable();

Ich kenne deinen AVR nicht, aber bestimmt gibt es so ein Header-File 
auch.
http://www.mikrocontroller.net/attachment/54088/spi_drv.h

weiter:

SPSR |= (1<<SPIF); // Enable transmission complete interrupt

Was willst du damit sagen?
lt. Reg def. ist dieses Register gar nicht beschreibbar.
Das passt hier gar nicht rein.

weiter:

const uint8_t *byte_to_send

Das sind mehr als 1 Byte Daten, die auf dem Stack landen, was kontra 
produktive ist.
hier ist die Übergabe als "call by value" besser geeignet.

Bei dem Return true, würde ich nur bleiben, wenn du deiner 
While-Schleife einen extra Exit gönnst. Sonst ist das sinnlos.

[Stephan (Gast)]

noch was vergessen:

  SELECT_DDR |= (1<<SELECT_PIN);    // Set slave select as output
  DESELECT_CONTROLLER

Diese Sachen gehören nicht in die SPI Init!
Das ist doch bestimmt projektabhängiger Code oder?

Noch ein kleine Hinweis:
Kennst du Doxygen? Nimm es um deinen Code zu dokumentieren.
z.b: Es gibt immer noch genügend Leute die nicht wissen das der SS Pin 
auf Output geschaltet werden muss.
Außerdem sollte der SS-Pin(wenn doch Input) während der Kommunikation 
als Master, nicht auf Low gesetzt werden. (Kollision)

Ich glaub das nennt man Side-Effects(komm gerade nicht auf das Wort), 
das kann zu Fehlern führen. Das sollte als Warnung in die Doku der Init 
Funktion.

Ist das bei den neueren eigentlich immer noch so?

[Marvin H. (memtest)]

Hallo Stephan,

ja du hast recht, das gehört nicht in die init, war hier ebenfalls nur 
zu Testzwecken.

Ja der SS Pin hat laut Dokumentation immernoch die beschriebenen 
Konsequenzen, daher ist er bei mir auch auf Output gesetzt.
Ich kann ihn leider nicht als Slave Select verwenden, da er keine 
Anbindung an einen Pinheader des X-Pro Boards hat (was ich sehr 
merkwürdig finde).
Deshalb habe ich einen extra Slave-Select verwendet.

Doxygen ist mir bekannt und die Dokumentation meiner UART ist auch 
Doxygen konform.
Es ging mir auch eher um die Inhalte meiner Kommentare, die bemängelt 
wurden.

[Stephan (Gast)]

Marvin H. schrieb:
> Ich kann ihn leider nicht als Slave Select verwenden, da er keine
> Anbindung an einen Pinheader des X-Pro Boards hat (was ich sehr
> merkwürdig finde).

Kommt mir irgend wie bekannt vor... (altes STK?)

1.  // Set MOSI, CLCK and SlaveSelect as Output

2.  // Set slave select as output
    
3.  // SPI enable, LSB first, SPI is Master,  Clock Polarity, Clock Phase, Prescaler 64
  
4.  SPSR |= .... // Enable transmission complete interrupt

1. + 2. 2x slave select(SlaveSelect) as Output
Ich weiß und du weißt es auch, das der Code was anderes meint, aber in 2 
Jahren... Es verwirrt.

3. so einen Ratenschwanz an Doku ließt keiner gerne, entweder den Doku 
String neben den Def. (alles untereinander) oder besser gute Def. 
verwenden.
und ob der Ratenschwanz noch mit den Register übereinstimmt weißt du in 
2 Jahren auch nicht mehr.

    
  SPCR = (1<<SPE) |                    // SPI enable
         (1<<DORD) |                   // LSB first
         (1<<MSTR) |                   // SPI is Master,
         (1<<CPOL) | (1<<CPHA) |       // Clock Polarity
         (1<< SPR1);                   // Prescaler 64   (hier fehlt SPR0)

  // BESSER auch ohne Doku
  SPCR = SPI_LSB_FIRST | SPI_MASTER | SPI_DATA_MODE_3 | SPI_CLKIO_BY_64;

4. hier erfährt ein erfahrender Programmierer einen innerlichen 
Aufschrei! (SPSR == STATUS REGISTER!!!)
Sorry, aber diese Zeile lässt vermuten das du nicht weißt was du tust. 
(nicht böse gemeint)

>Doxygen ist mir bekannt und die Dokumentation meiner UART ist auch
>Doxygen konform.
gut

[Marvin H. (memtest)]

Hallo Stephan,

vielen Dank für deine vielen Tipps und konstruktive Kritik,
ich werde das in meinen Code übernehmen und zukünftig auch beherzigen!

Das mit dem SPSR ist echt ein dummer Fehler gewesen (hätte mir 
eigentlich auffallen müssen) saß nur schon einige Stunde davor und war 
dann wohl blind.
Das dass eigentlich SPCR |= (1<<SPIE); sein müsste ist mir eigentlich 
bewusst (Schande über mich).

[Marvin H. (memtest)]

Hallo,
falls es noch jemanden interessieren sollte, ich hab mir mal die 
Übertragung mit nem Oszi angesehen.

Der Ps2 Controller hat kein definiertes Ausgangssignal (pinker Graph), 
weder auf Miso noch auf Acknolege. Der Pegel hängt an den Pins trotz 
Pullup auf ca 1,2V (siehe Scopes).

Werde mir denmnächst mal einen anderen Controller leihen und testen.

gelb = Chip Select
grün = Clock
blau = Mosi
pink = Miso