I2C not working on PIC18F

[newtekuser]

Porting my project from PIC16F887 to PIC18F64K40 and having issues getting I2C to work with the device crashing on I2C read to DS3231 RTC IC. I know it's a code configuration issue because the same hardware setup works fine if I swap the MCUs on the breadboard but I can't figure out why even after spending hours on end reading the Datasheet and trying different things.
I hooked up a logic analyzer on SCL and SDA lines and no packets are decoded though there are some signals going high/low. I'm thinking it sees garbage because the logic analyzer decodes fine if I swap the PIC16F back on the breadboard.

The PIC is configured to use the internal oscillator at 8 MHz, _XTAL_FREQ is set to 8000000 and i2c is initialized at 100KHz, same as the PIC16F.

Any clues/insights will be highly appreciated.

I2C code implementation:

Code: [Select]

void I2C_Init(uint32_t i2c_clk_freq) {   
    TRISCbits.TRISC3 = 1;
    TRISCbits.TRISC4 = 1;

    RC3PPS = 0x0F;
    SSP1CLKPPS = 0x13;

    RC4PPS = 0x10;
    SSP1DATPPS = 0x14;

    SSP1CON1bits.SSPEN = 0;

    SSP1CON1bits.SSPM = 0b1000;

    SSP1STATbits.SMP = 1;

    SSP1ADD = ((unsigned char)_XTAL_FREQ / (4 * i2c_clk_freq)) - 1;

    SSP1CON1bits.SSPEN = 1;
}

void I2C_Start(void) {
    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));

    SSP1CON2bits.SEN = 1; 
   
    while (SSP1CON2bits.SEN);
}

void I2C_Repeated_Start(void) {
    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));

    SSP1CON2bits.RSEN = 1; 
   
    while (SSP1CON2bits.RSEN);
}

void I2C_Stop(void) {
    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));

    SSP1CON2bits.PEN = 1; 
   
    while (SSP1CON2bits.PEN);
}

void I2C_Write(uint8_t i2c_data) {
    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F)); 

    SSP1BUF = i2c_data; 
   
    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F)); 

}

uint8_t I2C_Read(uint8_t ack) {
    uint8_t _data;

    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F)); 

    SSP1CON2bits.RCEN = 1;     

    while (!SSP1STATbits.BF); 
   
    _data = SSP1BUF; 

    while (SSP1CON2bits.ACKEN);
    SSP1CON2bits.ACKDT = (ack) ? 0 : 1;
    SSP1CON2bits.ACKEN = 1;
   
    while (SSP1CON2bits.ACKEN);

    return _data;
}
Calling I2C_Read() crashes the device, if I comment it out it does not.

Code: [Select]

    I2C_Start();
    I2C_Write(DS3231_ADDRESS_WRITE);
    I2C_Write(0);
    I2C_Repeated_Start();
    I2C_Write(DS3231_ADDRESS_READ);
    data = I2C_Read(0); // <----- this crashes the device
    I2C_Stop();

[Sacodepatatas]

I recall that the I2C in the PIC18F is very tricky. Once upon a time, i had to start the I2C module, then reset the device, start again, insert a certain delay (10ms or so) and then the I2C worked fine. Also if the ADC has the I2C pins configured as analog inputs, then the I2C module fails, so the code must tell the ADC that those lines are digital IOs. TRIS=1 doesn't perform all the operations required for that.

[Buriedcode]

When you say "crash" you mean the oscillator stops?  Or the code is stuck in a loop whilst it waits for a bit to be set/clear?  Just asking because the two are not the same.  And am I right in thinking, it worked with the '887, but not the PIC18?

This is where a hardware debugger really helps, or the poor mans equivalent - adding macro breakpoints/indication in the software, to make a pin high, or light an LED whilst it's in a function.  You can then see this, or measure the frequency/duty to see roughly how long the code is in that function/state.

First of all, strip it back - can you get the MCU to run - twiddle IO and measure it, so you know the oscillator is working.  Then you work with the GPIO you want to use - check that you can both set its output, and read from it, some pins are configured as analogue by default.  Then you step by step, check each of the I2C routines.  Start, write etc..

It sounds arduous, but breaking it down into small simple tasks means you can methodically work through the problem over a cup of coffee rather than stare at code hoping for inspiration.

(small note: I hate I2C always have. I can get it to work on most PICs/AVRs, but avoid it if I can).

[newtekuser]

