Help with MCC generated I2CSIMPLE getting stuck in a loop

[burkart]

Hello All,

I am working on a project (in MPLAB v5.10) with a PIC18F27K40 (PIC18 library v1.77.0) and I'm using the MCC (v3.85.1) generated I2C drivers, I2CSIMPLE from the Foundation Services. I can read from and write successfully to devices on the I2C bus. The problem comes when I try to communicate with a device that's not on the bus, the micro goes into an endless loop waiting for i2c_status to not be busy. My knowledge of programming in C is about 6 on a scale of 10, and for programming for embedded purposes, about 5 out of 10. I would like to have it so that I can check if a specific device is present on the I2C bus, and also be able to recover from errors on the bus without it going into a loop indefinitely.

This I2C driver is pretty complex, and I am having difficulties wrapping my head around it. How would I make it so that the driver just returns an error or something I can check for status, rather than loop endlessly until the operation completes, which it never will?

I have not edited any of the MCC generated code. This includes leaving the IRQ enable line commented in the i2c_master.c file, so instead it polls instead of using an interrupt to check if the i2c operation has completed.

        // uncomment the IRQ enable for an interrupt driven driver.
        // mssp1_enableIRQ();

Following is an example of how I am calling the i2c driver.

	i2c_write1ByteRegister(address, 0x0D, 0x07);    // GPPUB    pull-ups on pins 0-2

I am attempting to initialize a port extender, MCP23018, specifically enabling some pull-up resistors. I would like to issue this command, and if the extender is not present, then the micro will perform some tasks differently. With the port extender present the write operation works as expected and everything is fine. Of course the problem is when the extender is NOT on the bus to acknowledge.

I have another question as well. This driver seems to operate a little slow. When watching the bus with a logic analyzer I noticed a rather long pause between bytes. I went looking through the i2c driver and in i2c1_driver.c I found the following code which I suspect is the cause.

inline void mssp1_waitForEvent(uint16_t *timeout)
{
//    uint16_t to = (timeout!=NULL)?*timeout:100;
//    to <<= 8;

    if(PIR3bits.SSP1IF == 0)
    {
        while(1)// to--)
        {
            if(PIR3bits.SSP1IF) break;
            __delay_us(100);
        }
    }
}

What is the purpose of the 100 us delay in the while loop? Reducing or eliminating the delay results in reducing or removing the pause between byte transactions, but I don't know enough to know how else this edit will effect the driver. Also, what is the commented out code at the top of the function used for? Is this part of the infinite loop problem I mentioned above?

 

--
James Burkart

[Orunmila]

That delay is central to solving your problem. It is really part of some unfinished timeout code at the top of that function which you thankfully included into your question!

The idea was that you can use the concept shown in the comments to produce a timeout in the driver. If you do not expect any timeouts you can safely remove the delay, it simply makes the timeout be a multiple of the delay number, so if we delay 100us, and we set timeout to 100 (the default) then we will get a 10ms timeout.

What I would suggest is that you complete the timeout implementation as follows:

