Another I2C problem - code works in polled mode, doesn't work from ISR

[FridgeFreezer]

This has been bugging me all day and I can't for the life of me see what's going wrong... MCU is MCF52259, CodeWarrior 7.2. I'm reading data from an MCP23017 I/O expander.

If I use I2C0, initialise it with Freescale's own code and then run their polled I2C routine it works fine:

/* * I2CreceiveByte: I2C read byte * * Parameters: address: address to read *      id: I2C device to read * * Return : data: byte read it from device */uint8 FSI2CxreceiveByte(uint8 address, uint8 id){ uint8 data;  /* setting in Tx mode */ MCF_I2C0_I2CR |= MCF_I2C_I2CR_MTX;     /* send start condition */ MCF_I2C0_I2CR |= MCF_I2C_I2CR_MSTA;  /* devide ID to write */ MCF_I2C0_I2DR = id;         /* wait until one byte transfer completion */ while( !(MCF_I2C0_I2SR & MCF_I2C_I2SR_IIF )) ; /* clear the completion transfer flag */ MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;  /* memory address */ MCF_I2C0_I2DR = address;        /* wait until one byte transfer completion */ while( !(MCF_I2C0_I2SR & MCF_I2C_I2SR_IIF )) ; /* clear the completion transfer flag */ MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;    /* resend start */  MCF_I2C0_I2CR |= MCF_I2C_I2CR_RSTA;    /* device id to read */  MCF_I2C0_I2DR = (uint8)(id | 0x01);       /* wait until one byte transfer completion */ while( !(MCF_I2C0_I2SR & MCF_I2C_I2SR_IIF )) ; /* clear the completion transfer flag */ MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;  /* setting in Rx mode */ MCF_I2C0_I2CR &= ~MCF_I2C_I2CR_MTX;   /* send NO ACK */  MCF_I2C0_I2CR |= MCF_I2C_I2CR_TXAK;   /* dummy read */   data = MCF_I2C0_I2DR;        /* wait until one byte transfer completion */ while( !(MCF_I2C0_I2SR & MCF_I2C_I2SR_IIF )) ; /* clear the completion transfer flag */ MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;  /* read data received */ data = MCF_I2C0_I2DR;        /* wait until one byte transfer completion */ while( !(MCF_I2C0_I2SR & MCF_I2C_I2SR_IIF )) ; /* clear the completion transfer flag */ MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;  /* generates stop condition */ MCF_I2C0_I2CR &= ~MCF_I2C_I2CR_MSTA;  /* send the received data */ return data;}

