First Attempt at Bitbang SPI, would like feedback

Go To Last Post
41 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

So ill preface this by saying this is most likely wrong, I don't have access to a Logic Analyzer or Oscilloscope (although an LA is on my "Buy next" list). This is in NO way nowhere close to being a "full bitbang library" nor do I want it to be. This is just a super duper simple example for my learning experience to understand timing diagrams (my weak point). I want to understand how SPI works because even though I feel like I get the timing diagram.....im sure i've probably done something wrong. Im still a beginner so this is probably going to be rough looking code. This also is of course only meant to cover Mode 0 (just as a starting point).

 

This is also just the "master code" currently.

 

Code Below:

 

/*
 * 328p_SPI_BitBang.c
 *
 * Created: 11/29/2018 10:59:43 PM
 * This is only for Mode 0 of SPI and is not meant to be anything but a basic test
 */ 

#include <avr/io.h>
#include <avr/delay.h>

/* Macro's */
#define _BV(bit)    (1 << (bit)) 

/* SPI Port Defines */
#define SCLK		PINB4
#define MOSI		PINB3
#define MISO		PINB2
#define SS			PINB1

/* SPI DDR/PINS */
#define SPI_DDR		DDRB
#define SPI_PORT	PORTB
#define SPI_PIN		PINB


#define F_CPU 1000000UL

void enable_SPI(void) {
	SPI_DDR |= 0x1A;			/* Set SS/MOSI/SCLK to Output, MISO to Input */
}


void ss_low(void) {
	SPI_PORT &= ~(1 << SS);
}

void ss_high(void) {
	SPI_PORT |= (1 << SS);
}


void sclk_enable(void) {
	SPI_PORT |= (1 << SCLK);
}

void sclk_disable(void) {
	SPI_PORT &= ~(1 << SCLK);
}

void mosi_high(void) {
	SPI_PORT |= (1 << MOSI);
}

void mosi_low(void) {
	SPI_PORT &= ~(1 << MOSI);
}

unsigned char transmit_byte(unsigned char data_byte) {
	
	int i;
	char ret_data;
	ss_low();			/* Set SS Low */
	_delay_ms(100);
	
	/* SCLK Currently holding low, MOSI is low, SS is low */
	for(i = 0; i < 8; i++) {
		sclk_disable(); //Set Low
		if(data_byte & 0x80) {
			mosi_high();
		}
		else {
			mosi_low();
		}
		/* MOSI Value is latched in */
		_delay_ms(10); // Probably unnecessary
		sclk_enable(); //Send High
		_delay_ms(10); // Also maybe unnecessary? Im horrible on timing
		data_byte >>= 1; //I feel like this is wrong? I see people do <<= instead but....don't we want to shift out LSB first each time, and MOSI receives MSB first?
		if(PINB & MISO) {
			_BV(8-i-1); //I *think* this is right? we receive MSB first correct? so it should be 7,6,5,4,3,2,1,0?
		}
	}
	ss_high(); // Pull Slave-Select back up
	sclk_disable(); // Set Clock back to idle (0 in our case for mode 0)
	return ret_data;
	
}

int main(void)
{
    enable_SPI();		/* Set up correct Data Direction Ports */
    while (1) 
    {
		transmit_byte('A'); //Can this convert?
    }
}

 

Obviously this is just sending a basic byte. Is this ANYWHERE close to being on the right track? I've been looking at timing diagrams and it seems like the data is "there" (I don't know the proper term for it) but maybe "readied?" before the clock signal goes high. It's then sampled and then the clock signal goes low.

 

