Fix AVR SPI parameter configuration, remove timeouts due to sync protocol. (#8775)

docs/spi_driver.md

@@ -16,7 +16,7 @@ No special setup is required - just connect the `SS`, `SCK`, `MOSI` and `MISO` p
 You may use more than one slave select pin, not just the `SS` pin. This is useful when you have multiple devices connected and need to communicate with them individually.
 `SPI_SS_PIN` can be passed to `spi_start()` to refer to `SS`.
 
-## ARM Configuration
+## ChibiOS/ARM Configuration
 
 ARM support for this driver is not ready yet. Check back later!
 
@@ -28,7 +28,7 @@ Initialize the SPI driver. This function must be called only once, before any of
 
 ---
 
-### `void spi_start(pin_t slavePin, bool lsbFirst, uint8_t mode, uint8_t divisor)`
+### `bool spi_start(pin_t slavePin, bool lsbFirst, uint8_t mode, uint16_t divisor)`
 
 Start an SPI transaction.
 
@@ -48,12 +48,16 @@ Start an SPI transaction.
    |`2` |Leading edge falling|Sample on leading edge |
    |`3` |Leading edge falling|Sample on trailing edge|
 
- - `uint8_t divisor`  
+ - `uint16_t divisor`  
    The SPI clock divisor, will be rounded up to the nearest power of two. This number can be calculated by dividing the MCU's clock speed by the desired SPI clock speed. For example, an MCU running at 8 MHz wanting to talk to an SPI device at 4 MHz would set the divisor to `2`.
 
+#### Return Value
+
+`false` if the supplied parameters are invalid or the SPI peripheral is already in use, or `true`.
+
 ---
 
-### `spi_status_t spi_write(uint8_t data, uint16_t timeout)`
+### `spi_status_t spi_write(uint8_t data)`
 
 Write a byte to the selected SPI device.
 
@@ -61,8 +65,6 @@ Write a byte to the selected SPI device.
 
  - `uint8_t data`  
    The byte to write.
- - `uint16_t timeout`  
-   The amount of time to wait, in milliseconds, before timing out.
 
 #### Return Value
 
@@ -70,22 +72,17 @@ Write a byte to the selected SPI device.
 
 ---
 
-### `spi_status_t spi_read(uint16_t timeout)`
+### `spi_status_t spi_read(void)`
 
 Read a byte from the selected SPI device.
 
-#### Arguments
-
- - `uint16_t timeout`  
-   The amount of time to wait, in milliseconds, before timing out.
-
 #### Return Value
 
 `SPI_STATUS_TIMEOUT` if the timeout period elapses, or the byte read from the device.
 
 ---
 
-### `spi_status_t spi_transmit(const uint8_t *data, uint16_t length, uint16_t timeout)`
+### `spi_status_t spi_transmit(const uint8_t *data, uint16_t length)`
 
 Send multiple bytes to the selected SPI device.
 
@@ -95,8 +92,6 @@ Send multiple bytes to the selected SPI device.
    A pointer to the data to write from.
  - `uint16_t length`  
    The number of bytes to write. Take care not to overrun the length of `data`.
- - `uint16_t timeout`  
-   The amount of time to wait, in milliseconds, before timing out.
 
 #### Return Value
 
@@ -104,7 +99,7 @@ Send multiple bytes to the selected SPI device.
 
 ---
 
-### `spi_status_t spi_receive(uint8_t *data, uint16_t length, uint16_t timeout)`
+### `spi_status_t spi_receive(uint8_t *data, uint16_t length)`
 
 Receive multiple bytes from the selected SPI device.
 
@@ -114,12 +109,10 @@ Receive multiple bytes from the selected SPI device.
    A pointer to the buffer to read into.
  - `uint16_t length`  
    The number of bytes to read. Take care not to overrun the length of `data`.
- - `uint16_t timeout`  
-   The amount of time to wait, in milliseconds, before timing out.
 
 #### Return Value
 
-`SPI_STATUS_TIMEOUT` if the timeout period elapses, `SPI_STATUS_SUCCESS` on success, or `SPI_STATUS_ERROR` otherwise.
+`SPI_STATUS_TIMEOUT` if the internal transmission timeout period elapses, `SPI_STATUS_SUCCESS` on success, or `SPI_STATUS_ERROR` otherwise.
 
 ---
 

drivers/avr/spi_master.c

@@ -34,6 +34,10 @@
 #    define SPI_MISO_PIN B4
 #endif
 
+#ifndef SPI_TIMEOUT
+#    define SPI_TIMEOUT 100
+#endif
+
 static pin_t   currentSlavePin    = NO_PIN;
 static uint8_t currentSlaveConfig = 0;
 static bool    currentSlave2X     = false;
@@ -47,65 +51,74 @@ void spi_init(void) {
     SPCR = (_BV(SPE) | _BV(MSTR));
 }
 
-void spi_start(pin_t slavePin, bool lsbFirst, uint8_t mode, uint8_t divisor) {
-    if (currentSlavePin == NO_PIN && slavePin != NO_PIN) {
-        if (lsbFirst) {
-            currentSlaveConfig |= _BV(DORD);
-        }
+bool spi_start(pin_t slavePin, bool lsbFirst, uint8_t mode, uint16_t divisor) {
+    if (currentSlavePin != NO_PIN || slavePin == NO_PIN) {
+        return false;
+    }
 
-        switch (mode) {
-            case 1:
-                currentSlaveConfig |= _BV(CPHA);
-                break;
-            case 2:
-                currentSlaveConfig |= _BV(CPOL);
-                break;
-            case 3:
-                currentSlaveConfig |= (_BV(CPOL) | _BV(CPHA));
-                break;
-        }
+    currentSlaveConfig = 0;
 
-        uint8_t roundedDivisor = 1;
-        while (roundedDivisor < divisor) {
-            roundedDivisor <<= 1;
-        }
+    if (lsbFirst) {
+        currentSlaveConfig |= _BV(DORD);
+    }
 
-        switch (roundedDivisor) {
-            case 16:
-                currentSlaveConfig |= _BV(SPR0);
-                break;
-            case 64:
-                currentSlaveConfig |= _BV(SPR1);
-                break;
-            case 128:
-                currentSlaveConfig |= (_BV(SPR1) | _BV(SPR0));
-                break;
-            case 2:
-                currentSlave2X = true;
-                break;
-            case 8:
-                currentSlave2X = true;
-                currentSlaveConfig |= _BV(SPR0);
-                break;
-            case 32:
-                currentSlave2X = true;
-                currentSlaveConfig |= _BV(SPR1);
-                break;
-        }
+    switch (mode) {
+        case 1:
+            currentSlaveConfig |= _BV(CPHA);
+            break;
+        case 2:
+            currentSlaveConfig |= _BV(CPOL);
+            break;
+        case 3:
+            currentSlaveConfig |= (_BV(CPOL) | _BV(CPHA));
+            break;
+    }
 
-        SPSR |= currentSlaveConfig;
-        currentSlavePin = slavePin;
-        setPinOutput(currentSlavePin);
-        writePinLow(currentSlavePin);
+    uint16_t roundedDivisor = 1;
+    while (roundedDivisor < divisor) {
+        roundedDivisor <<= 1;
+    }
+
+    switch (roundedDivisor) {
+        case 16:
+            currentSlaveConfig |= _BV(SPR0);
+            break;
+        case 64:
+            currentSlaveConfig |= _BV(SPR1);
+            break;
+        case 128:
+            currentSlaveConfig |= (_BV(SPR1) | _BV(SPR0));
+            break;
+        case 2:
+            currentSlave2X = true;
+            break;
+        case 8:
+            currentSlave2X = true;
+            currentSlaveConfig |= _BV(SPR0);
+            break;
+        case 32:
+            currentSlave2X = true;
+            currentSlaveConfig |= _BV(SPR1);
+            break;
     }
+
+    SPCR |= currentSlaveConfig;
+    if (currentSlave2X) {
+        SPSR |= _BV(SPI2X);
+    }
+    currentSlavePin = slavePin;
+    setPinOutput(currentSlavePin);
+    writePinLow(currentSlavePin);
+
+    return true;
 }
 
-spi_status_t spi_write(uint8_t data, uint16_t timeout) {
+spi_status_t spi_write(uint8_t data) {
     SPDR = data;
 
     uint16_t timeout_timer = timer_read();
     while (!(SPSR & _BV(SPIF))) {
-        if ((timeout != SPI_TIMEOUT_INFINITE) && ((timer_read() - timeout_timer) >= timeout)) {
+        if ((timer_read() - timeout_timer) >= SPI_TIMEOUT) {
             return SPI_STATUS_TIMEOUT;
         }
     }
@@ -113,12 +126,12 @@ spi_status_t spi_write(uint8_t data, uint16_t timeout) {
     return SPDR;
 }
 
-spi_status_t spi_read(uint16_t timeout) {
+spi_status_t spi_read() {
     SPDR = 0x00;  // Dummy
 
     uint16_t timeout_timer = timer_read();
     while (!(SPSR & _BV(SPIF))) {
-        if ((timeout != SPI_TIMEOUT_INFINITE) && ((timer_read() - timeout_timer) >= timeout)) {
+        if ((timer_read() - timeout_timer) >= SPI_TIMEOUT) {
             return SPI_STATUS_TIMEOUT;
         }
     }
@@ -126,21 +139,21 @@ spi_status_t spi_read(uint16_t timeout) {
     return SPDR;
 }
 
-spi_status_t spi_transmit(const uint8_t *data, uint16_t length, uint16_t timeout) {
+spi_status_t spi_transmit(const uint8_t *data, uint16_t length) {
     spi_status_t status = SPI_STATUS_ERROR;
 
     for (uint16_t i = 0; i < length; i++) {
-        status = spi_write(data[i], timeout);
+        status = spi_write(data[i]);
     }
 
     return status;
 }
 
-spi_status_t spi_receive(uint8_t *data, uint16_t length, uint16_t timeout) {
+spi_status_t spi_receive(uint8_t *data, uint16_t length) {
     spi_status_t status = SPI_STATUS_ERROR;
 
     for (uint16_t i = 0; i < length; i++) {
-        status = spi_read(timeout);
+        status = spi_read();
 
         if (status > 0) {
             data[i] = status;
@@ -155,9 +168,9 @@ void spi_stop(void) {
         setPinOutput(currentSlavePin);
         writePinHigh(currentSlavePin);
         currentSlavePin = NO_PIN;
+        SPSR &= ~(_BV(SPI2X));
         SPCR &= ~(currentSlaveConfig);
         currentSlaveConfig = 0;
-        SPSR               = 0;
         currentSlave2X     = false;
     }
 }

drivers/avr/spi_master.h

@@ -41,15 +41,15 @@ extern "C" {
 #endif
 void spi_init(void);
 
-void spi_start(pin_t slavePin, bool lsbFirst, uint8_t mode, uint8_t divisor);
+bool spi_start(pin_t slavePin, bool lsbFirst, uint8_t mode, uint16_t divisor);
 
-spi_status_t spi_write(uint8_t data, uint16_t timeout);
+spi_status_t spi_write(uint8_t data);
 
-spi_status_t spi_read(uint16_t timeout);
+spi_status_t spi_read(void);
 
-spi_status_t spi_transmit(const uint8_t *data, uint16_t length, uint16_t timeout);
+spi_status_t spi_transmit(const uint8_t *data, uint16_t length);
 
-spi_status_t spi_receive(uint8_t *data, uint16_t length, uint16_t timeout);
+spi_status_t spi_receive(uint8_t *data, uint16_t length);
 
 void spi_stop(void);
 #ifdef __cplusplus

tmk_core/protocol/lufa/adafruit_ble.cpp

@@ -152,7 +152,7 @@ static bool sdep_send_pkt(const struct sdep_msg *msg, uint16_t timeout) {
     bool     ready      = false;
 
     do {
-        ready = spi_write(msg->type, 100) != SdepSlaveNotReady;
+        ready = spi_write(msg->type) != SdepSlaveNotReady;
         if (ready) {
             break;
         }
@@ -165,7 +165,7 @@ static bool sdep_send_pkt(const struct sdep_msg *msg, uint16_t timeout) {
 
     if (ready) {
         // Slave is ready; send the rest of the packet
-        spi_transmit(&msg->cmd_low, sizeof(*msg) - (1 + sizeof(msg->payload)) + msg->len, 100);
+        spi_transmit(&msg->cmd_low, sizeof(*msg) - (1 + sizeof(msg->payload)) + msg->len);
         success = true;
     }
 
@@ -205,7 +205,7 @@ static bool sdep_recv_pkt(struct sdep_msg *msg, uint16_t timeout) {
 
         do {
             // Read the command type, waiting for the data to be ready
-            msg->type = spi_read(100);
+            msg->type = spi_read();
             if (msg->type == SdepSlaveNotReady || msg->type == SdepSlaveOverflow) {
                 // Release it and let it initialize
                 spi_stop();
@@ -215,11 +215,11 @@ static bool sdep_recv_pkt(struct sdep_msg *msg, uint16_t timeout) {
             }
 
             // Read the rest of the header
-            spi_receive(&msg->cmd_low, sizeof(*msg) - (1 + sizeof(msg->payload)), 100);
+            spi_receive(&msg->cmd_low, sizeof(*msg) - (1 + sizeof(msg->payload)));
 
             // and get the payload if there is any
             if (msg->len <= SdepMaxPayload) {
-                spi_receive(msg->payload, msg->len, 100);
+                spi_receive(msg->payload, msg->len);
             }
             success = true;
             break;