Fix issue introduced by correction to #591

Author: SpenceKonde

megaavr/libraries/SPI/README.md

@@ -30,6 +30,17 @@ Notes:
 
 When `SPI.swap()` or `SPI.pins()` is called, assuming it was called with a valid option, the pin mapping requested is saved. When `SPI.begin()` is called, this stored value will specify changes to `PORTMUX.SPIROUTEA` and `PORTx` registers. Thus, if a non-default pin mapping is required, you must set it before before calling `SPI.begin()`. To change the pin mapping after `SPI.begin()` you must turn off SPI with`SPI.end()`, call `SPI.swap()` or `SPI.pins()` and then `SPI.begin()` again.
 
+
+## Note on standard SPI.setClockDivider() method
+As always, constants of the form `SPI_CLOCK_DIVn` are provided. These are what should be passed to setClockDivider().
+
+In the event that an invalid value is passed, if the value is known at compile time, an error will be produced telling you that you're passing an invalid value. However, this only catches the most common, obvious cases. Since there are no runtime exceptions, invalid values that are not known at compile time will get through. When an invalid value is passed that can't be picked out at compile time, we are left with a connundrum:
+
+What should we set the clock divider to? We can't just have the function return and do nothing, because then it will appear that the problem is with the library. My first thought was to just have it set to the minimum speed, which will likely function, if possibly with poor performance, but I realized the SPI peripheral's odd design gives us a way to signalize an error condition here, because there are only 7 supported speeds, but the 2+1 bits are structured as a single-bit field that doubles the clock speed, ahd a 2-bit field that selects thee base between /4, /16, /64, and /128. Hence, /128 with CLK2x and /64 without both generate the same clock frequency.
+
+`SPI_CLOCK_DIV2`, `SPI_CLOCK_DIV4`, `SPI_CLOCK_DIV8`, `SPI_CLOCK_DIV16`, `SPI_CLOCK_DIV32`, `SPI_CLOCK_DIV64`, `SPI_CLOCK_DIV128`
+
+
 ## Two SPI ports
 The AVR DA/DB-series parts have two hardware SPI ports. On parts with more pins, they can be pin-swapped to different sets of pins (up to three sets of pins per SPI peripheral). The AVR DD-series has only a single SPI port - but it has a far more pin options than the DA/DB-series parts do. Originally, it was expected that two libraries could be created like is done for the few classic AVRs with multiple SPI ports (eg, ATmega328PB) and the many 32-bit architectures with multiple SPI ports; however, it was discovered in 1.2.0 (which attempted to implement this) that the existing libraries with which we desire compatibility (an SPI library that you need to modify everything you use with it is hardly satisfactory) were more challenging to work with than expected. In order to work with existing libraries, we need only guarantee that our instances of SPI_class have names matching the convention; that sounds like a low bar - and indeed, it is: the only way it could be a problem is if one of those key names happened to already be used for something, and not just any something, but something which had a greater authority to be naming things than anything the core or core libraries did.
 
@@ -50,7 +61,7 @@ As of 1.3.0, the version of SPI.h included with DxCore allows all SPI0 and SPI1
 Unless the old attachInterrupt implementation is selected, `SPI.usingInterrupt(number)` will cause it to globally disable interrupts while in a transaction. This is not ideal, however *you should not have interrupts performing SPI transactions, period* - SPI transactions are not fast; interrupts should be fast. This is here for compatibility ONLY. The standard implementation of this does not map cleanly onto the new attachInterrupt(). Furthermore, the standard implementation has several logic gaps and race conditions which, together, could still result in the same sort of behavior it was aimed at preventing.
 Similarly, `SPI.notUsingInterrupt(number)` will set it back to the normal mode. This is also not ideal, but we needed a timely fix and as I said, this functionality only, as supplied, approximated correct behavior. I'll come back to this at some point, but it is no longer an urgent - not least because you should never use these functions, because you should not be using SPI from interrupts! The one exception to this is the case where the number passed is NOT_AN_INTERRUPT (-1 or 255), in which case we match the old behavior and simply return.
 
-## SPI.attachInterrupt and SPI.detachInterrupt
+## SPI.attachInterrupt() and SPI.detachInterrupt
 These methods, as far as I can tell were never supported for the official or third party AVR boards, only third party extensions where some other API was used to support SPI slave mode functionality. They were marked as something that should never be called, and there was no sign of code that made use of them within the core. In fact, there was no sign of use of them on the wider internet - except for slave mode extensions for different architectures; hence, I feel safe ditching these.
 
 ## SS (Slave Select) pin

megaavr/libraries/SPI/src/SPI.cpp

@@ -40,6 +40,8 @@
   #define SPI_MODULE SPI0
 #endif
 