I think my biggest confusion is how we control when sampling on the trailing edge vs falling/etc... occurs via Software. I mean I understand what this means on a timing diagram but im not really sure how to control that  on the software side (if that makes sense). To me I think (When sending a bit of data, it'll go like this using GPIO pins).

 

Lets pretend were sending a HIGH logic level (1)

  1. MOSI is set to 1 on the GPIO pin of choice
  2. SCLK goes high
  3. Slave see's SCLK high so it says "We got data!"
  4. Slave see's a high logic level on MOSI line
  5. Slave stores that or does w/e with it
  6. Slave sends back a 1 or 0 on it's MISO line to Master
  7. Master see's this on MISO line, does whatever it needs to with it.
  8. SCLK goes low
  9. Repeat

 

So to me, this seems like how this would go with the above. I guess since we are sort of "imitating the clock" with a GPIO pin it feels odd to wrap my head around........but I don't see where we can control when MISO/Slave samples the data, I mean it is going to be before the clock GPIO pin goes low...but I suppose it's not really on the trailing edge is it? (Maybe im over-complicating this)

 

Anyways, thanks for anyone that takes a look and points out what doesn't make any sense (Cause im sure I missed something)

 

Thanks!

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Both MOSI and MISO are the same bit order.

So if you want MSB first then:

data_byte <<= 1; 

 

Why sclk_enable and sclk_disable? Surely inkeeping with the rest of the code it should be sclk_high and sclk_low?

 

what's this supposed to do?

if(PINB & MISO) {
			_BV(8-i-1); //I *think* this is right? we receive MSB first correct? so it should be 7,6,5,4,3,2,1,0?
		}

surely you want something like:

 

if(PINB & MISO) {
			ret_data |= 1;
		}

and don't forget to shift it each time through the loop:

	ret_data <<=1;
	

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

In a real SPI master SS is not controlled by the SPI hardware but by the user software. Therefore I think you should move any manipulation of it out of your transmit_byte() routine into main() as that is where it would be using a real SPI peripheral.

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Take a look at the Atmel App Note AVR320 Software SPI Master:  http://ww1.microchip.com/downloa...

 

Jim

 

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274
get $5 free gold/silver https://www.onegold.com/join/713...

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:

Both MOSI and MISO are the same bit order.

So if you want MSB first then:

data_byte <<= 1; 

 

Why sclk_enable and sclk_disable? Surely inkeeping with the rest of the code it should be sclk_high and sclk_low?

 

what's this supposed to do?

if(PINB & MISO) {
			_BV(8-i-1); //I *think* this is right? we receive MSB first correct? so it should be 7,6,5,4,3,2,1,0?
		}

surely you want something like:

 

if(PINB & MISO) {
			ret_data |= 1;
		}

and don't forget to shift it each time through the loop:

	ret_data <<=1;
	

 

 

Good points on all counts. Am I at least somewhat close? I realize on the _BV part that was dumb because I was basically just making a 11111111 value (I blame it being late at night! ha). Isn't doing the alternative way you described just left shifting in a 1 each cycle though (Unless we receive a 0) but if it was a 0 then we would left shift a 1 next clock cycle. Maybe im looking at it wrong. But to me:

 

ret_data <<=1;

Is just going to left shift a 1 every cycle. (Unless that was meant to be included in the "if" statement). If that was the case then how are we handling when it should be a 0 in that position? (Im sure im missing something here)

 

I guess the SPI example I looked at was wrong, because it looked like it was doing LSB first. I looked here:

http://maxembedded.com/2013/11/s... (It shows an example in the 2nd image of shifting out the LSB first). But then again I had seen examples that did MSB first so I was confused.

 

 

Also Brian, I did not know that about SS but it makes sense, I will move that to main. 

 

Also Kartman im a little unsure what you meant by "Both MOSI and MISO are the same bit order.", im assuming you mean if you transfer MSB first to the slave, then slave should give back MSB first?

Last Edited: Fri. Nov 30, 2018 - 02:28 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Always helps to draw this kind of thing out on paper.

 

now as it happens one of the interview questions I set potential employees happens to include something about synchronous serial routines so here's something I prepared earlier:

        +---+   +---+   +---+   +---+   +---+   +---+   +---+   +---+
        |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |
OUT0 ---+   +---+   +---+   +---+   +---+   +---+   +---+   +---+   +--------
aka CLOCK

         +---------------+       +-------+               +-------+
         |               |       |       |               |       |
OUT1 ----+               +-------+       +---------------+       +-----------
aka DATA
            1       1       0       1       0       0       1        0

0b11010011 = 0xD2 = QED :-)

