espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.76k stars 741 forks source link

Software SPI MISO always MSB? #2278

Closed fanoush closed 1 year ago

fanoush commented 1 year ago

I was trying to use software SPI to read SWD data bits however no matter what mode is set - "msb" or "lsb" it comes back same and wrong for me = msb , this line below looks like result is always shifted in same direction https://github.com/espruino/Espruino/blob/5345b5f71ff4fa2c53ba0a772ea356270b5ca947/src/jsspi.c#L74 Is this a bug or actually a feature of SPI? I already solved the issue by using combined routine that reverses bit order and switches endianness in one go as I need to do it anyway since SPI transfers are byte oriented, however if this is bug it can be fixed anyway.

BTW it would be nice to send/receive arbitrary number of bits with each SPI transfer. OTOH for sending bits there is already the shiftOut , not sure how it came to be but complementary shiftIn would be nice too :-)

gfwilliams commented 1 year ago

Is this a bug or actually a feature of SPI?

It sounds like a bug to me. Probably it's best to fix it in the if (rx) rx[i] = result; line though as I'm pretty sure there's an 8 bit reverse function somewhere.

I think shiftOut started out as a copy of the Arduino one, but yes, a shiftin might be neat (especially if it supported >1 data pin).

fanoush commented 1 year ago

It sounds like a bug to me.

OK, was not sure if master could send LSB and the response could be MSB

As for fix, this feels easier and is similar to MOSI code, ther are two variants - with if or always shifting

diff --git a/src/jsspi.c b/src/jsspi.c
index 5ad0be4fb..a15d139a5 100644
--- a/src/jsspi.c
+++ b/src/jsspi.c
@@ -71,7 +71,7 @@ void jsspiSoftwareFunc(
         if (inf->pinSCK != PIN_UNDEFINED)
           jshPinSetValue(inf->pinSCK, !CPOL );
         if (inf->pinMISO != PIN_UNDEFINED)
-          result = (result<<1) | (jshPinGetValue(inf->pinMISO )?1:0);
+          if (jshPinGetValue(inf->pinMISO )) result |= (1<<bit);
         if (inf->pinSCK != PIN_UNDEFINED)
           jshPinSetValue(inf->pinSCK, CPOL );
       } else { // CPHA=1
@@ -82,7 +82,7 @@ void jsspiSoftwareFunc(
         if (inf->pinSCK != PIN_UNDEFINED)
           jshPinSetValue(inf->pinSCK, CPOL );
         if (inf->pinMISO != PIN_UNDEFINED)
-          result = (result<<1) | (jshPinGetValue(inf->pinMISO )?1:0);
+          result |= (jshPinGetValue(inf->pinMISO )?1:0)<<bit;
       }
     }
     if (rx) rx[i] = result;

I would guess the first variant could be better optimized but the second variant actually produces shorter code

          if (jshPinGetValue(inf->pinMISO )) result |= (1<<bit);
   4c7f6:   f7f9 fdf9   bl  463ec <jshPinGetValue>
   4c7fa:   b110        cbz r0, 4c802 <jsspiSoftwareFunc+0x92>
   4c7fc:   2301        movs    r3, #1
   4c7fe:   40ab        lsls    r3, r5
   4c800:   431f        orrs    r7, r3
        if (inf->pinSCK != PIN_UNDEFINED)

vs

         result |= (jshPinGetValue(inf->pinMISO )?1:0)<<bit;
   4c846:   f7f9 fdd1   bl  463ec <jshPinGetValue>
   4c84a:   40a8        lsls    r0, r5
   4c84c:   4307        orrs    r7, r0

so can I make PR with the second variant result |= (jshPinGetValue(inf->pinMISO )?1:0)<<bit; in both places?

I think shiftOut started out as a copy of the Arduino one

Oh, I see. There is actualy shiftIn too https://www.arduino.cc/reference/en/language/functions/advanced-io/shiftin/

, but yes, a shiftin might be neat (especially if it supported >1 data pin).

Will check shiftOut code if I can easily copy that code and reverse direction (or even pass direction flag into same code)

gfwilliams commented 1 year ago

I guess I might prefer result |= (jshPinGetValue(inf->pinMISO )?1:0)<<bit; but I don't have strong opinions either way :) The PR sounds great.

or even pass direction flag into same code

That would be fantastic if it worked :)