 |
 |
| View previous topic :: View next topic |
| Author |
Message |
jeremiah
Joined: 20 Jul 2010
Posts: 1345
|
| i2c_read locks up on PIC24 after a restart with R/W set to 1 |
 Posted: Tue Jul 20, 2010 7:45 am |
 |
|
Pertinent Info:
PIC24 to PIC24 I2C setup
Both are PIC24FJ64GA004
Compiler completely up to date: 4.109
I2C hardware confirmed to work (used with EEPROM and basic PIC2PIC)
Slave Address = 0b1000000 (0x80 in 8-bit, 0x40 in 7-bit)
Problem:
I am mainly working on a slave application, but have setup a master app to facilitate testing as well. At first I thought my master code was hanging due to it not reading the next byte following a restart with an address+R/W set to 1 (i2c_write(0x81)). Using the debugger on the slave, I found that the slave was locking up on reading the address byte at that point. As a quick test, I changed the master to send address + R/W set to 0 (i2c_write(0x80)), and the slave worked fine (though of course the master didn't since I wasn't writing data back in that config).
I know you normally only read one byte at a time on an interrupt, but the application this is working towards actually requires doing it all at once like that. It works fine as long as I don't send a i2c_write(0x81).
I can't figure out why the slave is hickuping on reading the address byte in the i2c_write(0x81) scenario.
Here is the slave code. The variable changed is used to facilitate printing data in the timer code later on. The master is programmed to send the string of data once after being on for about 10 seconds. The printing happens somewhere between then and 2 seconds later.
| Code: |
#include <24FJ64GA004.h>
#device *=16
#FUSES NOWDT //No Watch Dog Timer
#FUSES NOJTAG //JTAG disabled
#FUSES NOPROTECT //Code not protected from reading
#FUSES NOWRT //Program memory not write protected
#FUSES NODEBUG //No Debug mode for ICD
#FUSES ICSP1 //ICD uses PGC1/PGD1 pins
#FUSES NOIOL1WAY //Allows multiple reconfigurations of peripheral pins
#FUSES WINDIS //Watch Dog Timer in non-Window mode
#FUSES WPRES128 //Watch Dog Timer PreScalar 1:128
#FUSES WPOSTS16 //Watch Dog Timer PostScalar 1:32768
#FUSES NOIESO //Internal External Switch Over mode enabled
#FUSES PR //Pimary oscillaotr enabled
#FUSES NOCKSFSM //Clock Switching is disabled, fail Safe clock monitor is disabled
#FUSES NOOSCIO //OSC2 is clock output
#FUSES HS
#FUSES I2C1SELD
#use delay(clock=22118400)
#use i2c(Slave,sda=PIN_B9,scl=PIN_B8, FORCE_HW, address=0x40)
#word SPI1STAT = 0x240
#word SPI2STAT = 0x260
#word I2C1CON = 0x206
#word I2C1STAT = 0x208
#word I2C2CON = 0x216
#word I2C2STAT = 0x218
#bit SCL1REL = I2C1CON.12
#bit SCL2REL = I2C2CON.12
#bit I2C1STOPRX = I2C1STAT.4
#bit I2C2STOPRX = I2C2STAT.4
#bit I2C1DATARX = I2C1STAT.5
#bit I2C2DATARX = I2C2STAT.5
#pin_select U1TX = PIN_C1 //C1
#pin_select U1RX = PIN_B3 //B3
#use rs232(UART1,baud=9600,parity=N,bits=8,DISABLE_INTS)
#define UART_Vcc PIN_C0 //PIC controlled UART power
#define MAX_RAW_MSG_LEN 1000
//Variables used for the msg. Named as a temp.
volatile unsigned int8 raw_msg[MAX_RAW_MSG_LEN];
volatile BYTE msg_state[MAX_RAW_MSG_LEN];
volatile BYTE msg_start[MAX_RAW_MSG_LEN];
volatile unsigned int8 changed = 0;
volatile unsigned int32 ssp_count = 0;
volatile unsigned int32 bytes_read = 0;
void do_ssp(){
//unsigned int8 state = 0x00;
int32 timeout = 0;
int16 byte_count = 0;
ssp_count++;
while(byte_count < MAX_RAW_MSG_LEN && timeout < 200000){
if(i2c_poll() == TRUE){
msg_state[byte_count]=i2c_isr_state();
msg_start[byte_count]=I2C1DATARX;
raw_msg[byte_count++]=i2c_read(); //locks up here if 0x81 was used as addr+R/W bit (from Master)
timeout = 0;
}else{
timeout++;
delay_us(10);
}
}
bytes_read = byte_count;
changed = 1;
SCL1REL = 1; //needed to release the clock since interrupt sometimes takes too long
}
#INT_SI2C
void ssp_interrupt ()
{
do_ssp();
}
#int_timer1
void timer1_isr(void)
{
int32 i = 0;
output_toggle(PIN_B12);
if(changed == 1){
printf("ssp_count = %d\r\n",ssp_count);
printf("\r\n\r\n");
changed = 0;
for(i=0; i<(bytes_read); i++){
printf("%x | %x --> %x\r\n",msg_state[i],msg_start[i],raw_msg[i]);
}
printf("\r\n\r\n");
}
}
void main()
{
//This line serves no function other than to remove an annoying warning.
rs232_errors = 0;
output_high(UART_Vcc); //Turns on serial port
delay_ms(100);
setup_spi(SPI_SS_DISABLED);
SPI1STAT = 0;
setup_spi2(SPI_SS_DISABLED);
SPI2STAT = 0;
setup_wdt(WDT_OFF);
setup_timer2(TMR_DISABLED);
setup_timer3(TMR_DISABLED);
setup_timer4(TMR_DISABLED);
setup_timer5(TMR_DISABLED);
setup_adc(ADC_OFF);
setup_vref(VREF_DISABLED);
//Turn on SOSC
#asm
MOV #0x742, W1
MOV.B #0x02, W0
MOV #0x46, W2
MOV #0x57, W3
MOV.B W2,[W1]
MOV.B W3,[W1]
MOV.B W0,[W1]
#endasm
setup_timer1(TMR_EXTERNAL|TMR_DIV_BY_1); //every 2 seconds
set_timer1(0);
output_low(PIN_B12);
output_low(PIN_B13);
output_float(PIN_B8);
output_float(PIN_B9);
enable_interrupts(INT_SI2C);
enable_interrupts(INT_TIMER1);
enable_interrupts(INTR_GLOBAL);
while (TRUE) {}
}
|
MASTER code:
| Code: |
#include <24FJ64GA004.h>
#device *=16
#FUSES NOWDT //No Watch Dog Timer
#FUSES NOJTAG //JTAG disabled
#FUSES NOPROTECT //Code not protected from reading
#FUSES NOWRT //Program memory not write protected
#FUSES NODEBUG //No Debug mode for ICD
#FUSES ICSP1 //ICD uses PGC1/PGD1 pins
#FUSES NOIOL1WAY //Allows multiple reconfigurations of peripheral pins
#FUSES WINDIS //Watch Dog Timer in non-Window mode
#FUSES WPRES128 //Watch Dog Timer PreScalar 1:128
#FUSES WPOSTS16 //Watch Dog Timer PostScalar 1:32768
#FUSES NOIESO //Internal External Switch Over mode enabled
#FUSES PR //Pimary oscillaotr enabled
#FUSES NOCKSFSM //Clock Switching is disabled, fail Safe clock monitor is disabled
#FUSES NOOSCIO //OSC2 is clock output
#FUSES HS
#FUSES I2C1SELD
#use delay(clock=22118400)
#use i2c(Master,sda=PIN_B9,scl=PIN_B8)
#word SPI1STAT = 0x240
#word SPI2STAT = 0x260
#pin_select U1TX = PIN_C1 //C1
#pin_select U1RX = PIN_B3 //B3
#use rs232(UART1,baud=9600,parity=N,bits=8,DISABLE_INTS)
#define UART_Vcc PIN_C0 //PIC controlled UART Power
unsigned int8 count=0;
#int_timer1
void timer1_isr(void)
{
/*
NOTE: I realize I am not reading after sending the restart with R/W set to 1.
The slave isn't getting past reading in the address in order to write
another byte. I initially had a read in there but nothing was ever
sent back to the master to be read, so it was pointless.
NOTE2: the delay_us(50) lines aren't really needed. It works the same without
them. I added them in upon request from CCSINFO tech support.
*/
output_toggle(PIN_B12);
if(count == 5){ //fires after first 10 seconds (and on overflows)
i2c_start();
i2c_write(0x80); //Address & R/W
delay_us(50);
i2c_write(0x01); //Protocol
delay_us(50);
i2c_write(0x02); //header length
delay_us(50);
i2c_write(0x00); //SID
delay_us(50);
i2c_write(0xFF); //Header Data
delay_us(50);
i2c_write(0x34); //Checksum (not implemented yet)
delay_us(50);
i2c_start();
delay_us(50);
i2c_write(0x80); //This is the line works fine (if you change it to 0x81, the slave locks up)
delay_us(50);
i2c_write(0x48);
delay_us(50);
i2c_start();
delay_us(50);
i2c_write(0x81); //slave doesn't lock up if you comment out this line
delay_us(50);
i2c_stop();
output_toggle(PIN_B13);
//count = 0;
}
count++;
}
void main()
{
//This line serves no function other than to remove an annoying warning.
rs232_errors = 0;
output_high(UART_Vcc); //Turns on serial port
setup_spi(SPI_SS_DISABLED);
SPI1STAT = 0;
setup_spi2(SPI_SS_DISABLED);
SPI2STAT = 0;
setup_wdt(WDT_OFF);
setup_timer2(TMR_DISABLED);
setup_timer3(TMR_DISABLED);
setup_timer4(TMR_DISABLED);
setup_timer5(TMR_DISABLED);
setup_adc(ADC_OFF);
setup_vref(VREF_DISABLED);
//Turn on SOSC
#asm
MOV #0x742, W1
MOV.B #0x02, W0
MOV #0x46, W2
MOV #0x57, W3
MOV.B W2,[W1]
MOV.B W3,[W1]
MOV.B W0,[W1]
#endasm
setup_timer1(TMR_EXTERNAL|TMR_DIV_BY_1); //every 2 seconds
set_timer1(0);
output_low(PIN_B12);
output_low(PIN_B13);
enable_interrupts(INT_TIMER1);
enable_interrupts(INTR_GLOBAL);
while(TRUE){
sleep();
}
}
|
Any help or insight would be appreciated. I am looking into how to do it manually at the moment, but would prefer to use the library functions if possible. Alternately, if anyone has a listing of handmade libraries, that would be a helpful reference as I explore the manual route (to help understand what bits need to be set in what sequence).
Thanks! |
|
 |