Either with a pencil and paper or doing it like this in a text editor I think it helps to sketch out what:

	for(i = 0; i < 8; i++) {
		sclk_disable(); //Set Low
		if(data_byte & 0x80) {
			mosi_high();
		}
		else {
			mosi_low();
		}
		/* MOSI Value is latched in */
		_delay_ms(10); // Probably unnecessary
		sclk_enable(); //Send High
		_delay_ms(10); // Also maybe unnecessary? Im horrible on timing
		data_byte >>= 1; //I feel like this is wrong? I see people do <<= instead but....don't we want to shift out LSB first each time, and MOSI receives MSB first?
		if(PINB & MISO) {
			_BV(8-i-1); //I *think* this is right? we receive MSB first correct? so it should be 7,6,5,4,3,2,1,0?
		}
	}

will do and it will quickly become evident if your "probably/maybe unnecessary" lines are required or not.

 

(I'd say some kind of "signal settling delay" is required, I'm not sure it needs to be as much as 10ms for each clock period though ;-)

 

Drawing such diagrams also helps to understand clock phase and polarity too - that is when is the data signal valid in relation to the clock pulse - is it on a rising edge or a falling edge?

Last Edited: Fri. Nov 30, 2018 - 02:47 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ret_data<<=1; shifts the bits 1 place, with a ‘0’ bit introduced.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

Always helps to draw this kind of thing out on paper.

 

now as it happens one of the interview questions I set potential employees happens to include something about synchronous serial routines so here's something I prepared earlier:

        +---+   +---+   +---+   +---+   +---+   +---+   +---+   +---+
        |   |   |   |   |   |   |   |   |   |   |   |   |   |   |   |
OUT0 ---+   +---+   +---+   +---+   +---+   +---+   +---+   +---+   +--------
aka CLOCK

         +---------------+       +-------+               +-------+
         |               |       |       |               |       |
OUT1 ----+               +-------+       +---------------+       +-----------
aka DATA
            1       1       0       1       0       0       1        0

0b11010011 = 0xD2 = QED :-)

Either with a pencil and paper or doing it like this in a text editor I think it helps to sketch out what:

	for(i = 0; i < 8; i++) {
		sclk_disable(); //Set Low
		if(data_byte & 0x80) {
			mosi_high();
		}
		else {
			mosi_low();
		}
		/* MOSI Value is latched in */
		_delay_ms(10); // Probably unnecessary
		sclk_enable(); //Send High
		_delay_ms(10); // Also maybe unnecessary? Im horrible on timing
		data_byte >>= 1; //I feel like this is wrong? I see people do <<= instead but....don't we want to shift out LSB first each time, and MOSI receives MSB first?
		if(PINB & MISO) {
			_BV(8-i-1); //I *think* this is right? we receive MSB first correct? so it should be 7,6,5,4,3,2,1,0?
		}
	}

will do and it will quickly become evident if your "probably/maybe unnecessary" lines are required or not.

 

(I'd say some kind of "signal settling delay" is required, I'm not sure it needs to be as much as 10ms for each clock period though ;-)

 

Drawing such diagrams also helps to understand clock phase and polarity too - that is when is the data signal valid in relation to the clock pulse - is it on a rising edge or a falling edge?

 

Im curious, what exactly is the interview question?

But from what im gathering I probably don't need a delay before or after. When I think about it....it doesn't make any sense because the MOSI will just be sitting there, and the delay after enabling clock doesn't really make any sense either. I guess it doesn't exactly "hurt"? but doesn't really make any sense.

sclk_enable();

I maybe need a delay on the initial transmit_byte before the for loop...but I guess it makes no sense to delay after the clock goes up or down since MOSI will already be "latched in".

 

I think this goes back to me struggling with how code converts into timing, since I see the code actions happening but don't know how they would actually look on a timing diagram (Like I suppose each instruction would be a clock cycle?)

 

ret_data<<=1; shifts the bits 1 place, with a ‘0’ bit introduced.

 

This made sense once I wrote this out. Because if MISO line is reading NOT a 1 we are just shifting everything over 1 (Which will introduce a 0).

 

This is helpful info. Was I at least somewhat on the right track on my initial code? It seems like I wasn't at least WAYYY too far off. Also no clue why the link I used had LSB first....is it ALWAYS MSB first in EVERY instance of SPI?

 

