Got rid of a few magic numbers, cleaned up more code

external_mem.h

@@ -10,6 +10,9 @@
 
 #include <stdint.h>
 #include <stdbool.h>
+#include "ports.h"
+
+#define NUM_CHIPS		4
 
 // Holds info about the chip (retrieved with JEDEC standards)
 struct ChipID
@@ -25,14 +28,6 @@ struct ChipID
 #define IC4				(1 << 0)
 #define ALL_CHIPS		(IC1 | IC2 | IC3 | IC4)
 
-// I feel kind of sick making these available to the outside world, but
-// I'm doing it for efficiency.
-// These are the bits on port B corresponding to the control signals.
-// Pass them to ExternalMem_Assert() or ExternalMem_Deassert().
-#define SIMM_WE		(1 << 6)
-#define SIMM_OE		(1 << 5)
-#define SIMM_CS		(1 << 4)
-
 // Initializes the (bit-banged) external memory interface
 void ExternalMem_Init(void);
 
@@ -51,23 +46,11 @@ void ExternalMem_SetDataAsInput(void);
 // Reads back the value the chips are putting onto the data lines
 uint32_t ExternalMem_ReadData(void);
 
-/*// Sets the state of the chip select line
-void ExternalMem_AssertCS(void);
-void ExternalMem_DeassertCS(void);
-
-// Sets the state of the write enable line
-void ExternalMem_AssertWE(void);
-void ExternalMem_DeassertWE(void);
-
-// Sets the state of the output enable line
-void ExternalMem_AssertOE(void);
-void ExternalMem_DeassertOE(void);*/
-
 // This is not the nicest-looking software engineering practice
 // in the world, but it saves needlessly wasted CPU cycles
 // that would be wasted in layers upon layers of abstraction
-#define ExternalMem_Assert(assertMask) PORTB &= ~(assertMask);
+#define ExternalMem_Assert(assertMask) do { Ports_ControlOff(assertMask); } while (0)
-#define ExternalMem_Deassert(assertMask) PORTB |= (assertMask);
+#define ExternalMem_Deassert(assertMask) do { Ports_ControlOn(assertMask); } while (0)
 
 // Reads a set of data from all 4 chips simultaneously
 void ExternalMem_Read(uint32_t startAddress, uint32_t *buf, uint32_t len);

ports.c

@@ -7,12 +7,6 @@
 
 #include "ports.h"
 #include "mcp23s17.h"
-#include <avr/io.h>
-
-// SIMM control signals on port B
-#define SIMM_WE		(1 << 6)
-#define SIMM_OE		(1 << 5)
-#define SIMM_CS		(1 << 4)
 
 // Save some time by not changing the register
 // unless the value has changed [SPI = relatively slow]
@@ -96,42 +90,6 @@ void Ports_DataOut_RMW(uint32_t data, uint32_t modifyMask)
 	PORTF &= ((modifiedDataOff >> 0) & 0xFF);
 }
 
-void Ports_SetCSOut(bool data)
-{
-	if (data)
-	{
-		PORTB |= SIMM_CS;
-	}
-	else
-	{
-		PORTB &= ~SIMM_CS;
-	}
-}
-
-void Ports_SetOEOut(bool data)
-{
-	if (data)
-	{
-		PORTB |= SIMM_OE;
-	}
-	else
-	{
-		PORTB &= ~SIMM_OE;
-	}
-}
-
-void Ports_SetWEOut(bool data)
-{
-	if (data)
-	{
-		PORTB |= SIMM_WE;
-	}
-	else
-	{
-		PORTB &= ~SIMM_WE;
-	}
-}
-
 void Ports_SetAddressDDR(uint32_t ddr)
 {
 	DDRA = (ddr & 0xFF); // A0-A7

ports.h

@@ -10,6 +10,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+#include <avr/io.h>
 
 // Under normal operation, we will only be using the address pins as outputs.
 // The data pins will switch back and forth between all inputs and all outputs.
@@ -19,7 +20,7 @@
 // Ports_SetDataOut(),
 // Ports_SetAddressDDR() [once at the beginning]
 // Ports_SetDataDDR(),
-// and Ports_ReadDataInData
+// and Ports_ReadData
 //
 // The reason I have implemented all this functionality is to give me complete
 // control over all the pins for other use cases, such as a SIMM electrical test.
@@ -27,15 +28,22 @@
 // many shorted output/input scenarios. So even though these functions are overkill,
 // they will be useful for diagnostics.
 
+// I feel kind of sick making these available to the outside world, but
+// I'm doing it for efficiency because they change fairly often.
+// These are the bits on port B corresponding to the control signals.
+#define SIMM_WE		(1 << 6)
+#define SIMM_OE		(1 << 5)
+#define SIMM_CS		(1 << 4)
+
 void Ports_Init(void);
 
 void Ports_SetAddressOut(uint32_t data);
 void Ports_AddressOut_RMW(uint32_t data, uint32_t modifyMask);
 void Ports_SetDataOut(uint32_t data);
 void Ports_DataOut_RMW(uint32_t data, uint32_t modifyMask);
-void Ports_SetCSOut(bool data);
+#define Ports_SetCSOut(data) do { if (data) Ports_ControlOn(SIMM_CS); else Ports_ControlOff(SIMM_CS); } while (0)
-void Ports_SetOEOut(bool data);
+#define Ports_SetOEOut(data) do { if (data) Ports_ControlOn(SIMM_OE); else Ports_ControlOff(SIMM_OE); } while (0)
-void Ports_SetWEOut(bool data);
+#define Ports_SetWEOut(data) do { if (data) Ports_ControlOn(SIMM_WE); else Ports_ControlOff(SIMM_WE); } while (0)
 
 void Ports_SetAddressDDR(uint32_t ddr);
 void Ports_AddressDDR_RMW(uint32_t ddr, uint32_t modifyMask);
@@ -57,4 +65,9 @@ bool Ports_ReadCS(void);
 bool Ports_ReadOE(void);
 bool Ports_ReadWE(void);
 
+// These two functions are for turning control signals on and off.
+// Pass it a mask of SIMM_WE, SIMM_OE, and SIMM_CS.
+#define Ports_ControlOn(mask)  do { PORTB |= (mask);  } while (0)
+#define Ports_ControlOff(mask) do { PORTB &= ~(mask); } while (0)
+
 #endif /* PORTS_H_ */

usb_serial/usb_serial.c

@@ -113,11 +113,11 @@ void USBSerial_HandleWaitingForCommandByte(uint8_t byte)
 	// Asked to identify the chips in the SIMM. Identify them and send reply.
 	case IdentifyChips:
 	{
-		struct ChipID chips[4];
+		struct ChipID chips[NUM_CHIPS];
 		SendByte(CommandReplyOK);
 		ExternalMem_IdentifyChips(chips);
 		int x;
-		for (x = 0; x < 4; x++)
+		for (x = 0; x < NUM_CHIPS; x++)
 		{
 			SendByte(chips[x].manufacturerID);
 			SendByte(chips[x].deviceID);