Ttelmah
Joined: 11 Mar 2010
Posts: 19504
|
|
| Posted: Tue Jul 20, 2010 8:36 am |
|
|
The obvious missing thing, is NACK.
When the master reads from the slave device (which is what 0x81 'triggers', it needs to acknowledge each byte after this point (the default), and then _NACK_ the very last byte before the stop. This is part of the I2C 'spec', and without it, the hardware _will_ get confused.
If you look at the example drivers. For instance the one for the 24128, you will see that you have:
| Code: |
i2c_write((0xa1)|(hi(address)>>5));
data=i2c_read(0);
i2c_stop();
|
Once it turns the bus around (sending '0xa1', to set the 'read' bit), then last read before the stop, has a '0', which says 'send NACK'.
Best Wishes |
|
|
jeremiah
Joined: 20 Jul 2010
Posts: 1345
|
|
| Posted: Tue Jul 20, 2010 8:55 am |
|
|
| Ttelmah wrote: | The obvious missing thing, is NACK.
When the master reads from the slave device (which is what 0x81 'triggers', it needs to acknowledge each byte after this point (the default), and then _NACK_ the very last byte before the stop. This is part of the I2C 'spec', and without it, the hardware _will_ get confused.
If you look at the example drivers. For instance the one for the 24128, you will see that you have:
| Code: |
i2c_write((0xa1)|(hi(address)>>5));
data=i2c_read(0);
i2c_stop();
|
Once it turns the bus around (sending '0xa1', to set the 'read' bit), then last read before the stop, has a '0', which says 'send NACK'.
Best Wishes |
Thanks! I'll work that in.
The thing that confuses me about the suggestion though is that the slave is locking up on just reading the address coming in from the master. The slave code doesn't even get to a point where it can write data to the master, so I am unsure how not sending a NACK would cause that as the master isn't getting any data to read in order to NACK (since the slave locks up before it is able to write any data on the line). |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19504
|
|
| Posted: Tue Jul 20, 2010 9:22 am |
|
|
The problem here is your I2C handler in the slave. When the address is received 'expecting' data to come back, you have to load the return data in the same transaction.
When you read 'I2C_ISR_STATE', if it is >=0x80, you need to load a byte before exiting the ISR, and you don't want to be reading bytes, except on the very first transaction (none are waiting to be read...). If you try to read after this, the slave ISR will hang.
Look at the manual entry for I2C_ISR_STATE. It has an example, showing what the ISR has to do. You don't want to be reading I2C_POLL (the fact the ISR has been called, says either a byte has been received, and needs to be read, or a byte has gone the other way, and another needs to be written). Notice, that if state=0, the address is read, and nothing else. If it =80, the address is read, _and_ the byte is written to be sent back, all in the same transaction.
Best Wishes |
|
|
jeremiah
Joined: 20 Jul 2010
Posts: 1345
|
|
| Posted: Tue Jul 20, 2010 10:01 am |
|
|
I think you are misunderstanding what I am doing. The ISR isn't reading just one byte and finishing. It is interrupting on the first byte and then reading in the whole stream via a loop. This is why I use the i2c_poll function, to make sure there is data on the i2c_bus (or else it could lock up). Past the first byte, you cannot guarantee there will be more data.
I looked at the manual, and what it shows is when the i2c_isr_state() returns a 0x80, you read the byte first and then write the data second:
| Code: |
void i2c_isr(){
state = i2c_isr_state();
if((state == 0) || (state == 0x80)
i2c_read();
if(state >= 80)
i2c_write(send_buff[state-0x80]);
else if (state > 0)
rcv_buffer[state-1] = i2c_read();
}
|
That's what the manual specifies for the i2c_isr_state example. It shows you read the address first and then write the data byte. I'll try just writing the data byte and see what happens.
What I am doing is:
Interrupt on the first data (actually the address).
enter while loop
poll to make sure data is there to read (redundant the first time only)
read data
respond back if 0x81
keep looping until no data
eventually process that data.
I went back and implemented your previous suggestions (adding the NACK and making sure the slave writes data back). The slave still hangs when it reads the address, so I will try skipping the read in that case. It will bother me if that works though since it works if you send 0x80 as the address instead.
Here are the code changes I made to test so far:
slave code loop change
| Code: |
output_low(PIN_B13);
delay_us(6);
while(byte_count < MAX_RAW_MSG_LEN && timeout < 200000){
if(i2c_poll() == TRUE){
output_toggle(PIN_B13);
msg_state[byte_count]=i2c_isr_state();
msg_start[byte_count]=I2C1DATARX;
delay_us(6);
output_toggle(PIN_B13);
if(msg_state[byte_count] == 0x80 && msg_start[byte_count] == 0){ //this should pic up i2c_write(0x81)
delay_us(2);
output_toggle(PIN_B13);
delay_us(2);
output_toggle(PIN_B13);
raw_msg[byte_count++]=i2c_read(); //locks up here if 0x81 was used as addr+R/W bit (from Master) **STILL locks up
delay_us(10);
output_toggle(PIN_B13);
delay_us(10);
output_toggle(PIN_B13);
i2c_write(0x28);
}else{
delay_us(4);
output_toggle(PIN_B13);
delay_us(4);
output_toggle(PIN_B13);
raw_msg[byte_count++]=i2c_read(); //locks up here if 0x81 was used as addr+R/W bit (from Master)
}
timeout = 0;
}else{
timeout++;
delay_us(10);
}
}
|
master if statement changes:
| Code: |
if(count == 5){ //fires after first 10 seconds (and on overflows)
i2c_start();
i2c_write(0x80); //Address & R/W
delay_us(50);
i2c_write(0x01); //Protocol
delay_us(50);
i2c_write(0x02); //header length
delay_us(50);
// i2c_write(0x00); //SID
// delay_us(50);
// i2c_write(0xFF); //Header Data
// delay_us(50);
/// i2c_write(0x34); //Checksum (not implemented yet)
// delay_us(50);
i2c_start();
i2c_write(0x80); //This is the line works fine
delay_us(50);
i2c_write(0x48);
delay_us(50);
i2c_start();
i2c_write(0x81); //slave doesn't lock up if you comment out this line
delay_us(50);
temp = i2c_read(0); //declared at top of function
delay_us(50);
i2c_stop();
output_toggle(PIN_B13);
//count = 0;
}
|
I'll work on taking out the read and only doing the write for the 0x81 case. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19504
|
|
| Posted: Tue Jul 20, 2010 10:19 am |
|
|
No, do what it shows.
At present, you are not loading a byte for the read from the master. This will hang the USB.
Once you have read the address, you have to load the return byte immediately.
You are sitting inside the ISR, looping - don't.
Keep your byte count as a 'static' variable, and it'll be there the next time the I2C_ISR is called.
Best Wishes |
|
|
jeremiah
Joined: 20 Jul 2010
Posts: 1345
|
|
| Posted: Tue Jul 20, 2010 10:43 am |
|
|
Staying in the ISR too long shouldn't be a problem. The whole purpose of the pic is just to respond to the I2C command structure. The main won't be doing anything and there won't be any other interrupts and there will be only the master computer and this slave on the bus.
I know it is normally a no-no, but in this case it shouldn't provide any problems.
In either case, I really appreciate your input. It has been very helpful.
I did receive an email from someone at CCS about it. He said after reviewing:
| Quote: |
I determined that the problem your having is due to a bug in the i2c_read() function that cause the function to get stuck when perform the read on the Slave after receiving a read command from the Master. I have submitted the bug to my boss and will let you know when it is fixed.
|
He provided a workaround involving the registers until it is fixed:
| Code: |
#word I2C1RCV = getenv("SFR:I2C1RCV")
#bit I2C1R_W = I2C1STAT.2
|
| Code: |
void do_ssp(){
//unsigned int8 state = 0x00;
int32 timeout = 0;
int16 byte_count = 0;
ssp_count++;
while(byte_count < MAX_RAW_MSG_LEN && timeout < 200000){
if(i2c_poll() == TRUE){
msg_state[byte_count]=i2c_isr_state();
msg_start[byte_count]=I2C1DATARX;
if(I2C1R_W){
raw_msg[byte_count++] = I2C1RCV;
i2c_write(0xAA); //data to send to master I used 0xAA for testing
}else{
raw_msg[byte_count++]=i2c_read();
}
timeout = 0;
}else{
timeout++;
delay_us(10);
}
}
bytes_read = byte_count;
changed = 1;
SCL1REL = 1;
}
|
I'll still have to modify it some obviously.
Again, thanks for the help! |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