A Secondary question: How exactly "in code" do we control when/where things are being sampled (rising/falling edge). I understand what it means on a timing diagram but I really don't understand how we can control that if it's just GPIO pins being high or low (Since we are imitating a clock basically through GPIO, but it's not really a "clock"). Or can you just not really control it to a finite degree? (This might be a stupid question, apologies if so)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Setup a timer to set a flag (overflow or compare match) and wait in a while loop for it to set, clear the flag, do what is needed, go back and wait for the next flag.

Or set it up to interrupt and do other things while waiting for the next interrupt.

 

Jim

 

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274
get $5 free gold/silver https://www.onegold.com/join/713...

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You’re controlling the clock and you control when you sample. If you’ve set the clock high and it was previously low, that’s a rising edge. With a ‘real’ flip flop, things like setup and hold times are in the order of nanoseconds whereas in your code it is much longer- multiples of the cpu clock freq. so, if you want to read MISO, read the miso port bit, then change the clock bit.

Even though you consider that you are imitating a clock, the slave sees it as a clock and it is your synchronous reference so it is an actual clock for all intents and purposes.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Isn't this all that SPI mode 0 is?

 

 

uint8_t  mySPIoutByte = 0x55,  mySPIinByte = 0;

 

SSpinDOWN();

for (int8_t mask=7; mask < -1; mask -=1) {  // MostSigBit sent first with SPI

     if (mySPIoutByte >> mask) MOSIhi();

     else MOSIlow();

     clkUP();

     clkDOWN();

     if (MISOpin) mySPIinByte |= (1<<mask);

}

SSpinUP();

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ki0bk wrote:

Setup a timer to set a flag (overflow or compare match) and wait in a while loop for it to set, clear the flag, do what is needed, go back and wait for the next flag.

Or set it up to interrupt and do other things while waiting for the next interrupt.

 

Jim

 

Sorry, im confused what you are referring to? (or in reference to what?)

 

Also Kartman yeah that makes sense, I guess im thinking "in hardware" terms but this isn't hardware so I just need to look past that.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've made the changes in my code with everyones suggestions. Without an osciolloscope/logic analyzer is there a good simple way to check if it's "working".

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hook up a spi slave device and see if it works. I recommend getting a logic analyser - you can get cheapies for around $10usd. Cheaper than a McMeal and much more useful.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You could put LEDs on all the SPI lines and slow the clocking down so much that you can see the LEDs change as things happen, or add a push-button to single step through each part of the process!

 

But seriously, get a Logic Analyser.

 

You could, as Kartman says, just hook up an SPI slave device but what do you do when it doesn't work.

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You can wire the software SPI to the hardware SPI and see if the data checks out, no need to use external chips.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

El Tangas wrote:

You can wire the software SPI to the hardware SPI and see if the data checks out, no need to use external chips.

Hmmm I didn't even think of that. I might try the button idea since that would let me step through it at least.

 

Maybe hook an LED up to the CLK line (to see everytime it's working) and an LED up to the MOSI line (to confirm the 0 and 1's).

 

I figured spi-slave devices would be more timing specific wouldn't they? Which the hardware usually handles?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

BTW does this actually work:

 

transmit_byte('A');

As in will it convert it to binary when Im doing the bit shifting/etc?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes, this is interpreted as a byte with the ASCII value of the A character.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just wanted to thank everyone, I got it running with some LED's and was able to check my ASCII characters against the binary values for them and confirm. I used 2 LED's, one for the clock and one for the binary 1/0 representation of the character. It wasn't working at first until a put a 1 second delay (probably didn't need that much) after the "button" check to control my clock.

 

Code Below:

 

/*
 * 328p_SPI_BitBang.c
 *
 * Created: 11/29/2018 10:59:43 PM
 * This is only for Mode 0 of SPI and is not meant to be anything but a basic test
 */ 

#include <avr/io.h>
#include <avr/delay.h>

/* Macro's */
#define _BV(bit)    (1 << (bit)) 

/* SPI Port Defines */
#define SCLK		PINC5
#define MOSI		PINC4
#define MISO		PINC3
#define SS			PINC2

/* SPI DDR/PINS */
#define SPI_DDR		DDRC
#define SPI_PORT	PORTC
#define SPI_PIN		PINC

/* Button DDR/PORT/PIN */
#define BTN_DDR		DDRB
#define BTN_PORT	PORTB
#define BTN_PIN		PINB
#define BUTTON		PINB1

#define F_CPU 1000000UL

void enable_SPI(void) {
	SPI_DDR = 0b00110100;			/* Set SS/MOSI/SCLK to Output, MISO to Input */
}