I recall that the I2C in the PIC18F is very tricky. Once upon a time, i had to start the I2C module, then reset the device, start again, insert a certain delay (10ms or so) and then the I2C worked fine. Also if the ADC has the I2C pins configured as analog inputs, then the I2C module fails, so the code must tell the ADC that those lines are digital IOs. TRIS=1 doesn't perform all the operations required for that.

Good to know. I am disabling all analog inputs at the very beginning of main().

Code: [Select]

ANSELA = 0x00;
ANSELB = 0x00;
ANSELC = 0x00;
ANSELD = 0x00;
ANSELE = 0x00;

[newtekuser]

When you say "crash" you mean the oscillator stops?  Or the code is stuck in a loop whilst it waits for a bit to be set/clear?  Just asking because the two are not the same.  And am I right in thinking, it worked with the '887, but not the PIC18?

This is where a hardware debugger really helps, or the poor mans equivalent - adding macro breakpoints/indication in the software, to make a pin high, or light an LED whilst it's in a function.  You can then see this, or measure the frequency/duty to see roughly how long the code is in that function/state.

First of all, strip it back - can you get the MCU to run - twiddle IO and measure it, so you know the oscillator is working.  Then you work with the GPIO you want to use - check that you can both set its output, and read from it, some pins are configured as analogue by default.  Then you step by step, check each of the I2C routines.  Start, write etc..

It sounds arduous, but breaking it down into small simple tasks means you can methodically work through the problem over a cup of coffee rather than stare at code hoping for inspiration.

(small note: I hate I2C always have. I can get it to work on most PICs/AVRs, but avoid it if I can).

Upon doing more experimenting it looks like the device itself does not crash (locking up) as I initially thought as I have a timer running that turns off my 16x2 LCD after 30 sec and that still works. And yes, the same setup on the breadboard still works if I just swap the PIC16 back on it. Of course, for the PIC18F I had to modify a lot of stuff to get it to the point where I can see the menu on the LCD and navigate it, the timers, and other things I have going on.
The behavior I described as a crash is in that the LCD goes blank once I enter the clock function (which just reads the time off the RTC and displays it) and the menu system no longer gets displayed. This doesn't happen if I enter other menus, and of course it doesn't happen on the PIC16F.

I also tried to do an unlock before messing with the port mappings but that didn't have an effect either. I am in the process of stripping the project down to bare-bones and remove all functionality including the LCD menu and just do the i2c reads and debug with LEDs, or maybe UART but I have a feeling I'm going to run into similar issues as with i2c.


   

Code: [Select]

PPSLOCK = 0x55;
    PPSLOCK = 0xAA;
    PPSLOCKED = 0;

    RC3PPS = 0x0F;
    SSP1CLKPPS = 0x13;
    RC4PPS = 0x10;
    SSP1DATPPS = 0x14;

    PPSLOCK = 0x55;
    PPSLOCK = 0xAA;
    PPSLOCKED = 1;

LE: Surprisingly I got UART communication working on the PIC18F with little effort and I didn't have to mess with the PPS at all. Now onto the I2C issue...

[newtekuser]

