I2C protocol fails between Raspberry PI and MCU

Thread Starter

zaferaltun

Joined Aug 31, 2016
65
Hello

I have Raspberry Pi 3 and a custom circuit designed and coded by me. The circuit has 18F87K22 MCU, they are talking with I2C protocol. I developed the MCU's code by looking to the datasheet in C, using XC8 compiler. It is working in Proteus simulator as well but the real word is different as you know.

The problem is sometimes I2C protocol is just disconnecting in a way as I understood. I 'm using smbus library in Python code on RP side and getting Input/Output Error (Error 5) sometimes but totally random. if I disconnect the circuit, I 'm getting that error always, so it means the connection is losing but I don't know how?

I 'm giving latency between operations but it does not matter the latency is 1ms or 1 second, as I told before, it is totally random. I updated the RP kernel to 4.4 but nothing changed. I tried decreasing baudrate down to 9600 but nothing changed. There is no difference either 100000 or 9600. I tried to catch the problem like overflow etc on circuit side but it seems there is no overflow or something like that as I tested. Also I checked if the MCU is resetting but no, it is not resetting. How can I fix this problem? Actually is the problem about RP's I2C protocol or smbus library or my own circuit? There are lots of prorability but here is where the experince talks, please help me. I 'm not using external pull ups since RP has already 1.8k internal pull up resistors, I2C connection cable is about 50cm length. I can share the codes if needed.

I 'm awaiting urgent replies please.
Best
 

Papabravo

Joined Feb 24, 2006
21,227
There are a couple of things that are unclear from your original post. Does the PIC18F87K22 have builtin hardware support for the I2C protocol or is it bit banging? [EDIT: yes it does have two MSSPs that can be used for I2C] Does the Pi 3 have I2C hardware or is it bit banging? The reason I ask is that I2C has no concept of connection or disconnection. It is not a connection oriented protocol in the sense that most network protocols use that concept. If SDA and SCL are connected and have proper voltage levels and pullups then the two ends are connected, period, full stop.