First you need to set the timeout variable to a fixed value, and then we will reduce the unit delay to 1us (you could remove it altogether and the timeout will be 50,000 loop cycles. If this is too short you can always increase the variable to 32bit and increase the count or add a couple of NOP's to the function.

It should look like this when you are done:

inline void mssp1_waitForEvent(uint16_t *timeout)
{
    uint16_t to = 50000; // This will cause a 50ms timeout on I2C

    if(PIR3bits.SSP1IF == 0)
    {
        while(to--) 
        {
            if(PIR3bits.SSP1IF) break;
            __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms
        }
    }
}

MPLAB-X has died on me today, I am re-installing and will check out if this is sufficient to fix your problem as soon as I have it back up and running, in the meantime you could try the above.

 

[Orunmila]

Ok, I am back up and running and I see that with that you will end up stuck in the next loop. You can force it to break out of the loop by simulating an error. The correct error for your case, and correct behavior in most cases when you get a timeout, would be to perform the AddressNAK behavior. You can trigger that by doing this:

inline void mssp1_waitForEvent(uint16_t *timeout)
{
    uint16_t to = 50000; // This will cause a 50ms timeout on I2C

    if(PIR3bits.SSP1IF == 0)
    {
        while(to--) 
        {
            if(PIR3bits.SSP1IF) break;
            __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms
        }
      
        if (to == 0)
        {
          i2c_status.state = I2C_ADDRESS_NACK; // State Override for timeout case
        }  
    }
}

 

If you want different behavior in the case of a timeout than you have for an address NAK you can always add an event to the state table called stateHandlerFunction, and at this location set the state to the new entry, and then copy the do_I2C_DO_ADDRESS_NACK() function and change the behavior in that to what you want to do different for a timeout.  You may e.g. set the error to I2C_TIMEOUT which you could add to i2c_error_t and you probably do not want to do the do_I2C_SEND_RESTART.

All of that is of course a more substantial change, but I could walk you through this if you want.

[burkart]

So this is where my limited grasp of C stops me. Using that last bit of code you posted, I see what you are doing, but I get an error upon compiling.

mcc_generated_files/i2c1_driver.c:100:1: error: (192) undefined identifier "i2c_status"
mcc_generated_files/i2c1_driver.c:100:18: error: (196) struct/union required
mcc_generated_files/i2c1_driver.c:100:20: error: (192) undefined identifier "I2C_ADDRESS_NACK"

I'm gonna crack my C textbook open and see if I can't figure this out.

[N9WXU]

It looks like you switched microcontrollers.  I noticed the generated code was for an MSSP and now it is for an I2C peripheral.  Which MCU are you using with the errors above?

[Orunmila]

It is just the other layer, it has mssp at the bottom, i2c_driver and master and simple on top of that.

I just did not pay close enough attention there, I did not check if it would compile, let me check it out tomorrow and I can help you to get that to build.

[Orunmila]

Ok this is slightly more complex because the status is one layer up so you have to pass the fact that there was a timeout up one layer, but that function does not return anything, so you have to change that.

3 easy steps.

Start in i2c_driver.h and change the prototype of the function like this:

INLINE int mssp1_waitForEvent(uint16_t*);

Change the mssp code to this:

inline int mssp1_waitForEvent(uint16_t *timeout)
{
   uint16_t to = 50000; // This will cause a 50ms timeout on I2C

    if(PIR3bits.SSP1IF == 0)
    {
        while(to--) 
        {
            if(PIR3bits.SSP1IF) break;
            __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms
        }
      
        if (to == 0)
        {
            return -1;
        }  
    }
   
   return 0;
}

And then lastly catch the error in i2c_master.c

inline void i2c_poller(void)
{
    while(i2c_status.busy)
    {
        if (mssp1_waitForEvent(NULL) == -1)
        {
            i2c_status.state = I2C_ADDRESS_NACK; // State Override for timeout case
        }
        i2c_ISR();
    }
}

So what this does is pass up -1 when there is a timeout, which will then advance the state machine (which happens in i2c_ISR() ) based on the status.

Now as I said I just used the Address NAK behavior there, when you have no slave connected you should see an address NAK (I should actually check why you are not getting one), but in this case we are saying a timeout requires the same behavior which should work ok.

If it does not we may want to add a state for timeout as I described before.

But let's try the timeout using the I2C_ADDRESS_NACK method first for your board. If this does not work I will crack out a board and run it on the hardware to see exactly what is happening.

 

 

 

[burkart]

So I made the code changes. When there is nothing on the I2C bus, the driver still goes into an endless loop. When I debug I notice that it never gets to time out, or rather the variable "to" never gets to count down from 50000. I am guessing PIR3bits.SSP1IF interrupt flag gets set and it breaks out of the mssp1_waitForEvent function. But if there is nothing on the bus why is this flag setting? Is this the correct interrupt to be monitoring?

I don't know enough to know if I'm asking the right questions, but I am thinking that if the Master device on the I2C bus sends out an address and does not receive an ACK from the slave, shouldn't a time-out routine be looking for that slave's acknowledgement? Do I have that right?

[Orunmila]

Yes that is what I meant when I said "I should actually check why you are not getting one". 

I would expect that you would send out the address and receive back a NAK when there is nothing connected to the bus. It would be important that you do have the pull-up resistors on the bus though. 

Looks like I am going to have to crack out that hardware and look at the signals on my Saleae to see what exactly is going on. I left my Saleae at the office so I can only do that tomorrow. 

Stand by and I will let you know what I find.

[burkart]

I'm using a PICKit 4 for programming and debugging. I am using 2k ohm pull-ups. The transitions look nice and crisp, no ramp-ups when the bus is released. For debugging I have the I2C speed set to 100kHz, but everything still works at 1MHz. As you can see in the image below, the slave is sending ACK when everything is peachy.

So the question now is how is the code reacting to a NACK. What I've noticed is, after the first byte is transmitted and there is nothing there to respond back to the master, the master then sends the next byte, and all subsequent bytes endlessly, interestingly the value of these bytes is the value of the first byte, plus one, as if the master is requesting a read operation.

I'll debug and see if I can further understand the behavior and operation of the I2C driver, specifically what it's doing to monitor ACK and NACK.

[Orunmila]

So there seems to be a couple of bugs here, firstly that it is not realizing that the address was a NAK, that is pretty standard and should work correctly, but after that it switches the command from write to read, which is also not correct. I would expect it to retry, but it does not make sense to retry using read if the initial attempt was to write to the device...

I don't know how soon I will have time to debug this so here are some tips for you on how the driver works.

The state machine and all decisions happen in i2c_masterOperation which is in i2c_master.c. In that function it will check if you are using the interrupt driven driver, in which case it will just return and let the state change happen in the IRQ, else it will run the poller, which will poll for the change in state and run the interrupt if the state change has happened.

Essentially the poller is just polling the interrupt flag SSP1IF and calling the ISR when this is set.

So there are 2 possible ways this could be going wrong. Case 1 is if the IF is never set, you can check this in the debugger but from what you describe it is not timing out which means that the IF seems to be set when the NAK happens.

The other option is that NAK check is disabled or not successfully checked in the ISR. You should check here :

void i2c_ISR(void)
{       
    mssp1_clearIRQ();
    
    // NOTE: We are ignoring the Write Collision flag.
    // the write collision is when SSPBUF is written prematurely (2x in a row without sending)

    // NACK After address override Exception handler
    if(i2c_status.addressNACKCheck && i2c1_driver_isNACK())
    {
        i2c_status.state = I2C_ADDRESS_NACK; // State Override
    }

    i2c_status.state = fsmStateTable[i2c_status.state]();
}

That if check there should be true.

From what I can see, if you are not seeing a timeout, you must be seeing this ISR function being called from the poller, and this must set the state to I2C_ADDRESS_NACK, if this does not happen we can investigate if it is because the check is disabled or the ACKSTAT register has the wrong value.

If it goes in there and the state is set the next place to look is where the NAK is processed, which should be here:

// TODO: probably need 2 addressNACK's one from read and one from write.
//       the do NACK before RESTART or STOP is a special case that a new state simplifies.
static i2c_fsm_states_t do_I2C_DO_ADDRESS_NACK(void)
{
    i2c_status.addressNACKCheck = 0;
    i2c_status.error = I2C_FAIL;
    switch(i2c_status.callbackTable[i2c_addressNACK](i2c_status.callbackPayload[i2c_addressNACK]))
    {
        case i2c_restart_read:
        case i2c_restart_write:
            return do_I2C_SEND_RESTART();
        default:
            return do_I2C_SEND_STOP();
    }
}

 

[Orunmila]

Ok, it is never that easy right :)