Fixed it, the issue was related to timing. Once I added a 2ms delay between each i2c calls (which I don't have to do when using the PIC16F), everything started behaving otherwise i2c would lock up before the first i2c read call.

[Atlan]

Missing ack or stop bit? Fsck i2c? Support 400khz?

[JPortici]

As i recall the K40 series uses the old MSSP and not the newer I2C peripheral introduced in the K42 series... So the code should have been the same and behaved the same as in the PIC16
Wonder if your timing problem is something waiting to happen in the other design as well (you didn't give much info anyway... like pointing out where in the Read function execution stops, waveforms...)
 you definetly should not need to add arbitrary delays

[newtekuser]

As i recall the K40 series uses the old MSSP and not the newer I2C peripheral introduced in the K42 series... So the code should have been the same and behaved the same as in the PIC16
Wonder if your timing problem is something waiting to happen in the other design as well (you didn't give much info anyway... like pointing out where in the Read function execution stops, waveforms...)
 you definetly should not need to add arbitrary delays

When I debugged this I started a new project completely from scratch with just the the I2C code I shared in my first post and the problem was still there as nothing would come out on the UART terminal when reading time values. Sometimes on power-on it would manage to do the only first I2C write with the DS3231 address, other times get to first i2c read but then and blow up and not get passed that.
Adding these small delays is what fixed it for me. There's nothing else going on in this version of the project, no timers, no interrupts as in the initial port etc.

Code: [Select]

// read current time and date from the RTC chip
    I2C_Start();
    __delay_ms(2);
    I2C_Write(DS3231_ADDRESS_WRITE);
    __delay_ms(2);
    I2C_Write(0);
    __delay_ms(2);
    I2C_Repeated_Start();
    __delay_ms(2);
    I2C_Write(DS3231_ADDRESS_READ);
    __delay_ms(2);
    second = I2C_Read(1);
    __delay_ms(2);
    minute = I2C_Read(1);
    __delay_ms(2);
    hour   = I2C_Read(0);
    I2C_Stop();
    __delay_ms(2);

[woody]

I have to agree with @JPortici that adding delays might solve it for now but does not fix the root cause of this problem.

Things to check:
- Pullup resistors. You probably did but they bit me enough times to mention them
- Vcc and device type as there seems to be a problem in using I2C when Vcc>3.0V on LF devices (according to the errata)

What always gets me out of I2C problems is looking at the signals with a scope. It immediately shows you frequency and rise times  are as expected.

[newtekuser]

I have to agree with @JPortici that adding delays might solve it for now but does not fix the root cause of this problem.

Things to check:
- Pullup resistors. You probably did but they bit me enough times to mention them
- Vcc and device type as there seems to be a problem in using I2C when Vcc>3.0V on LF devices (according to the errata)

What always gets me out of I2C problems is looking at the signals with a scope. It immediately shows you frequency and rise times  are as expected.

Pull-ups are 10K, I went down to 1.5K but no difference in behavior. My PIC18F is the non-LF version and both ICs (PIC and RTC) are powered by 5V. I shall transplant the PIC18 onto the PCB that has the PIC16 instead of using the breadboard before poking with the scope. I found that most of measurements with the scope are off on the breadboard anyway.

[DavidAlfa]

I found nothing about the PIC18F64K40, maybe a typo? PIC18F46K40?
I doubt it's crashing, probably getting locked in one of the while() statements.
Don't you have debugger to see where it's halting?

Still, you can debug with leds or showing data in the display:

Code: [Select]

uint8_t I2C_Read(uint8_t ack) {
    uint8_t _data;
    LCD_Send("R1");
    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));
    LCD_Send("R2");
    SSP1CON2bits.RCEN = 1;     
    LCD_Send("R3");
    while (!SSP1STATbits.BF);
    LCD_Send("R4");   
    _data = SSP1BUF;
    LCD_Send("R5");
    while (SSP1CON2bits.ACKEN);   
    LCD_Send("R6");
    SSP1CON2bits.ACKDT = (ack) ? 0 : 1;
    SSP1CON2bits.ACKEN = 1;
    LCD_Send("R7");
    while (SSP1CON2bits.ACKEN);
    return _data;
}

Anyways, try:
https://github.com/tinfever/FU-Dyson-BMS/blob/main/firmware/i2c.h
https://github.com/tinfever/FU-Dyson-BMS/blob/main/firmware/i2c.c

I see the code was improved, but only for SSP1, so use that!

[newtekuser]

I found nothing about the PIC18F64K40, maybe a typo? PIC18F46K40?
I doubt it's crashing, probably getting locked in one of the while() statements.
Don't you have debugger to see where it's halting?

Still, you can debug with leds or showing data in the display:

Code: [Select]

uint8_t I2C_Read(uint8_t ack) {
    uint8_t _data;
    LCD_Send("R1");
    while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));
    LCD_Send("R2");
    SSP1CON2bits.RCEN = 1;     
    LCD_Send("R3");
    while (!SSP1STATbits.BF);
    LCD_Send("R4");   
    _data = SSP1BUF;
    LCD_Send("R5");
    while (SSP1CON2bits.ACKEN);   
    LCD_Send("R6");
    SSP1CON2bits.ACKDT = (ack) ? 0 : 1;
    SSP1CON2bits.ACKEN = 1;
    LCD_Send("R7");
    while (SSP1CON2bits.ACKEN);
    return _data;
}

Anyways, try:
https://github.com/tinfever/FU-Dyson-BMS/blob/main/firmware/i2c.h
https://github.com/tinfever/FU-Dyson-BMS/blob/main/firmware/i2c.c

I see the code was improved, but only for SSP1, so use that!

Typo, it's the PIC18F46K40. As for my findings, this is what I posted earlier:

When I debugged this I started a new project completely from scratch with just the the I2C code I shared in my first post and the problem was still there as nothing would come out on the UART terminal when reading time values. Sometimes on power-on it would manage to do the only first I2C write with the DS3231 address, other times get to first i2c read but then and blow up and not get passed that.
Adding these small delays is what fixed it for me. There's nothing else going on in this version of the project, no timers, no interrupts as in the initial port etc.