void ss_low(void) {
	SPI_PORT &= ~(1 << SS);
}

void ss_high(void) {
	SPI_PORT |= (1 << SS);
}

void sclk_high(void) {
	SPI_PORT |= (1 << SCLK);
}

void sclk_low(void) {
	SPI_PORT &= ~(1 << SCLK);
}

void mosi_high(void) {
	SPI_PORT |= (1 << MOSI);
}

void mosi_low(void) {
	SPI_PORT &= ~(1 << MOSI);
}

unsigned char transmit_byte(unsigned char data_byte) {

	int i;
	char ret_data;

	_delay_ms(100);

	/* SCLK Currently holding low, MOSI is low, SS is low */
	for(i = 0; i < 8; i++) {
		loop_until_bit_is_clear(BTN_PIN,BUTTON);
		_delay_ms(1000);
		sclk_low(); //Set Low
		if(data_byte & 0x80) {
			mosi_high();
		}
		else {
			mosi_low();
		}
		/* MOSI Value is latched in */
		sclk_high(); //Send High
		data_byte <<= 1; //MSB First
		if(PINB & MISO) {
			ret_data |= 1;
		}
		ret_data <<=1; //Shift Left
	}
	ss_high(); // Pull Slave-Select back up
	sclk_low(); // Set Clock back to idle (0 in our case for mode 0)
	return ret_data;

}

int main(void)
{
	/* Initialize the data direction register for Port B */
	BTN_DDR = 0b00000000;				/* Setting Pin "1" to Input	*/
	BTN_PORT = 0b00000010;				/*	PUR on PB1	*/
	/*													*/

	ss_low();							/* Set SS Low */
    enable_SPI();						/* Set up correct Data Direction Ports */
    while (1)
    {
		transmit_byte('M');
    }
}

 

Hopefully this thread becomes useful for someone, it's obviously a SUPER basic implementation but I feel it's pretty easy to read and understand for others. :)

 

I do wonder if this would actually work with an SPI Slave device, I feel like the timing would be super off but im not sure how timing critical SPI slave devices usually are....or if they see the right ups and downs of the clock/mosi/etc... they can handle it anyways?

Last Edited: Thu. Dec 6, 2018 - 05:57 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your return value is wrong. You should left-shift ret_data before OR-ing the MISO bit.
And you are handling MISO completely wrong. MISO is a bit-position. You need a bit-mask like with all the other signals.
.
There are plenty of bit-banged SPI examples. Think about all the Arduino projects where the punter has wired up random pins.
.
David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:
Your return value is wrong. You should left-shift ret_data before OR-ing the MISO bit. And you are handling MISO completely wrong. MISO is a bit-position. You need a bit-mask like with all the other signals. . There are plenty of bit-banged SPI examples. Think about all the Arduino projects where the punter has wired up random pins. . David.
 

 

That's a good point, I didn't notice because I was just checking the MOSI with my LED example. Im not sure what you mean by the bit mask for MISO? Im going by what others above suggested (Which seems like it should work)

 