But if I enable interrupt 17 (I2C0) and then use the following routine, it doesn't work and the "data" read back is actually the "address" I'm supposed to be reading (EG I'm supposed to send chip ID 0x4E, register address 0x12 and get back "0xFF" as data I instead get "0x12" back as data. Using a scope I can't see any data going back & forth on the I2C bus lines either.

Anyway, here's my ISR, it's a complete copy-and-paste of the FS routine above but with the wait loops replaced with an incrementing state-machine counter:

__interrupt void i2c_isr(void){<variables, interrupt mask, etc. here> switch(state) {  //  // Start - setup port, send start & device ID  //  case 1:      // clear the interrupt   MCF_I2C0_I2SR = 0;            /* setting in Tx mode */      MCF_I2C0_I2CR = MCF_I2C_I2CR_IEN | MCF_I2C_I2CR_IIEN | MCF_I2C_I2CR_MTX; // Enable         /* send start condition */   MCF_I2C0_I2CR |= MCF_I2C_I2CR_MSTA;      /* devide ID to write */   MCF_I2C0_I2DR = id;         // wait until one byte transfer completion   break;    //  // Started, device ID sent, now send register address  //  case 2:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;      /* memory address */   MCF_I2C0_I2DR = address;    // wait until one byte transfer completion    break;    //  // Register address sent, restart in read mode  //  case 3:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;        /* resend start */   MCF_I2C0_I2CR |= MCF_I2C_I2CR_RSTA;          /* device id to read */    MCF_I2C0_I2DR = (uint8)(id | 0x01);      // wait until one byte transfer completion    break;    //  // Send clock pulses to read data  //  case 4:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;      /* setting in Rx mode */   MCF_I2C0_I2CR &= ~MCF_I2C_I2CR_MTX;       /* send NO ACK */    MCF_I2C0_I2CR |= MCF_I2C_I2CR_TXAK;       /* dummy read */     temp = MCF_I2C0_I2DR;   break;      //  // Read data  //  case 5:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;      /* read data received */   data = MCF_I2C0_I2DR;    // This will trigger another read   // ...so wait until one byte transfer completion    break;    //  // Finish  //  case 6:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;       /* generates stop condition */   MCF_I2C0_I2CR &= ~MCF_I2C_I2CR_MSTA;    break;  //  // Default / end  //  case 0:  default:   // Reset / go round again   break; }  if(state < 7) // Stop after one run {  state++; }}

I'm sure I must be doing something wrong somewhere but I can't for the life of me see it

[TomE]

That looks good to me too.

I'd suggest you start a microsecond counter somewhere (start a DMA timer running) and then log the time (from that counter) and status register contents (at the start and end of the ISR) into an in-memory array. The timing of the interrupts and the status register should tell you what's going on. Maybe even flash a LED on and off to show where the code is and correlate that with what the CRO sees on the I2C pins. Horribly low-level debugging, but sometimes you just have to.

Tom

[FridgeFreezer]

Having been off working on other problems I've just come back to this and I may have made a little progress...

Freescale's polled routine actually fails locked-up if you call it twice in quick succession, I found that adding this to the end of it fixes the problem:

 while((MCF_I2C0_I2SR & MCF_I2C_I2SR_IBB )) ; // Wait for bus to be !busy

Curiouser and curiouser.

[TomE]

Didn't you ask this question and mark it "Solved" 17 months ago?

https://community.freescale.com/message/64722#64722

How did the code you received then work, or does it show the same problem?

Searching for "I2C" in this forum gets 120 hits. Searching for "I2SR" gets 10 including this one complaining of "infinite waits" from 4 years ago:

https://community.freescale.com/message/33399#33399

The Transmit code from that post includes this comment about "waiting for bus busy" which might match what you're finding:

// Don't bother to wait for bus busy, just wait until the ISR signals// that the I2C transaction is completeWhile (i2c.status == I2C_IN_PROGRESS)      ;

I'd suggest getting as much sample code as you can. Google finds 177 matches for "MCF_I2C_I2SR_IBB".

Tom

[FridgeFreezer]

> Didn't you ask this question and mark it "Solved" 17 months ago?

Almost - I asked for an example & I got one, but in the end we had a change of plan with the hardware and ended up using an SPI device where we were going to use I2C so I never got as far as implementing a proper I2C ISR.

Having come back to I2C for a new stage of the project I thought it would be good to implement an ISR, the uTasker example is helpful but for me it's too tied into all the uTasker framework to use directly, so I thought I'd start with the Freescale example (what could possibly go wrong?!).

Thanks for pointing out google I must say I tend to post here as I've never had a great hit-rate with code & coding advice found via google as it's often at the enthusiastic amateur end of things, whereas I'm more of a jaded & cynical amateur

So far all of the other code I've seen has been using arbitrary delay/wait loops which doesn't really gain me anything over the official Freescale version.

[TomE]

It looks pretty obvious that the I2C is running a state machine that is somewhat "sub-optimal" and isn't transitioning cleanly as far as the user status trasfers are concerned.

So it looks like it has to be "write the data, wait for SOMETHING and only after that will it interrupt properly.

Probably like an old UART - the transitions are driven by the bit-clock.

So whatever hacky "delays" have been put into different bits of code would depend on the CPU speed, the CPU execution speed (cache on or off) and the I2C baud rate.

So for reliable and reuseable code that you can have some degree of trust in, I'd suggest "you start a microsecond counter somewhere (start a DMA timer running) and then log the time (from that counter) and status register contents (at the start and end of the ISR) into an in-memory array. The timing of the interrupts and the status register should tell you what's going on"

Moreover, log all register changes through a whole manually-timed cycle, including when you see the interrupt controller post the interrupts.

That should give enough information to see what the timing should be.

it would also be worth raising a Technical Request to try and get a definitive answer, you might get lucky.

"Step Zero" is of course to trawl through all the previous queries in this forum on the subject of the I2C implementations. All this may have been sorted 5 years ago. I'd also suggest Googling with "site:www.freescale.com". You're looking for any app notes for OTHER products (older ones, when there were more App Notes written :smileyhappy:. I found this which looks interesting:

 http://www.freescale.com/files/analog/doc/app_note/AN4342.pdf

Tom

[FridgeFreezer · Accepted solution]

Well I think I've solved it - the problem with the ISR was not the same as the problem with the polled/wait-loop routine.

That the ISR worked if I put the first case (setup & Tx start & device ID) into a normal function:

void iic_trigger_interrupt(uint8 id){ // clear the interrupt MCF_I2C0_I2SR = 0; /* setting in Tx mode */ MCF_I2C0_I2CR |= MCF_I2C_I2CR_MTX;    /* send start condition */ MCF_I2C0_I2CR |= MCF_I2C_I2CR_MSTA; /* devide ID to write */ MCF_I2C0_I2DR = id;}

 This can be called conditionally when the I2C status flag is "idle", but is not as elegant as I was hoping - I like to keep all the code in the ISR if I can.

When I tried triggering the ISR by using the interrupt force register:

MCF_INTC0_INTFRCL |= MCF_INTC_INTFRCL_INTFRC17

It misbehaved. Since TFM doesn't mention clearing or resetting of the INTFRCx register I had not been clearing the INTFRCx register in the ISR, but adding a test & clear fixed it:

if(MCF_INTC0_INTFRCL & MCF_INTC_INTFRCL_INTFRC17){    MCF_INTC0_INTFRCL &= ~MCF_INTC_INTFRCL_INTFRC17;}

 Kudos to Tom's method of setting a fast timer loop to monitor states, I've been using a scope on the SDA line plus two GPIO pins to indicate status bits from the I2SR register and it really does help nail things down.

So - the answer as is so often the case is that assumption is the mother of all f***ups, with the more-common-than-it-should-be ambiguity of documentation & poor example code by Freescale. Hey ho, I'm happy to have it sorted.

For those playing along at home, this is now my very basic I2C interrupt routine in its entireity, hopefully it will help someone (if only as an example of how not to do it!):

// This could just as well be be a static var inside the ISR // for some applications... iic->state = 0; // External I2C status flag/semaphore iic->data = 0; // Where your data byte will end up__interrupt__ void i2c_handler(){ uint8 ipl = 0; uint8 port = I2C_PORT_INTERNAL; uint8 temp = 0, i = 0, address = 0, id = 0; address = REGISTER_IN_DEVICE; id = I2C_DEVICE_ID; ipl = DisableInts(); // ensure interrupts remain blocked during ISRs   // If interrupt was forced, clear it  if(MCF_INTC0_INTFRCL & MCF_INTC_INTFRCL_INTFRC17)  {   MCF_INTC0_INTFRCL &= ~MCF_INTC_INTFRCL_INTFRC17;  }          if(MCF_I2C0_I2SR & MCF_I2C_I2SR_IAL) // We lost control of the bus    {     // Arbitration lost     MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IAL; // Clear it     iic->state = 7;     /* clear the completion transfer flag */ MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;   /* generates stop condition */ MCF_I2C0_I2CR &= ~MCF_I2C_I2CR_MSTA;  /* wait for bus to go idle */ while((MCF_I2C0_I2SR & MCF_I2C_I2SR_IBB )) ;    }     // // What are we doing? // switch(iic->state) {  //  // Start - setup port, send start & device ID  //  case 1:      // clear the interrupt   MCF_I2C0_I2SR = 0;         /* setting in Tx mode */   MCF_I2C0_I2CR |= MCF_I2C_I2CR_MTX;         /* send start condition */   MCF_I2C0_I2CR |= MCF_I2C_I2CR_MSTA;      /* devide ID to write */   MCF_I2C0_I2DR = id;         // wait until one byte transfer completion   break;    //  // Started, device ID sent, now send register address  //  case 2:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;         // Check for ACK   if(MCF_I2C0_I2SR & MCF_I2C_I2SR_RXAK)   {    // Not currently used   }      /* memory address */   MCF_I2C0_I2DR = address;            // wait until one byte transfer completion    break;    //  // Register address sent, restart in read mode  //  case 3:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;           // Check for ACK   if(MCF_I2C0_I2SR & MCF_I2C_I2SR_RXAK)   {    // Not currently used   }         /* Send repeated start (was "resend start") */   MCF_I2C0_I2CR |= MCF_I2C_I2CR_RSTA;          /* device id to read */    MCF_I2C0_I2DR = (uint8)(id | 0x01);   // wait until one byte transfer completion    break;    //  // Send clock pulses to read data  //  case 4:  case 104:  case 204:   /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;      /* setting in Rx mode */   MCF_I2C0_I2CR &= ~MCF_I2C_I2CR_MTX;       /* send NO ACK */    MCF_I2C0_I2CR |= MCF_I2C_I2CR_TXAK;       /* dummy read */     temp = MCF_I2C0_I2DR;   break;      //  // Read data  //  case 5:  case 105:  case 205:    /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;      /* read data received */   iic->data = MCF_I2C0_I2DR;   // This will trigger another read   // ...so wait until one byte transfer completion    // Freescale just do it and it just works, no-one knows why...   break;    //  // Finish  //  case 6:  case 106:  case 206:     /* clear the completion transfer flag */   MCF_I2C0_I2SR &= ~MCF_I2C_I2SR_IIF;       /* generates stop condition */   MCF_I2C0_I2CR &= ~MCF_I2C_I2CR_MSTA;    break;  case 7:   /* clear the completion transfer flag */   MCF_I2C0_I2SR = 0;   break;  //  // Default / end  //  case 0:  default:   // Reset / go round again   break; }  if(iic->state < 6) // Stop after one run {  iic->state++; } else {  iic->state = 0; // Wait for next time round. }   EnableInts(ipl);}

[JimDon]

Hate to burst your bubble, but as Tom pointed out this is really not a great way to write this sort of code.

You should now go back and take a look at the layout of Marks u-Tasker interrupt driven code, as the method you used is a pretty limited way to do things.

Like it can only handle the chip you are using...

You need things like a flexible read and write buffer so you can generally send/receive n bytes. 

You are correct that Freescale really blew it on this batch of sample code....

[FridgeFreezer]

Jim - you are right, however we don't have the time/resource/scope to build 100% portable code and nor do we really have the requirement to, the task(s) at hand are standardised on the one MCU to keep life simple. If we did need that level of flexibility then we'd probably already be on uTasker.

Changing MCU would lead to a hardware redesign that would be as painful for the customer as porting my awful software we've already had the experience of doing a ground-up redesign (HW+SW) to move one product away from a horrific & expensive M.Core based board, the biggest headache software-wise is not the bits of the code that transferred across, it's the hardware changes which mean writing new driver code whatever happens.

[JimDon]

Sorry, I was not clear.

I did not mean another MCU, I meant a different I2C device that requires different command sequences, more/less read writes. 

My experience says now is the time to do it while the issues are fresh in your mind. I can' imagine it would take more that a day, as the uTasker code is really pretty much what you need as an example, you just need to tweak the read write buffers a bit.

[FridgeFreezer]

Jim - in that case, that is exactly what I'm doing as the next step. The example code was purely as a starting point for anyone trying to do the same/similar thing, I figure once you can read 1 byte from an I2C device you can expand the driver to do more.

[TomE]

I searched for "I2C" on Freescale, and found one called "MCF5282 Sample Code for I2C (zip)" that looks pretty good.

http://www.freescale.com/webapp/sps/download/license.jsp?colCode=MCF5282I2C&location=null&fsrch=1&sr...

Tom

[TomE]

I said that the MCF5282 code "looks pretty good".

I'm currently using that code as a starting point and I take it all back. It may be "pretty good" for first-cut test code, but it is light-years away from being "production ready".

It uses global buffers everywhere instead of using parameters, so it can't easily be used in a system with more than one I2C port. It sits in hard loops waiting for responses, and that causes watchdog resets when it goes wrong. There's no "error status" response from the I2C function as it doesn't check for any errors - like not getting an ACK from the chip, or getting a timeout waiting for a response from the controller. So it generates weird I2C bus cycles or locks up forever whenever anything goes wrong. Whenever I see code like the following - an delay-loop that doesn't match anything in any documents, and where the comment is very unhelpful (not saying WHY it is waiting) I know the code needs work:

    /* Wait for a bit */    i=10000;    while(i--);

The code is also inconstent in its use of defines for the register bits in some places and using "magic hex numbers" in others:

    /* Clear IBCR.IBDIS, disable interrupts */    MCF5282_I2C_I2CR = 0x80;     ...    /* Put module in master TX mode (generates START) */    MCF5282_I2C_I2CR |= (MCF5282_I2C_I2CR_MSTA | MCF5282_I2C_I2CR_MTX);    ...    /* Generate STOP */    MCF5282_I2C_I2CR &= ~MCF5282_I2C_I2CR_MSTA;    ...    /* Restore module to it's idle (but active) state */    MCF5282_I2C_I2CR = 0xC0;

The last line in the above "POLLING_MODE" code also enables I2C interrupts (0xC0 instead of using 0x80), which might cause problems.

This bit looks to be in error too:

    /* Receive data from slave */    while (i2c_rx_buffer.length > 0)         {        /* Wait for transfer to complete */        while (!(MCF5282_I2C_I2SR & MCF5282_I2C_I2SR_IIF));        MCF5282_I2C_I2SR &= ~MCF5282_I2C_I2SR_IIF;        /* Check for second-to-last and last byte transmission.  After second-to-last           byte is received, it is required to disable the ACK bit in order to signal           to the slave that the last byte has been received.  The actual NAck does not           take place until after the last byte has been received. */        if (i2c_rx_buffer.length == 2)         {            /* Disable Acknowledge, generate STOP after next byte transfer */            MCF5282_I2C_I2CR |= MCF5282_I2C_I2CR_TXAK;        }        if(i2c_rx_buffer.length == 1)         {            /* Generate STOP */            MCF5282_I2C_I2CR &= ~MCF5282_I2C_I2CR_MSTA;        }

I2C requires that when reading a stream of bytes, all bytes up to the last one should be ACKed but the last one shouldn't be. The above code will do that, but only if reading two or more bytes. It doesn't handle the case of not ACKing a single-byte read, and that is the most common case when using I2C.

Tom

[FridgeFreezer]

I take it all back

 Oh dear! On the bright side I've been too busy to have had a proper look at that code so maybe I shan't bother now!

Your comments about the many foibles of Freescale's drivers sum up my feelings too - wherever I see:

while(SOMETHING){    ; // Wait for faultless execution of thing}

I get nervous - and FS's code seems full of this sort of thing. I'd expect to see Arduinoists doing this for quick-and-dirty bit-banging drivers, but as a hardware driver routine / example from the guys who made the chip, it's rather disappointing.