Couple of small oversights up to now.

1. The do_I2C_SEND_STOP did not clear the busy flag
2. My code I posted above has a bug, doing a post-decrement and checkign for 0 later, which does not work.
3. NAK polling is enabled without any timeout, we need to disable this.

After fixing these 3 things it should work as expected.

This is how:

1. There is a bug in the driver at do_I2C_SEND_STOP, change that to look like this. It is setting the state to idle here but missing the clearing of the busy flag.

static i2c_fsm_states_t do_I2C_SEND_STOP(void)
{
    i2c1_driver_stop();
    i2c_status.busy = 0;
    return I2C_IDLE;
}

2. There is a bug in the code I posted above, I was doing to--, and after that checking for 0, but the post-decrement will reduce to to -1 by the time I was checking, so that should be like the following, main difference being  :

inline int mssp1_waitForEvent(uint16_t *timeout)
{
   uint16_t to = 50000; // This will cause a 50ms timeout on I2C

    if(PIR3bits.SSP1IF == 0)
    {
        while(--to) 
        {
            if(PIR3bits.SSP1IF) break;
            __delay_us(1); // We reduce this to 1us, the while loop will add some delay so the timeout will be at least 50ms
        }
      
        if (to == 0)
        {
            return -1;
        }  
    }
   
   return 0;
}

3. At this point the driver will operate in the default mode which is NAK Polling, this means that every time we get a NAK the driver will retry, assuming that the device we are talking to is busy, and we need to retry when a NAK happens. This is activated by setting the addressNAK callback to do a restart-write. We do not want this for your case, we want to fail and return when there is no answer. We can do this by commenting out the line in i2c_simple_master which enables this as follows:

void i2c_write1ByteRegister(i2c_address_t address, uint8_t reg, uint8_t data)
{
    while(!i2c_open(address)); // sit here until we get the bus..
    i2c_setDataCompleteCallback(wr1RegCompleteHandler,&data);
    i2c_setBuffer(&reg,1);
    //i2c_setAddressNACKCallback(i2c_restartWrite,NULL); //NACK polling?
    i2c_masterWrite();
    while(I2C_BUSY == i2c_close()); // sit here until finished.
}

Once you have done these three things the driver should do what you need, it will go back to idle after an address NAK with i2c_status.error set to I2C_FAIL.

 

 

[burkart]

Success!!!

So the code no longer gets hung up on a NACK. Woot!

Now I'm gonna try to figure out how to have the code retry a set number of times before it gives up and moves on.

[Orunmila]

The driver is more like a framework for you to inject your own functions to change the behavior. The framework is pretty solid but the injected stuff is not complete (as you have discovered).

The way to do that is to add a counter to the code we just did. When you start a transaction set it to X and count down on every retry. If you enable the NAK polling callback that will cause a retry, so you just have to modify that address nak function to retry only a limited number of times and that would be it 🙂