It is true that a misbehaving transmitter can force the slave device to quit responding(ACK'ing). It is also true that in any error condition the master can RESET the slave by issuing a STOP followed by a valid START. I think it much more likely that your code is at fault, rather than the hardware in the Pi 3 or the microchip part.

There are other possible recovery mechanisms for dealing with a slave device that holds SDA low preventing the master from issuing a STOP condition.
 
Last edited:

Thread Starter

zaferaltun

Joined Aug 31, 2016
65
There are a couple of things that are unclear from your original post. Does the PIC18F87K22 have builtin hardware support for the I2C protocol or is it bit banging? [EDIT: yes it does have two MSSPs that can be used for I2C] Does the Pi 3 have I2C hardware or is it bit banging? The reason I ask is that I2C has no concept of connection or disconnection. It is not a connection oriented protocol in the sense that most network protocols use that concept. If SDA and SCL are connected and have proper voltage levels and pullups then the two ends are connected, period, full stop.

It is true that a misbehaving transmitter can force the slave device to quit responding(ACK'ing). It is also true that in any error condition the master can RESET the slave by issuing a STOP followed by a valid START. I think it much more likely that your code is at fault, rather than the hardware in the Pi 3 or the microchip part.

There are other possible recovery mechanisms for dealing with a slave device that holds SDA low preventing the master from issuing a STOP condition.

Well, I 'm in doubt about my code since I 'm developing something with I2c protocol for the first time, but I read somethings in RP forums about this error although there is no coded MCU, just a EEPROM or some sensors. Yet, here is my code:

Code:
#define _XTAL_FREQ              16000000

#define ADC_RANGE               4095                    // 2 ^ 12 (4096) bit resolution - 1
#define ADC_VREF                5000                    // 5V
#define I2C_DATA_BUFFER_LEN     10
#define I2C_CMD_BUFFER_LEN      10

#include <xc.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#pragma config FOSC             = INTIO2    // Oscillator (Internal RC oscillator)
#pragma config PLLCFG           = OFF       // PLL x4 Enable bit (Disabled)
#pragma config FCMEN            = OFF       // Fail-Safe Clock Monitor (Disabled)
#pragma config IESO             = OFF       // Internal External Oscillator Switch Over Mode (Disabled)
#pragma config WDTEN            = OFF       // Watchdog Timer (WDT disabled in hardware; SWDTEN bit disabled)
#pragma config XINST            = OFF       // Extended Instruction Set (Disabled) VERY IMPORTANT FOR UNEXPECTED BEHAVIORS
#pragma config SOSCSEL          = DIG       // SOSC Power Selection and mode Configuration bits (Digital (SCLKI) mode)
#pragma config RETEN            = OFF       // VREG Sleep Enable bit (Disabled - Controlled by SRETEN bit)

int SLAVE_ADDR                  = 0x76;     // TEMP
i2cInProgress             = 0;

void init(void);
void delayUS(int us);
void delayMS(int ms);

void main(void) {

    init();
  
    delayMS(500);
  
    while (1) {
      
        if (!i2cInProgress && SSP1STATbits.S) {
            i2cInProgress = 1;
          
            // Enable automatic baud rate calculation
            BAUDCON1bits.ABDEN = 1;
        }
      
        if (SLAVE_ADDR && i2cInProgress && PIR1bits.SSP1IF){
          
            // Receive overflow. A byte is received while the SSPxBUF register is still holding the previous byte (must be cleared insoftware)
            if (SSP1CON1bits.SSPOV){
              
                // Clear
                unsigned char prevByte = SSP1BUF;
                unsigned int bf = SSP1STATbits.BF;  // clear
                SSP1CON1bits.SSPOV = 0;
              
                // Release the clock if strecting enabled
                if (SSP1CON2bits.SEN) SSP1CON1bits.CKP = 1;
              
                // Clear interrup bit
                PIR1bits.SSP1IF = 0;
              
            } else if (BAUDCON1bits.ABDOVF){
                BAUDCON1bits.ABDOVF = 0;    // clear
            } else {
              
                if (!SSP1STATbits.RW){                  // Master is writing sometihng
              
                    if (!SSP1STATbits.DA){              // Last byte was the address

                        // Get address
                        unsigned char addr = SSP1BUF;

                        // Some synchronous codes here...

                        // Release the clock if strecting enabled
                        if (SSP1CON2bits.SEN) SSP1CON1bits.CKP = 1;
                      
                        // Clear interrup bit
                        PIR1bits.SSP1IF = 0;
                      
                    } else if (i2cAddressMatched) {     // Otherwise data

                        // Get data
                        unsigned char data = SSP1BUF;
                      
                        // Some synchronous codes here...

                        // Release the clock if strecting enabled
                        if (SSP1CON2bits.SEN) SSP1CON1bits.CKP = 1;
                      
                        // Clear interrup bit
                        PIR1bits.SSP1IF = 0;

                    }

                } else if (SSP1STATbits.RW) {           // Master is trying to reading
                  
                    // Clear BF by reading
                    unsigned int bf = SSP1STATbits.BF;
                  
                    // Process if any command suitable got before reading
                    if (i2cCmd[0] && !i2cCmdProcessed){
                      
                        // Get address
                        unsigned char addr = SSP1BUF;

            // Some synchronous codes here...

                        // Transmit data byte by byte from starting MSB
                        for (int i = 0; i < I2C_DATA_BUFFER_LEN; i++){
                            while(!PIR1bits.SSP1IF);
                            // Clear BF by reading
                            unsigned int bf = SSP1STATbits.BF;
                            SSP1BUF = i2cData[i];
                          
                            // Release the clock if strecting enabled
                            if (SSP1CON2bits.SEN) SSP1CON1bits.CKP = 1;
                          
                            PIR1bits.SSP1IF = 0;

                       // Some synchronous codes here...
                        }
                      
                        i2cCmdProcessed = 1;
                      
                        SSP1BUF = '\0'; // EOF transmit
                      
                        // Release the clock if strecting enabled
                        if (SSP1CON2bits.SEN) SSP1CON1bits.CKP = 1;

                        PIR1bits.SSP1IF = 0;

                    } else {
                        SSP1BUF = '\0';
                    }
                  
                    // delayMS(1); causing write error after reading
                  
                    // Release the clock if strecting enabled
                    if (SSP1CON2bits.SEN) SSP1CON1bits.CKP = 1;
                  
                    // Clear interrup bit
                    PIR1bits.SSP1IF = 0;

                }
              
            }
          
        }
      
        if (!SSP1STATbits.S && SSP1STATbits.P){
            i2cInProgress = 0;
        }
      
    }

    return;
}

void init() {
  
    // Ultra-low power on sleep
    WDTCONbits.SRETEN = 0;
  
    // OSC
    OSCCONbits.IRCF0 = 1;
    OSCCONbits.IRCF1 = 1;
    OSCCONbits.IRCF2 = 1;
    OSCCONbits.OSTS = 1;
    OSCCONbits.HFIOFS = 1;
    OSCCONbits.SCS0 = 0;
    OSCCONbits.SCS1 = 0;
  
    // Disable comparators
    CM1CONbits.CON = 0;
    CM2CONbits.CON = 0;
    CM3CONbits.CON = 0;
  
    // ANALOG INPUTS
  
    // Enable all analog inputs
    ANCON0 = 0xFF;
    ANCON1 = 0xFF;
    ANCON2 = 0xFF;
  
    // Set conversion as left justified and vref as AVREF+ and AVSS
    ADCON1bits.VCFG = 0b00;
    ADCON1bits.VNCFG = 0b0;
    ADCON2bits.ADFM = 0;
  
    // Set acquisition time for minimum requirements (14 TAD, ~16 TOSC)
    ADCON2bits.ADCS = 0b101;
    ADCON2bits.ACQT = 0b010;
  
    // I2C CONFIGURATION
  
    // Set as slave (SSPM(3:0) = 0110) as interrupt enabled
    SSP1CON1 = 0b00101110;
  
    // Set stretching enable
    SSP1CON2bits.SEN = 1;
  
    // Configure Input Levels and slew rate as I2C Standard Levels (Slew Rate control (SMP) set for 100kHz)
    SSP1STAT = 0b10000000;
  
    // Set i2c address to SLAVE_ADDR variable. Note that last bit of SSPADD is not used in 7 bit addressing mode
    SSP1ADD = SLAVE_ADDR << 1;
  
    // PORTS
  
    // Set address switch input port
    TRISB = 0b11111111;
    PORTB = 0;
    LATB = 0;
  
    // Set analog inputs ports. Note that sharp numbers shows input indices as starting from 1
    TRISAbits.TRISA1 = 1;   // #1
    PORTA = 0;
    LATA = 0;
  
    TRISF = 0b11111111;     // #2, #7, #8, #9, #10, #11, #12
    PORTF = 0;
    LATF = 0;
  
    TRISG = 0b11111111;     // #13, #14, #15, #16
    PORTG = 0;
    LATG = 0;
  
    TRISH = 0b11111111;     // #3, #4, #5, #6, #17, #18, #19, #20
    PORTH = 0;
    LATH = 0;
  
    // Set digital output ports
    TRISAbits.TRISA4 = 0;   // #7
    TRISAbits.TRISA5 = 0;   // #8
    PORTA = 0;
    LATA = 0;
  
    TRISCbits.TRISC1 = 0;   // #6
    TRISCbits.TRISC2 = 0;   // #4
    TRISCbits.TRISC7 = 0;   // #5
    PORTC = 0;
    LATC = 0;
  
    TRISDbits.TRISD4 = 0;   // #9
    TRISDbits.TRISD5 = 0;   // #10
    TRISDbits.TRISD6 = 0;   // #11
    TRISDbits.TRISD7 = 0;   // #12
    PORTD = 0;
    LATD = 0;
  
    TRISEbits.TRISE4 = 0;   // #20
    TRISEbits.TRISE5 = 0;   // #19
    TRISEbits.TRISE6 = 0;   // #18
    TRISEbits.TRISE7 = 0;   // #17
    PORTE = 0;
    LATE = 0;
  
    TRISJ = 0b00000000;     // #1, #2, #3, #13, #14, #15, #16 and activity led pin
    PORTJ = 0;
    LATJ = 0;
}

void delayUS(int us){
    while(us-- >= 0){
        __delay_us(1);
    }
}

void delayMS(int ms){
    while(ms-- >= 0){
        __delay_ms(1);
    }
}
 
Last edited:

Papabravo

Joined Feb 24, 2006
21,227
Three observations:
  1. You're using the && operator. You do realize this is probably the wrong thing to do with operands that are single bits -- don't you? Since the operands to && are .TRUE. and .FALSE. you don't know how the compiler will treat numerical quantities and bit quantities as it evaluates these expressions. Conside using the single & operator which does a bitwise .AND. operation. I'd have to look at the compiled code to find the problems.
  2. You're not using interrupts, and I think you should.
  3. The 500 msec delay is suspicious. Plenty of time for the RPi to muck things up while nobody is paying attention. What is that for? Is there some reason you can't be ready to roll when initialization is complete?
What you want to do is get through your initialization as fast as possible and be ready to respond to any incoming data.
 
Last edited:

Thread Starter

zaferaltun

Joined Aug 31, 2016
65
Two observations:
  1. You're not using interrupts, and I think you should.
  2. The 500 msec delay is suspicious. Plenty of time for the RPi to muck things up while nobody is paying attention. What is that for? Is there some reason you can't be ready to roll when initialization is complete?

Hi again, the answers below, thanks for replying:

1. I 'm thinking that I 'm already using interrupts, please see the init function in my code, there is a line like this:

// Set as slave (SSPM(3:0) = 0110) as interrupt enable
SSP1CON1 = 0b00101110;

Is not it enough? Or shoudl I set another interrupt bit? I 'm not sure, I 'm not expert about it.

2. The 500 msec delay is just a one-time delay as inital, it is outside of while loop as you can see, so I dont think it affects the protocol.
 

Papabravo

Joined Feb 24, 2006
21,227
You are not using interrupts since you have not created an interrupt service routine. You have not configured the MSSP interrupt to be enabled and you have not set the global interrupt enable flag. Check bits PIR1<3> and PIR3<7> depending on which MSSP you are using.

The 500 msec delay serves no useful function. See my other comment about the use of && to do bit comparisons.

New question. Why are you enabling automatic baudrate detection for one of the asynchronous serial ports?

You need to rewrite your code to avoid the && operators -- they being used incorrectly.

I see now that you are polling the interrupt flag. Not sure that is a good idea.
 
Last edited:

geekoftheweek

Joined Oct 6, 2013
1,219
I'll agree with the use of && and &... it's bitten me before.

I've never used interrupts with I2C in my projects. While it serves a purpose generaly you won't need them and it tends to make programming a bit more difficult. SSP1CON1 = 0b00100110 would work with the way your code is heading. You really don't need start and stop interrupts if you're doing basic I2C communication as it will only interrupt if the address matches and then after every byte until the stop condition. PIR1bits.SSP1IF will be set regardless of if you have interrupts enabled or not.

I would suggest adding a way to detect if the I2C fails and keeps repeating the 'while(!PIR1bits.SSP1IF);' line. Normally the problems I have are because for some reason the next byte didn't get sent or read and it just keeps looping and waiting.
If you decide not to use start and stop interrupts (I recommend if possible) you will need to add a test for the stop condition also in your while section otherwise it won't get detected.

Reading the SSPSTAT doesn't clear the BF flag. BF gets set and cleared by the PIC itself when it's time. You can eliminate those lines.

To save some instruction cycles you should eliminate the if (SSP1CON2bits.SEN) and just use SSP1CON1bits.CKP = 1. The SEN bit does not change unless you change it. Since you set it during initialization it will be set until you change it.

You could also eliminate the test for overflow to simplify things until it works properly. In theory as long as you always handle the I2C when needed you won't have an overflow condition. Actually since you are using clock stretching it won't send or receive until you release the clock anyways so as long as you read SSPBUF everytime (when the address is received and after every byte received) you won't have an overflow.

As Papa said the automatic baud rate has nothing to do with I2C.

If you don't have it already I would suggest to blink and led or something outside of the I2C code just to make sure the PIC is still doing something. Normally when I2C fails the program will get stuck waiting for the interrupt flag to be set.

I've found that it seems certain PICs I've messed with are particular about the order in which the clock is released, the interrupt flag is cleared, and data is written or read in SSPBUF. It seems like everything is in order, but check out the waveforms in the datasheet to confirm.

When it fails check voltages on SCL and SDA. If one of them are low disconnect your slave device and see if it changes. That will give you an idea if your master or slave is the one with the issue.
 
Last edited:

geekoftheweek

Joined Oct 6, 2013
1,219
Just an idea:

Code:
// I2C initialization
     SSP1CON1 = 0b00000110;  // slave mode 7 bit address
    SSP1CON2bits.SEN = 1;  // enable clock stretching
    SSP1STAT = 0b10000000; 
    SSP1ADD = SLAVE_ADDR << 1;
    SSP1CON1bits.SSPEN =1; // save enable of module until everything set up


// I2C slave polling
  if (!PIR1bits.SSP1IF) return; // no address match has occured

  // Get address
  unsigned char addr = SSP1BUF;

  if (SSP1STATbits.RW) { // master is reading
      while (!SSP1STATbits.P) {
          // move data to SSP1BUF
          SSP1BUF = data;
          // Release the clock
          SSP1CON1bits.CKP = 1;
          // Clear interrup bit
         PIR1bits.SSP1IF = 0;
  
         while (!PIR1bits.SSP1IF) {
              // increment counter or something here to check for infinite loop due to error
         }
     } 
  }

  else { // master is writing
      while (!SSP1STATbits.P) {
          // move data to SSP1BUF
          data = SSP1BUF;
          // Release the clock
          SSP1CON1bits.CKP = 1;
          // Clear interrup bit
          PIR1bits.SSP1IF = 0;
  
          while (!PIR1bits.SSP1IF) {
              // increment counter or something here to check for infinite loop due to error
          }
      }
  }
  return;
}
Kind of a basic idea of what needs to happen at minimum.
 

Papabravo

Joined Feb 24, 2006
21,227
I'll agree with the use of && and &... it's bitten me before.

I've never used interrupts with I2C in my projects. While it serves a purpose generaly you won't need them and it tends to make programming a bit more difficult. SSP1CON1 = 0b00100110 would work with the way your code is heading. You really don't need start and stop interrupts if you're doing basic I2C communication as it will only interrupt if the address matches and then after every byte until the stop condition. PIR1bits.SSP1IF will be set regardless of if you have interrupts enabled or not.

I would suggest adding a way to detect if the I2C fails and keeps repeating the 'while(!PIR1bits.SSP1IF);' line. Normally the problems I have are because for some reason the next byte didn't get sent or read and it just keeps looping and waiting.
If you decide not to use start and stop interrupts (I recommend if possible) you will need to add a test for the stop condition also in your while section otherwise it won't get detected.

Reading the SSPSTAT doesn't clear the BF flag. BF gets set and cleared by the PIC itself when it's time. You can eliminate those lines.

To save some instruction cycles you should eliminate the if (SSP1CON2bits.SEN) and just use SSP1CON1bits.CKP = 1. The SEN bit does not change unless you change it. Since you set it during initialization it will be set until you change it.

You could also eliminate the test for overflow to simplify things until it works properly. In theory as long as you always handle the I2C when needed you won't have an overflow condition. Actually since you are using clock stretching it won't send or receive until you release the clock anyways so as long as you read SSPBUF everytime (when the address is received and after every byte received) you won't have an overflow.

As Papa said the automatic baud rate has nothing to do with I2C.

If you don't have it already I would suggest to blink and led or something outside of the I2C code just to make sure the PIC is still doing something. Normally when I2C fails the program will get stuck waiting for the interrupt flag to be set.

I've found that it seems certain PICs I've messed with are particular about the order in which the clock is released, the interrupt flag is cleared, and data is written or read in SSPBUF. It seems like everything is in order, but check out the waveforms in the datasheet to confirm.

When it fails check voltages on SCL and SDA. If one of them are low disconnect your slave device and see if it changes. That will give you an idea if your master or slave is the one with the issue.
Thanks for the contribution; you clearly have more experience with this specific part than I do. Most of my previous projects were an I2C master talking to a peripheral device, and interrupts were of limited usefulness. Same for an SPI Master, and a UART transmitter. When doing an I2C slave device it seems like responding to incoming data in an interrupt service routine is the thing to do -- IMHO.
 

Thread Starter

zaferaltun

Joined Aug 31, 2016
65
I really thank both of you, especially thank you @geekoftheweek for very long explanation and sample code. I will check and try all thing both you said. I 'm suprised about && AND operator, interesting. Sometimes I 'm getting in doubt about the compiler strange behaviours, indeed.

I will write here the results after I check the things you said. Thanks again.

Best
 

geekoftheweek

Joined Oct 6, 2013
1,219
Best[/QUOTE]
Thanks for the contribution; you clearly have more experience with this specific part than I do. Most of my previous projects were an I2C master talking to a peripheral device, and interrupts were of limited usefulness. Same for an SPI Master, and a UART transmitter. When doing an I2C slave device it seems like responding to incoming data in an interrupt service routine is the thing to do -- IMHO.
You're probably right in ways about using an interrupt service routine, but for what I've done so far it has always worked fine without it. If your slave device has a really long program loop it will hold up the master until the slave gets around to checking if something happened... an interrupt driven routine would probably be better in that instance. I program in asm so it gets ugly quickly trying to keep track of what happened last and what needs to happen next. It is possible to do and I do have a LCD controller program that uses interrupts for I2C.

I really thank both of you, especially thank you @geekoftheweek for very long explanation and sample code. I will check and try all thing both you said. I 'm suprised about && AND operator, interesting. Sometimes I 'm getting in doubt about the compiler strange behaviours, indeed.

I will write here the results after I check the things you said. Thanks again.

Best
Thanks for the kind words. I went back and looked at what I put on here last night and realized I forgot something that will cause problems. You will also want to add test for a stop condition while waiting for an interrupt and add a goto or something to break the program out of the loop. I normally use asm so it didn't hit me until now.

while ((!PIR1bits.SSP1IF) && (!SSP1STATbits.P)) would probably do the trick.
 
Last edited:

Thread Starter

zaferaltun

Joined Aug 31, 2016
65
I tried all things both you said and now it is working, most of them like fine tuning. && AND operator is required, it did not work after I changed all && to &, it was surprising to me already, now I did not surprised. Simply, only disabling I2C interrupt setting is enough, you are right @geekoftheweek, it is working fine without interrupt already. The solution is hidden on this line:

SSP1CON1 = 0b00100110;

Thank you very much.
 
Top