Also I know there are tons of SPI bit-bang examples, this was purely for a learning exercise (Trying to write software based off the timing diagram).

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I’m a total noob, and I didn’t catch the return value issues either until they were pointed out, but I think what David is trying to say is that MISO is a bit position value (0-7). Think if it happened to be a position value such as 3 for your particular MCU, that would be 0b00000011 in binary and would return a 1 if either pin 0 or pin 1 were high. Honesty, I’m not sure what portable mask could be used. You could just program in a hard value like 0x01 (or whatever your MISO pin is.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

KC7MMI wrote:
I’m a total noob, and I didn’t catch the return value issues either until they were pointed out, but I think what David is trying to say is that MISO is a bit position value (0-7). Think if it happened to be a position value such as 3 for your particular MCU, that would be 0b00000011 in binary and would return a 1 if either pin 0 or pin 1 were high. Honesty, I’m not sure what portable mask could be used. You could just program in a hard value like 0x01 (or whatever your MISO pin is.

 

Yeah the return value issue made sense to me. Also it's starting to make sense what MISO is...but I always thought of MISO as the return response from the slave device (Which could be a 1 or 0) but maybe im just mis-understanding MISO completely.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Which AVR chip did you have in mind for this?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

For the ATmega328P, MISO is Port B, bit 4. Proper use of the MISO constant would be:

DDRB &= !(1 << MISO); //setup MISO as input

My understanding for the proper way to read would be:

if (PINB & (1 << MISO)) {...} //do this if the MISO bit on the PINB register is high...

I think that would be the most portable way, please correct me if I’m wrong.

Last Edited: Mon. Dec 10, 2018 - 11:48 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

KC7MMI wrote:
DDRB &= !(1 << MISO); //setup MISO as input

No.   It should be:

DDRB &= ~(1 << MISO); //setup MISO as input

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you—my mistake. Still learning! :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think maybe there is some misunderstanding, Im not wanting to use the onboard MISO but instead doing a software version of MISO using GPIO. That was the point of this exercise anyways. (Pretend this chip didn't have a hardware SPI). If we are actually referring to the MISO I had set up...I thought I had that handled in my DDRB call (it should be set to input).

 

edit:

OH wait I see, Im stupid. Not it makes sense what you all are talking about ha! What I actually had made no sense. Wow I can't believe I overlooked that. I thought everyone was talking about the DDRB setup of MISO. You all are talking about actually checking the input on the MISO pin (Which i've defined as PC3 I believe) right?

Last Edited: Tue. Dec 11, 2018 - 03:40 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yep!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well, it looks like you defined MISO and MOSI, which probably isn’t a good idea as those are already AVR definitions. I hadn’t noticed that previously. Maybe rename as SOFT_MOSI and SOFT_MISO?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

KC7MMI wrote:
as those are already AVR definitions.
Really? The IO header for 328 only has these definitions relating to SPI:

#define SPCR _SFR_IO8(0x2C)
#define SPR0 0
#define SPR1 1
#define CPHA 2
#define CPOL 3
#define MSTR 4
#define DORD 5
#define SPE 6
#define SPIE 7

#define SPSR _SFR_IO8(0x2D)
#define SPI2X 0
#define WCOL 6
#define SPIF 7

#define SPDR _SFR_IO8(0x2E)
#define SPDR0 0
#define SPDR1 1
#define SPDR2 2
#define SPDR3 3
#define SPDR4 4
#define SPDR5 5
#define SPDR6 6
#define SPDR7 7

As there aren't actually registers/bits with the names "MISO", "MOSI" they are free to use - in fact the code would have thrown redefinition warnings if they weren't.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Interesting. I would have expected it to be defined. Either way, probably best to change the name a little bit so as to not cause the confusion that was perfectly demonstrated above. ;)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

KC7MMI wrote:
I would have expected it to be defined.
Why? the code itself (apart from this kind of "soft" implementation) never has any direct connection to MOSI/MISO. The software just configures the SPI with SPCR, then sends/receives data with SPDR and monitors activity with SPSR. The fact that writing SPDr happens to make the MOSI, SCK (and indirectly MISO) wires wiggle is almost completely incidental to the software interface.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
Yep!

 

If that's true why are they saying how I set my DDRB is wrong (Yeah it's an ugly way to do it, but it should work). I can't tell if my MISO input is incorrect (reading the pin) based off what people are saying...but it looks right.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If I were reviewing your code I'd make it:

uint8_t exchange_byte(uint8_t data_byte) {

	uint8_t i;
	unit8_t ret_data = 0;

	/* SCLK Currently holding low, MOSI is low, SS is low */
        ss_low(); // select slave
	for(i = 0; i < 8; i++) {
		sclk_low(); //Set Low
		if(data_byte & 0x80) {
			mosi_high();
		}
		else {
			mosi_low();
		}
		/* MOSI Value is latched in */
		sclk_high(); //Send High
		data_byte <<= 1; //MSB First
		if(PINB & (1 << MISO)) {
			ret_data |= 1;
		}
		ret_data <<= 1; //Shift Left
	}
	ss_high(); // Pull Slave-Select back up
	sclk_low(); // Set Clock back to idle (0 in our case for mode 0)
	return ret_data;

}

int main(void)
{
	/* Initialize the data direction register for Port B */
	BTN_DDR = 0;				/* Setting all pins of the button port to Input	*/
	BTN_PORT = (1 << BUTTON);				/*	PUR on PB1	*/
	/*													*/

    enable_SPI();						/* Set up correct Data Direction Ports */
    while (1)
    {
		exchange_byte('M');
    }
}

