From 82df6ea459ea67b0ac92ba3247631824282b1e15 Mon Sep 17 00:00:00 2001
From: Doug Brown <doug@downtowndougbrown.com>
Date: Thu, 26 Nov 2020 19:38:57 -0800
Subject: [PATCH] Fix a few minor issues

I noticed that after I implemented the SPI optimization of cycle
counting instead of polling on SPIF, the first "normal" SPI transaction
I tried would fail. This is because nothing was clearing the SPIF flag
anymore, and the normal SPI driver still looks at it. So it was thinking
that the latest transaction was already completed (it wasn't). Worked
around this by making sure we clear the flag in SPI_Assert. I'm not
concerned about performance impact here because the actual clean SPI
driver is not used in performance-bound situations.

Fixed an issue that identified the wrong pins as shorted to ground in
the electrical test functionality. Whoops!
---
 hal/at90usb646/parallel_bus.c |  2 +-
 hal/at90usb646/spi.c          | 12 +++++++
 tests/simm_electrical_test.c  | 60 ++++++++++++++++-------------------
 3 files changed, 40 insertions(+), 34 deletions(-)

diff --git a/hal/at90usb646/parallel_bus.c b/hal/at90usb646/parallel_bus.c
index 4de533b..6729311 100644
--- a/hal/at90usb646/parallel_bus.c
+++ b/hal/at90usb646/parallel_bus.c
@@ -101,7 +101,7 @@ void ParallelBus_Init(void)
 
 	// Set all data lines to pulled-up inputs
 	ParallelBus_SetDataDir(0);
-	ParallelBus_SetDataPullups(0xFFFFFFFF);
+	ParallelBus_SetDataPullups(0xFFFFFFFFUL);
 	dataIsOutput = false;
 	// Note: During normal operation of read/write cycles, the pullups in the
 	// MCP23S17 will remember they are enabled, so we can do an optimization
diff --git a/hal/at90usb646/spi.c b/hal/at90usb646/spi.c
index 5ce8352..e1c9854 100644
--- a/hal/at90usb646/spi.c
+++ b/hal/at90usb646/spi.c
@@ -157,6 +157,18 @@ void SPI_ReleaseBus(SPIDevice *spi)
 void SPI_Assert(SPIDevice *spi)
 {
 	GPIO_SetOff(spi->csPin);
+
+	// Due to the optimization we do in ParallelBus talking directly to the
+	// SPI hardware without going through this driver, we need to make sure
+	// that the SPIF flag is cleared here. Otherwise we may think we're done
+	// too early, which would cause us to screw up the next SPI transfer.
+	// This happens because the optimized code doesn't look at SPSR, so the
+	// SPIF flag never gets cleared from the previous SPI operation.
+	if (SPSR & (1 << SPIF))
+	{
+		// Reading the data register clears the flag if it's set
+		(void)SPDR;
+	}
 }
 
 /** Deasserts an SPI device's chip select pin
diff --git a/tests/simm_electrical_test.c b/tests/simm_electrical_test.c
index 6909ae9..5aef975 100644
--- a/tests/simm_electrical_test.c
+++ b/tests/simm_electrical_test.c
@@ -93,7 +93,7 @@ int SIMMElectricalTest_Run(void (*errorHandler)(uint8_t, uint8_t))
 	// (We have to ignore them during the second phase of the test)
 	SIMMElectricalTest_ResetGroundShorts();
 
-	// First check for anything shorted to ground. Set all lines as inputs wit
+	// First check for anything shorted to ground. Set all lines as inputs with
 	// a weak pull-up resistor. Then read the values back and check for any
 	// zeros. This would indicate a short to ground.
 	ParallelBus_SetAddressDir(0);
@@ -116,53 +116,47 @@ int SIMMElectricalTest_Run(void (*errorHandler)(uint8_t, uint8_t))
 
 	// Read the address pins back first
 	uint32_t readback = ParallelBus_ReadAddress();
-	if (readback != SIMM_ADDRESS_PINS_MASK)
+	// Check each bit for a LOW which would indicate a short to ground
+	for (i = 0; i <= SIMM_HIGHEST_ADDRESS_LINE; i++)
 	{
-		// Check each bit for a LOW which would indicate a short to ground
-		for (i = 0; i <= SIMM_HIGHEST_ADDRESS_LINE; i++)
+		// Did we find a low bit?
+		if (!(readback & 1))
 		{
-			// Did we find a low bit?
-			if (!(readback & 1))
+			// That means this pin is shorted to ground.
+			// So notify the caller that we have a ground short on this pin
+			if (errorHandler)
 			{
-				// That means this pin is shorted to ground.
-				// So notify the caller that we have a ground short on this pin
-				if (errorHandler)
-				{
-					errorHandler(curPin, GROUND_FAIL_INDEX);
-				}
-
-				// Add it to our internal list of ground shorts also.
-				SIMMElectricalTest_AddGroundShort(curPin);
-
-				// And of course increment the error counter.
-				numErrors++;
+				errorHandler(curPin, GROUND_FAIL_INDEX);
 			}
 
-			// No matter what, though, move on to the next bit and pin.
-			readback >>= 1;
-			curPin++;
+			// Add it to our internal list of ground shorts also.
+			SIMMElectricalTest_AddGroundShort(curPin);
+
+			// And of course increment the error counter.
+			numErrors++;
 		}
+
+		// No matter what, though, move on to the next bit and pin.
+		readback >>= 1;
+		curPin++;
 	}
 
 	// Repeat the exact same process for the data pins
 	readback = ParallelBus_ReadData();
-	if (readback != SIMM_DATA_PINS_MASK)
+	for (i = 0; i <= SIMM_HIGHEST_DATA_LINE; i++)
 	{
-		for (i = 0; i <= SIMM_HIGHEST_DATA_LINE; i++)
+		if (!(readback & 1))
 		{
-			if (!(readback & 1))
+			if (errorHandler)
 			{
-				if (errorHandler)
-				{
-					errorHandler(curPin, GROUND_FAIL_INDEX);
-				}
-				SIMMElectricalTest_AddGroundShort(curPin);
-				numErrors++;
+				errorHandler(curPin, GROUND_FAIL_INDEX);
 			}
-
-			readback >>= 1;
-			curPin++;
+			SIMMElectricalTest_AddGroundShort(curPin);
+			numErrors++;
 		}
+
+		readback >>= 1;
+		curPin++;
 	}
 
 	// Check chip select in the same way...