+
+
 const SPISettings DEFAULT_SPI_SETTINGS = SPISettings();
 
 SPIClass::SPIClass() {
@@ -588,12 +590,14 @@ void SPIClass::setDataMode(uint8_t mode) {
 
 
 void SPIClass::setClockDivider(uint8_t div) {
-  mode_gm = ~(SPI_MODE_gm | SPI_CLK2X_bm);
-  if (div & mode_gm)
-  if (__builtin_constant_p(div))
-  uint8_t tem = SPI_MODULE.CTRLA;
-  tem &= ~SPI_PRESC_gm;
-  tem |=  div;
+  _check_valid_spi(div); // error if the user has passed something that's not a valid constant to it.
+  uint8_t tem = ~(SPI_PRESC_gm | SPI_CLK2X_bm);
+  if (div & tem) {  //it has bits set not in the bitfield - this can only be true if div is determined at runtime *and* invalid.
+    div = SPI_CLOCK_ERROR; //since we can't fulfill their request, set the divisor to an error
+  }
+  tem &= SPI_MODULE.CTRLA; // now we have the current value of the register with the bits masked off, reusing the temp variable as a mask.
+  tem |= div;
+  SPI_MODULE.CTRLA = div;
 }
 
 uint8_t SPIClass::transfer(uint8_t data) {
@@ -643,3 +647,6 @@ void SPIClass::transfer(void *buf, size_t count) {
 #if SPI_INTERFACES_COUNT > 0
   SPIClass SPI;
 #endif
+
+
+

megaavr/libraries/SPI/src/SPI.h

@@ -22,6 +22,31 @@
 #define _SPI_H_INCLUDED
 
 #include <Arduino.h>
+#ifndef SPI_CLOCK_DIV2
+  #define SPI_CLOCK_DIV2      (SPI_PRESC_DIV4_gc     | SPI_CLK2X_bm  )
+#endif
+#ifndef SPI_CLOCK_DIV4
+  #define SPI_CLOCK_DIV4      (SPI_PRESC_DIV4_gc                     )
+#endif
+#ifndef SPI_CLOCK_DIV8
+  #define SPI_CLOCK_DIV8      (SPI_PRESC_DIV16_gc    | SPI_CLK2X_bm  )
+#endif
+#ifndef SPI_CLOCK_DIV16
+  #define SPI_CLOCK_DIV16     (SPI_PRESC_DIV16_gc                    )
+#endif
+#ifndef SPI_CLOCK_DIV32
+  #define SPI_CLOCK_DIV32     (SPI_PRESC_DIV64_gc    | SPI_CLK2X_bm  )
+#endif
+#ifndef SPI_CLOCK_DIV64
+  #define SPI_CLOCK_DIV64     (SPI_PRESC_DIV64_gc                    )
+#endif
+#ifndef SPI_CLOCK_ERROR
+  #define SPI_CLOCK_ERROR     (SPI_PRESC_DIV128_gc   | SPI_CLK2X_bm  )  // There are two ways to get DIV64 prescaling on the SPI clock. The /128 x 2 route, which is functionally identical to /64. This is what the clock gets set to if you pass garbage to setClockDivider()
+
+#endif
+#ifndef SPI_CLOCK_DIV128
+  #define SPI_CLOCK_DIV128    (SPI_PRESC_DIV128_gc                   )
+#endif
 
 #if defined(SPI_MUX)
   #define SPI0_SWAP_DEFAULT  0x00
@@ -177,7 +202,14 @@
 
 
 //#define EXTERNAL_NUM_INTERRUPTS   NUM_TOTAL_PINS
-
+inline __attribute__((always_inline)) void _check_valid_spi(uint8_t div) {
+    if (__builtin_constant_p(div)) {
+      if (!(SPI_CLOCK_DIV2 == div || SPI_CLOCK_DIV4 == div || SPI_CLOCK_DIV8 == div || SPI_CLOCK_DIV16 == div || SPI_CLOCK_DIV32 == div || SPI_CLOCK_DIV64 == div || SPI_CLOCK_DIV128 == div)) {
+        badArg("setClockDivider called with argument that is not an SPI_CLOCK_DIVn constant");
+        // don't let them pass garbage to it
+      }
+    }
+  }
 
 class SPISettings {
   public:
@@ -346,26 +378,5 @@ class SPIClass {
   extern SPIClass SPI;
 #endif
 
-#ifndef SPI_CLOCK_DIV2
-  #define SPI_CLOCK_DIV2      (SPI_PRESC_DIV4_gc     | SPI_CLK2X_bm  )
-#endif
-#ifndef SPI_CLOCK_DIV4
-  #define SPI_CLOCK_DIV4      (SPI_PRESC_DIV4_gc                     )
-#endif
-#ifndef SPI_CLOCK_DIV8
-  #define SPI_CLOCK_DIV8      (SPI_PRESC_DIV16_gc    | SPI_CLK2X_bm  )
-#endif
-#ifndef SPI_CLOCK_DIV16
-  #define SPI_CLOCK_DIV16     (SPI_PRESC_DIV16_gc                    )
-#endif
-#ifndef SPI_CLOCK_DIV32
-  #define SPI_CLOCK_DIV32     (SPI_PRESC_DIV64_gc    | SPI_CLK2X_bm  )
-#endif
-#ifndef SPI_CLOCK_DIV64
-  #define SPI_CLOCK_DIV64     (SPI_PRESC_DIV64_gc                    )
-#endif
-#ifndef SPI_CLOCK_DIV128
-  #define SPI_CLOCK_DIV128    (SPI_PRESC_DIV128_gc                   )
 #endif
 
-#endif