1) function renamed to exchange_byte() as SPI is always both a send and a receive.

2) I prefer to use stdint.h types so "unsigned char" becomes "uint8_t" - also ret_data changed to match the actual function return type! Also "i" should not be an "int" (16 bits) when it's only counting 0..7. It too should be uint8_t. In fact don't ever use just "int", "short", "long", "long long" but always think about the bit width (and signage) you need and using stdint types makes you think more about this.

3) initial _delay_ms(100) removed - no idea what it was doing there but would you really want a 100ms delay on every byte you exchange ?

4) ret_data initialised to 0 - it is a stack frame automatic and will inherit a random value - there's no guarantee it starts as 0

5) add ss_low() before for loop as you weren't selecting the slave before you started

6) loop waiting for button removed - why would iterations of the for() loop be dependent on this? In fact why would the caller of an SPI routine expect it to have any reliance on buttons?

7) _delay_ms(1000) removed - It will take EIGHT SECONDS to transfer 1 byte if you have this delay for each bit sent received ?!?!

8)

		if(PINB & MISO) {

is just plain wrong. MISO is defined as:

#define MISO		PINC3

and PINC3 in turn is simply defined as 3. So this test was doing:

		if(PINB & 3) {

which is actually testing the states of bits 0 and 1 not the intended test of bit 3. It should be:

		if(PINB & (1 << MISO)) {

9) Change comment on BTN_DDR setting - it's not just setting pin 1 to input it is setting all 8 bits of the port to be inputs.

10) change BTN_PORT setup to use the more symbolic "BUTTON" rather than hard coding 0b00000010 because if you later changed:

#define BUTTON		PINB5

then without using (1 << BUTTON) then this change to bit 5 would not be reflected in the BTN_PORT setup to turn on the pull up.

11) remove ss_low() from the main loop - this should be done more locally - either an ss_low/ss_high on either side of the call in main() or in the byte exchange function itself.

12) also change:

void enable_SPI(void) {
	SPI_DDR = 0b00110100;			/* Set SS/MOSI/SCLK to Output, MISO to Input */
}

to be:

void enable_SPI(void) {
	SPI_DDR |= (1 << SCLK) | (1 << MOSI) | (1 << SS);
}

again you have to do it like this because if the user later changes:

/* SPI Port Defines */
#define SCLK		PINC7
#define MOSI		PINC2
#define MISO		PINC6
#define SS			PINC0

then the SPI_DDR setting would not be updated accordingly if you don't use these previously defined symbols.

Last Edited: Tue. Dec 11, 2018 - 03:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the above clawson, a lot of these were def. silly mistakes because it's my first time doing something like this. A ton of the delays were added because (ignorantly) I felt like they needed them for setup time/etc..., Im still not good at figuring at timing/etc... and what's necessary. 

 

The button thing was added just for my visual debugging, to be able to see it change on LED's I had set up. (It's obviously not "correct" but I didn't have a LA to be able to test so that was the best I could do lol.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Don't know if this is of any interest...

 

 

Attachment(s): 

#1 This forum helps those that help themselves

#2 All grounds are not created equal

#3 How have you proved that your chip is running at xxMHz?

#4 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand." - Heater's ex-boss

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Mercfh wrote:

So ill preface this by saying this is most likely wrong, I don't have access to a Logic Analyzer or Oscilloscope (although an LA is on my "Buy next" list).

 

If you need a Logic Analyser, this looks promising - uses HS-USB capture.

https://www.instructables.com/id...

 

Those boards are under $4 on eBay / Aliexpress.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Who-me wrote:

Mercfh wrote:

So ill preface this by saying this is most likely wrong, I don't have access to a Logic Analyzer or Oscilloscope (although an LA is on my "Buy next" list).

 

If you need a Logic Analyser, this looks promising - uses HS-USB capture.

https://www.instructables.com/id...

 

Those boards are under $4 on eBay / Aliexpress.

I will probably grab one of these, for 4 bucks it can't hurt.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

These are a pain. There’s the Logic clones that are more useful. I find these invaluable. Just this week i had to sort out a i2c slave problem. You can buy the Salaea logic program or use Sigrok.

Last Edited: Fri. Dec 21, 2018 - 08:34 AM