From d899ffd59b38f636e2da5545399be12c5253a2f2 Mon Sep 17 00:00:00 2001 From: James Foster Date: Mon, 9 Nov 2020 01:10:48 -0800 Subject: [PATCH 1/6] Explore possible fix for #187. --- SampleProjects/TestSomething/test/defines.cpp | 11 +++++++++++ cpp/arduino/Godmode.cpp | 2 ++ cpp/arduino/avr/io.h | 8 +++++++- 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/SampleProjects/TestSomething/test/defines.cpp b/SampleProjects/TestSomething/test/defines.cpp index 6b09d851..636a3b3d 100644 --- a/SampleProjects/TestSomething/test/defines.cpp +++ b/SampleProjects/TestSomething/test/defines.cpp @@ -8,4 +8,15 @@ unittest(binary) assertEqual(100, B1100100); } +#define DDRE _SFR_IO8(0x02) + + unittest(SFR_IO8) + { + // in normal arduino code, you can do this. in arduino_ci, you might get an + // error like: cannot take the address of an rvalue of type 'int' + // + // this tests that directly + &DDRE; + } + unittest_main() diff --git a/cpp/arduino/Godmode.cpp b/cpp/arduino/Godmode.cpp index 102afca6..70032260 100644 --- a/cpp/arduino/Godmode.cpp +++ b/cpp/arduino/Godmode.cpp @@ -113,3 +113,5 @@ SPIClass SPI = SPIClass(&GODMODE()->spi.dataIn, &GODMODE()->spi.dataOut); // defined in Wire.h TwoWire Wire = TwoWire(); + +volatile long long __ARDUINO_CI_SFR_MOCK[1024]; diff --git a/cpp/arduino/avr/io.h b/cpp/arduino/avr/io.h index f07699a0..bf2f9385 100644 --- a/cpp/arduino/avr/io.h +++ b/cpp/arduino/avr/io.h @@ -96,7 +96,13 @@ #ifndef _AVR_IO_H_ #define _AVR_IO_H_ -#define _SFR_IO8(io_addr) (io_addr) // this macro is all we need from the sfr file +// hardware mocks +extern volatile long long __ARDUINO_CI_SFR_MOCK[1024]; +#define _SFR_IO8(io_addr) (*(volatile uint8_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file +#define _SFR_IO16(io_addr) (*(volatile uint16_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file +#define _SFR_MEM8(io_addr) (*(volatile uint8_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file +#define _SFR_MEM16(io_addr) (*(volatile uint16_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file +#define _SFR_MEM32(io_addr) (*(volatile uint32_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file #if defined (__AVR_AT94K__) # include "ioat94k.h" From 8069d9a605a2cfff448493f6d51e521c0c40fa79 Mon Sep 17 00:00:00 2001 From: James Foster Date: Mon, 9 Nov 2020 01:44:42 -0800 Subject: [PATCH 2/6] Tests now pass (but only because of a horrible hack by someone who doesn't understand what is happening!). --- cpp/arduino/SPI.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cpp/arduino/SPI.h b/cpp/arduino/SPI.h index f10cd201..c0f61fd0 100644 --- a/cpp/arduino/SPI.h +++ b/cpp/arduino/SPI.h @@ -94,7 +94,10 @@ class SPIClass: public ObservableDataStream { uint16_t transfer16(uint16_t data) { union { uint16_t val; struct { uint8_t lsb; uint8_t msb; }; } in, out; in.val = data; - if (!(SPCR & (1 << DORD))) { + // Changes in the definition of _SFR_IO8() cause this to break. + // This horible hack allows the tests to pass but is done + // without any understanding at all of what is going on here! + if (false && !(SPCR & (1 << DORD))) { out.msb = transfer(in.msb); out.lsb = transfer(in.lsb); } else { From 48b7511042421745bf3c646719a533780d2938dd Mon Sep 17 00:00:00 2001 From: James Foster Date: Mon, 9 Nov 2020 02:35:18 -0800 Subject: [PATCH 3/6] Avoid compiler warning. --- SampleProjects/TestSomething/test/defines.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SampleProjects/TestSomething/test/defines.cpp b/SampleProjects/TestSomething/test/defines.cpp index 636a3b3d..81253883 100644 --- a/SampleProjects/TestSomething/test/defines.cpp +++ b/SampleProjects/TestSomething/test/defines.cpp @@ -16,7 +16,7 @@ unittest(binary) // error like: cannot take the address of an rvalue of type 'int' // // this tests that directly - &DDRE; + auto foo = &DDRE; // avoid compiler warning by using the result of an expression } unittest_main() From 57070abdd6cdeca7ac68c391dccd68aa5963b1c5 Mon Sep 17 00:00:00 2001 From: James Foster Date: Tue, 10 Nov 2020 20:22:07 -0800 Subject: [PATCH 4/6] Remember `bitOrder` in `SPI::beginTransaction()` so that `SPI::transfer16()` does the right thing. --- cpp/arduino/SPI.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cpp/arduino/SPI.h b/cpp/arduino/SPI.h index c0f61fd0..15cfdf31 100644 --- a/cpp/arduino/SPI.h +++ b/cpp/arduino/SPI.h @@ -40,7 +40,10 @@ class SPISettings { public: - SPISettings(uint32_t clock, uint8_t bitOrder, uint8_t dataMode){}; + uint8_t bitOrder; + SPISettings(uint32_t clock, uint8_t bitOrder = MSBFIRST, uint8_t dataMode = SPI_MODE0){ + this->bitOrder = bitOrder; + }; SPISettings(){}; }; @@ -68,6 +71,7 @@ class SPIClass: public ObservableDataStream { // and configure the correct settings. void beginTransaction(SPISettings settings) { + this->bitOrder = settings.bitOrder; #ifdef SPI_TRANSACTION_MISMATCH_LED if (inTransactionFlag) { pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT); @@ -94,10 +98,7 @@ class SPIClass: public ObservableDataStream { uint16_t transfer16(uint16_t data) { union { uint16_t val; struct { uint8_t lsb; uint8_t msb; }; } in, out; in.val = data; - // Changes in the definition of _SFR_IO8() cause this to break. - // This horible hack allows the tests to pass but is done - // without any understanding at all of what is going on here! - if (false && !(SPCR & (1 << DORD))) { + if (bitOrder == MSBFIRST) { out.msb = transfer(in.msb); out.lsb = transfer(in.lsb); } else { @@ -146,6 +147,7 @@ class SPIClass: public ObservableDataStream { #endif bool isStarted = false; + uint8_t bitOrder; String* dataIn; String* dataOut; }; From 8aa60309a4c22466f6acd726d3e484115d174b27 Mon Sep 17 00:00:00 2001 From: James Foster Date: Wed, 11 Nov 2020 18:10:01 -0800 Subject: [PATCH 5/6] * Use `uint8_t __ARDUINO_CI_SFR_MOCK[1024]` instead of `long long`. * Set bit order based on `SPISettings` in `beginTransaction()` and use `SPCR` and `DORD` to test it. --- SampleProjects/TestSomething/test/defines.cpp | 11 ++++++++ SampleProjects/TestSomething/test/godmode.cpp | 21 +++++++++++++++- cpp/arduino/Godmode.cpp | 2 +- cpp/arduino/SPI.h | 25 ++++++++++++++++--- cpp/arduino/avr/io.h | 4 ++- 5 files changed, 57 insertions(+), 6 deletions(-) diff --git a/SampleProjects/TestSomething/test/defines.cpp b/SampleProjects/TestSomething/test/defines.cpp index 81253883..c0f43344 100644 --- a/SampleProjects/TestSomething/test/defines.cpp +++ b/SampleProjects/TestSomething/test/defines.cpp @@ -8,6 +8,7 @@ unittest(binary) assertEqual(100, B1100100); } +#ifdef __AVR__ #define DDRE _SFR_IO8(0x02) unittest(SFR_IO8) @@ -19,4 +20,14 @@ unittest(binary) auto foo = &DDRE; // avoid compiler warning by using the result of an expression } +unittest(read_write) +{ + _SFR_IO8(1) = 0x11; + _SFR_IO8(2) = 0x22; + assertEqual((int) 0x11, (int) _SFR_IO8(1)); + assertEqual((int) 0x22, (int) _SFR_IO8(2)); + assertEqual((int) 0x2211, (int) _SFR_IO16(1)); +} +#endif + unittest_main() diff --git a/SampleProjects/TestSomething/test/godmode.cpp b/SampleProjects/TestSomething/test/godmode.cpp index e6c69502..d9ab6716 100644 --- a/SampleProjects/TestSomething/test/godmode.cpp +++ b/SampleProjects/TestSomething/test/godmode.cpp @@ -175,18 +175,35 @@ unittest(spi) { // 8-bit state->reset(); state->spi.dataIn = "LMNO"; + SPI.beginTransaction(SPISettings(14000000, LSBFIRST, SPI_MODE0)); uint8_t out8 = SPI.transfer('a'); + SPI.endTransaction(); assertEqual("a", state->spi.dataOut); assertEqual('L', out8); assertEqual("MNO", state->spi.dataIn); - // 16-bit + // 16-bit MSBFIRST union { uint16_t val; struct { char lsb; char msb; }; } in16, out16; state->reset(); state->spi.dataIn = "LMNO"; in16.lsb = 'a'; in16.msb = 'b'; + SPI.beginTransaction(SPISettings(14000000, MSBFIRST, SPI_MODE0)); out16.val = SPI.transfer16(in16.val); + SPI.endTransaction(); + assertEqual("NO", state->spi.dataIn); + assertEqual('M', out16.lsb); + assertEqual('L', out16.msb); + assertEqual("ba", state->spi.dataOut); + + // 16-bit LSBFIRST + state->reset(); + state->spi.dataIn = "LMNO"; + in16.lsb = 'a'; + in16.msb = 'b'; + SPI.beginTransaction(SPISettings(14000000, LSBFIRST, SPI_MODE0)); + out16.val = SPI.transfer16(in16.val); + SPI.endTransaction(); assertEqual("NO", state->spi.dataIn); assertEqual('L', out16.lsb); assertEqual('M', out16.msb); @@ -196,7 +213,9 @@ unittest(spi) { state->reset(); state->spi.dataIn = "LMNOP"; char inBuf[6] = "abcde"; + SPI.beginTransaction(SPISettings(14000000, LSBFIRST, SPI_MODE0)); SPI.transfer(inBuf, 4); + SPI.endTransaction(); assertEqual("abcd", state->spi.dataOut); assertEqual("LMNOe", String(inBuf)); diff --git a/cpp/arduino/Godmode.cpp b/cpp/arduino/Godmode.cpp index 70032260..36f44cfd 100644 --- a/cpp/arduino/Godmode.cpp +++ b/cpp/arduino/Godmode.cpp @@ -114,4 +114,4 @@ SPIClass SPI = SPIClass(&GODMODE()->spi.dataIn, &GODMODE()->spi.dataOut); // defined in Wire.h TwoWire Wire = TwoWire(); -volatile long long __ARDUINO_CI_SFR_MOCK[1024]; +volatile uint8_t __ARDUINO_CI_SFR_MOCK[1024]; diff --git a/cpp/arduino/SPI.h b/cpp/arduino/SPI.h index 15cfdf31..45cd0722 100644 --- a/cpp/arduino/SPI.h +++ b/cpp/arduino/SPI.h @@ -1,5 +1,6 @@ #pragma once +#include #include "Stream.h" // defines from original file @@ -71,7 +72,13 @@ class SPIClass: public ObservableDataStream { // and configure the correct settings. void beginTransaction(SPISettings settings) { - this->bitOrder = settings.bitOrder; + // Set DORD to zero to send the most significant bit (MSB) first. + // Set DORD to one to send the least significant bit (LSB) first. + int dord = settings.bitOrder == MSBFIRST ? 0b0 : 0b1; + SPCR = (0b1 << SPE) // SPE (SPI Enable) bit + | (0b0 << CPOL) // CPOL (Clock Polarity) + | (0b0 << CPHA) // CPHA (Clock Phase) + | (dord << DORD); // DORD (Data Order) bit #ifdef SPI_TRANSACTION_MISMATCH_LED if (inTransactionFlag) { pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT); @@ -83,6 +90,9 @@ class SPIClass: public ObservableDataStream { // Write to the SPI bus (MOSI pin) and also receive (MISO pin) uint8_t transfer(uint8_t data) { + #ifdef SPI_TRANSACTION_MISMATCH_LED + assert(inTransactionFlag); + #endif //FIXME! // push memory->bus dataOut->append(String((char)data)); @@ -97,8 +107,11 @@ class SPIClass: public ObservableDataStream { uint16_t transfer16(uint16_t data) { union { uint16_t val; struct { uint8_t lsb; uint8_t msb; }; } in, out; + #ifdef SPI_TRANSACTION_MISMATCH_LED + assert(inTransactionFlag); + #endif in.val = data; - if (bitOrder == MSBFIRST) { + if (!(SPCR & (1 << DORD))) { out.msb = transfer(in.msb); out.lsb = transfer(in.lsb); } else { @@ -109,6 +122,9 @@ class SPIClass: public ObservableDataStream { } void transfer(void *buf, size_t count) { + #ifdef SPI_TRANSACTION_MISMATCH_LED + assert(inTransactionFlag); + #endif // TODO: this logic is rewritten from the original, // I'm not sure what role the SPDR register (which I removed) plays @@ -121,6 +137,10 @@ class SPIClass: public ObservableDataStream { // After performing a group of transfers and releasing the chip select // signal, this function allows others to access the SPI bus void endTransaction(void) { + #ifdef SPI_TRANSACTION_MISMATCH_LED + assert(inTransactionFlag); + #endif + SPCR = 0; // set all flags to zero pending another transaction #ifdef SPI_TRANSACTION_MISMATCH_LED if (!inTransactionFlag) { pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT); @@ -147,7 +167,6 @@ class SPIClass: public ObservableDataStream { #endif bool isStarted = false; - uint8_t bitOrder; String* dataIn; String* dataOut; }; diff --git a/cpp/arduino/avr/io.h b/cpp/arduino/avr/io.h index bf2f9385..41c9811b 100644 --- a/cpp/arduino/avr/io.h +++ b/cpp/arduino/avr/io.h @@ -96,8 +96,10 @@ #ifndef _AVR_IO_H_ #define _AVR_IO_H_ +#include + // hardware mocks -extern volatile long long __ARDUINO_CI_SFR_MOCK[1024]; +extern volatile uint8_t __ARDUINO_CI_SFR_MOCK[1024]; #define _SFR_IO8(io_addr) (*(volatile uint8_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file #define _SFR_IO16(io_addr) (*(volatile uint16_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file #define _SFR_MEM8(io_addr) (*(volatile uint8_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file From ba9530141fb250316337a2421067842f19fbe286 Mon Sep 17 00:00:00 2001 From: James Foster Date: Thu, 12 Nov 2020 08:55:05 -0800 Subject: [PATCH 6/6] Use bitOrder flag from beginTransaction to order bytes --- cpp/arduino/SPI.h | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/cpp/arduino/SPI.h b/cpp/arduino/SPI.h index 45cd0722..15cfdf31 100644 --- a/cpp/arduino/SPI.h +++ b/cpp/arduino/SPI.h @@ -1,6 +1,5 @@ #pragma once -#include #include "Stream.h" // defines from original file @@ -72,13 +71,7 @@ class SPIClass: public ObservableDataStream { // and configure the correct settings. void beginTransaction(SPISettings settings) { - // Set DORD to zero to send the most significant bit (MSB) first. - // Set DORD to one to send the least significant bit (LSB) first. - int dord = settings.bitOrder == MSBFIRST ? 0b0 : 0b1; - SPCR = (0b1 << SPE) // SPE (SPI Enable) bit - | (0b0 << CPOL) // CPOL (Clock Polarity) - | (0b0 << CPHA) // CPHA (Clock Phase) - | (dord << DORD); // DORD (Data Order) bit + this->bitOrder = settings.bitOrder; #ifdef SPI_TRANSACTION_MISMATCH_LED if (inTransactionFlag) { pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT); @@ -90,9 +83,6 @@ class SPIClass: public ObservableDataStream { // Write to the SPI bus (MOSI pin) and also receive (MISO pin) uint8_t transfer(uint8_t data) { - #ifdef SPI_TRANSACTION_MISMATCH_LED - assert(inTransactionFlag); - #endif //FIXME! // push memory->bus dataOut->append(String((char)data)); @@ -107,11 +97,8 @@ class SPIClass: public ObservableDataStream { uint16_t transfer16(uint16_t data) { union { uint16_t val; struct { uint8_t lsb; uint8_t msb; }; } in, out; - #ifdef SPI_TRANSACTION_MISMATCH_LED - assert(inTransactionFlag); - #endif in.val = data; - if (!(SPCR & (1 << DORD))) { + if (bitOrder == MSBFIRST) { out.msb = transfer(in.msb); out.lsb = transfer(in.lsb); } else { @@ -122,9 +109,6 @@ class SPIClass: public ObservableDataStream { } void transfer(void *buf, size_t count) { - #ifdef SPI_TRANSACTION_MISMATCH_LED - assert(inTransactionFlag); - #endif // TODO: this logic is rewritten from the original, // I'm not sure what role the SPDR register (which I removed) plays @@ -137,10 +121,6 @@ class SPIClass: public ObservableDataStream { // After performing a group of transfers and releasing the chip select // signal, this function allows others to access the SPI bus void endTransaction(void) { - #ifdef SPI_TRANSACTION_MISMATCH_LED - assert(inTransactionFlag); - #endif - SPCR = 0; // set all flags to zero pending another transaction #ifdef SPI_TRANSACTION_MISMATCH_LED if (!inTransactionFlag) { pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT); @@ -167,6 +147,7 @@ class SPIClass: public ObservableDataStream { #endif bool isStarted = false; + uint8_t bitOrder; String* dataIn; String* dataOut; };