Code: [Select]

// read current time and date from the RTC chip
    I2C_Start();
    __delay_ms(2);
    I2C_Write(DS3231_ADDRESS_WRITE);
    __delay_ms(2);
    I2C_Write(0);
    __delay_ms(2);
    I2C_Repeated_Start();
    __delay_ms(2);
    I2C_Write(DS3231_ADDRESS_READ);
    __delay_ms(2);
    second = I2C_Read(1);
    __delay_ms(2);
    minute = I2C_Read(1);
    __delay_ms(2);
    hour   = I2C_Read(0);
    I2C_Stop();
    __delay_ms(2);

[woody]

I must admit that this intrigues me

I looked through your code and there was one thing I could not place:

Code: [Select]

while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));     <-------?

    SSP1CON2bits.SEN = 1;
    while (SSP1CON2bits.SEN);

What is this line doing? AFAIK to generate a start condition you would only need

Code: [Select]

    SSP1CON2bits.SEN = 1;
    while (SSP1CON2bits.SEN);

You have this line in every I2C sequence. I would expect you only check the buffer full status bit in i2c_read() and i2c_write() to find if receive or transmit is complete.

Mind, I could have this very wrong due to my lack of knowledge of this particular chip, but as your code is hanging more or less random on most likely a while() it made me wonder.




 

[Sacodepatatas]

I must admit that this intrigues me

I looked through your code and there was one thing I could not place:

Code: [Select]

while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));     <-------?

    SSP1CON2bits.SEN = 1;
    while (SSP1CON2bits.SEN);

What is this line doing?

It's just a standard check for ensuring there's no any ongoing bus operation before activating a new one, because the I2C module can't enqueue them.

[woody]

I tested this with the compiler I use for PIC code. This software has internal functions for i2c and it does not do this check. Not in code generated for the 16F887 and neither for the 18F46K40. For instance, when I look at the generated code for an i2c_start() on the latter chip I see:

Code: [Select]

000A8:  BSF    F97.0
000AA:  BTFSC  F97.0
000AC:  BRA    00AA

This being the equivalent of

Code: [Select]

    SSP1CON2bits.SEN = 1;
    while (SSP1CON2bits.SEN);

In both datasheets there is a warning regarding the queuing of events but that is prevented by waiting for the start / stop / read / write to finish and not by checking the buffer beforehand. So I am still wondering if the

Code: [Select]

while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));

is messing stuff up.

[newtekuser]

I tested this with the compiler I use for PIC code. This software has internal functions for i2c and it does not do this check. Not in code generated for the 16F887 and neither for the 18F46K40. For instance, when I look at the generated code for an i2c_start() on the latter chip I see:

Code: [Select]

000A8:  BSF    F97.0
000AA:  BTFSC  F97.0
000AC:  BRA    00AA

This being the equivalent of

Code: [Select]

    SSP1CON2bits.SEN = 1;
    while (SSP1CON2bits.SEN);

In both datasheets there is a warning regarding the queuing of events but that is prevented by waiting for the start / stop / read / write to finish and not by checking the buffer beforehand. So I am still wondering if the

Code: [Select]

while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));

is messing stuff up.

If I remove all delays and both checks, or remove all delays and leave only 'SSP1CON2 & 0x1F', then I get invalid values from the RTC chip. This is what the UART console spits out per the screenshot attached but at least it doesn't lock up.
If I remove all delays and only check for 'SSP1STATbits.BF', I get the same behavior as before where it randomly locks up in different places.

[woody]

I am not behind a workstation atm, but from the top of my head and in short:

-You do not want to check BF when doing start, restart or stop. Just set the SEN/PEN bits and wait for them to clear with the while()
-You do have to check BF upon sending a byte, where the flag is set after a write to SSPxBUF and cleared after sending is complete, and upon receiving a byte, where the flag is set after a data byte or address is loaded into SSPxBUF and cleared after a read from SSPxBUF.

[newtekuser]

I am not behind a workstation atm, but from the top of my head and in short:

-You do not want to check BF when doing start, restart or stop. Just set the SEN/PEN bits and wait for them to clear with the while()
-You do have to check BF upon sending a byte, where the flag is set after a write to SSPxBUF and cleared after sending is complete, and upon receiving a byte, where the flag is set after a data byte or address is loaded into SSPxBUF and cleared after a read from SSPxBUF.

Unfortunately that still didn't work and locks up as before.

Code: [Select]

void I2C_Init(uint32_t i2c_clk_freq) {   
    TRISCbits.TRISC3 = 1;
    TRISCbits.TRISC4 = 1;
    RC3PPS = 0x0F;
    SSP1CLKPPS = 0x13;

    RC4PPS = 0x10;
    SSP1DATPPS = 0x14;

    SSP1CON1bits.SSPEN = 0;

    SSP1CON1bits.SSPM = 0b1000; // I²C Master mode

    SSP1STATbits.SMP = 1;

    SSP1ADD = ((unsigned char)_XTAL_FREQ / (4 * i2c_clk_freq)) - 1;

    SSP1CON1bits.SSPEN = 1;
}

void I2C_Start(void) {
    while (SSP1CON2 & 0x1F);

    // Initiate start condition
    SSP1CON2bits.SEN = 1; 
    while (SSP1CON2bits.SEN);
}

void I2C_Repeated_Start(void) {
    //while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));
    while (SSP1CON2 & 0x1F);
   
    SSP1CON2bits.RSEN = 1; 
   
    while (SSP1CON2bits.RSEN);
}

void I2C_Stop(void) {
    //while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));
    while (SSP1CON2 & 0x1F);
   
    SSP1CON2bits.PEN = 1; 
   
    while (SSP1CON2bits.PEN);
}

void I2C_Write(uint8_t i2c_data) {
    while ((SSP1STATbits.R_nW) || (SSP1CON2 & 0x1F));

    SSP1BUF = i2c_data; 

    while (!PIR3bits.SSP1IF);
    while (SSP1STATbits.BF);


    PIR3bits.SSP1IF = 0;


    if (SSP1CON1bits.WCOL) {

        SSP1CON1bits.WCOL = 0;
    }

    if (SSP1CON2bits.ACKSTAT) {
       
    }
}


uint8_t I2C_Read(uint8_t ack) {
    uint8_t _data;

    //while (SSP1STATbits.BF || (SSP1CON2 & 0x1F)); 
    while (SSP1CON2 & 0x1F);
    SSP1CON2bits.RCEN = 1;     

    while (!SSP1STATbits.BF); 
   
    _data = SSP1BUF; 

    while (SSP1CON2bits.ACKEN);
    SSP1CON2bits.ACKDT = (ack) ? 0 : 1;
    SSP1CON2bits.ACKEN = 1;
   
    while (SSP1CON2bits.ACKEN);
    while (SSP1STATbits.BF);

    return _data;
}
 
 
 

[woody]

Get rid of the

while (SSP1CON2 & 0x1F);

And see what happens

[newtekuser]

Get rid of the

while (SSP1CON2 & 0x1F);

And see what happens

If I remove this too, I get bogus data back.

[tru]

I've worked with the PIC18F67J50 long ago and for reading of I2C eeproms, but forgot all of my findings  .  But, looking at my old code I think you are following the datasheet guide to wait for idle for in slave mode?

Code: [Select]

while (SSP1STATbits.BF || (SSP1CON2 & 0x1F));I have this for waiting idle in master mode:

Code: [Select]

while((SSPCON2 & 0x1F) || (SSPSTATbits.R_W))

[tru]

If it helps, my code for repeated start is:

Code: [Select]

unsigned char i2c_wait_for_idle(void){
    start_tmr1();
    while((SSPCON2 & 0x1F) || (SSPSTATbits.R_W)){
      if(is_tmr1_timed_out()){ return I2C_ERR_WAIT_FOR_IDLE; }
    }
    stop_tmr1();

    return I2C_ERR_NONE;
}

unsigned char i2c_repeated_start(void){
    unsigned char m_err = i2c_wait_for_idle();
    if(m_err){ return m_err; }
   
    BCLIF = 0;
    SSPCON2bits.RSEN = 1;
   
    start_tmr1();
    while(SSPCON2bits.RSEN){
      if(is_tmr1_timed_out()){ return I2C_ERR_WAIT_FOR_RSTART; }
    }
    stop_tmr1();
   
    if(BCLIF){
      return I2C_ERR_RSTART_COL;
    }
 
    return I2C_ERR_NONE;
}


[woody]

Weird. I take it the code does not hang, but now you get rubbish.

I'll take a look tomorrow at what my compiler makes of the i2c-read().

Do you have an .lst file so I can compare the outcome?

[newtekuser]

Weird. I take it the code does not hang, but now you get rubbish.

I'll take a look tomorrow at what my compiler makes of the i2c-read().

Do you have an .lst file so I can compare the outcome?

Attaching the .lst file. Thank you very much for your time btw!