 |
 |
| View previous topic :: View next topic |
| Author |
Message |
allenhuffman
Joined: 17 Jun 2019
Posts: 569
Location: Des Moines, Iowa, USA
|
| I2C ISR and clock stretching |
 Posted: Wed Oct 21, 2020 9:30 am |
 |
|
We have a PC master communicating via I2C with six PIC boards. Our existing code uses the #I2C_SISR interrupt. It receives incoming I2C bytes into a buffer, then sets a flag so code in a main loop will process the message. The code will then fill an output buffer so when the master starts reading the ISR will fetch bytes from this buffer.
This causes many timing issues if the master reads before the PIC has populated that buffer (incomplete messages, etc.). I just started exploring this code, and based on my non-ISR bootloader I wrote, I believe clock stretching will solve this issue. The Master should be able to write/read and just block until the slave is ready to respond.
Ideally, I'd use the ISR for receiving messages from the Master, and when the master switches to Read (i2c_isr_state == 0x80) I think doing:
| Code: |
// Read the address.
i2c_read (SYSTEM_BUS, TWO); |
...should enable clock stretching and the subsequent reads from the Master will block until the Slave writes.
If I recall my polled I2C code, this worked great. But, is this possible when using an ISR? The Master doing the read invokes the ISR but it keeps feeding bytes in the ISR, so no stretching needs to happen (ISR has data, even if it's not populated data).
I thought I could just do the address i2c_read() in the ISR, and skip the rest, doing the i2c_write() manually from the main loop. So far, this does not look like it is working.
Before I spend a few hours debugging and digging in to this, I thought I'd post here and see if someone can give me a "yes it should work" or "no way, you can't do that".
Thoughts appreciated.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Using: 24FJ256GA106, 24EP256GP202 and 24FJ64GA002. |
|
 |
Ttelmah
Joined: 11 Mar 2010
Posts: 19549
|
|
| Posted: Thu Oct 22, 2020 12:55 am |
|
|
You need to start by telling us 'what PIC', and what compiler version?.
Yes, assuming that 'TWO' is #defined as the number 2, what you show
should be leaving the clock held low. However the 'sequencing' of this
command is important. So post your ISR. Also the setup line for the I2C. |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 569
Location: Des Moines, Iowa, USA
|
|
| Posted: Thu Oct 22, 2020 10:09 am |
|
|
(Yeah, I hope TWO is 2. I've never worked anywhere that had folks defining constants to be constants.)
We use several PIC24 variations, all with hardware I2C. I'm just going for basic concepts -- i.e. can this even be done. I don't mind "doing the work" but if I'm trying to do something not possible, I always appreciate a heads up 
We use the latest CCS PIC compiler, whatever version that is.
The ISR appears to be based on the example from the help files. The core looks like the following:
| Code: |
#INT_SI2C2 LEVEL=7
void si2c2_isr (void)
{
int state = i2c_isr_state (SYSTEM_BUS);
if (state == RECEIVE_I2C) // 0x00
{
// Get a byte of data which is the address of the board.
boardAddress = i2c_read (SYSTEM_BUS);
}
// This detects data is ready to be sent to the master.
else if (state == TRANSMIT_I2C) // 0x80
{
// Get a byte of data which is the address of the board, and do not release clock at end of read.
i2c_read (SYSTEM_BUS, TWO);
/* this will be removed */
// Send the data in the TX buffer to the master.
i2c_write (SYSTEM_BUS, txSlaveBuffer[slaveIndexTX++]);
//i2c_write_slave (SYSTEM_BUS, txSlaveBuffer[slaveIndexTX++]);
}
// Transmits a message to the master.
else if (state > TRANSMIT_I2C)
{
/* this will be setting a flag to tell a main loop function to write out data */
// Send the data in the TX buffer to the master.
i2c_write (SYSTEM_BUS, txSlaveBuffer[slaveIndexTX++]);
//i2c_write_slave (SYSTEM_BUS, txSlaveBuffer[slaveIndexTX++]);
}
// Receives a message from the master.
else if (state > RECEIVE_I2C)
{
/* this will be removed. */
// Store the message in the RX buffer.
rxSlaveBuffer[slaveIndexRX++] = i2c_read (SYSTEM_BUS);
}
else
{
//Do nothing.
}
} |
This issue is that the Master (PC/Widows) will write a message and then sleep a bit, then read for a response. If the PIC hasn't had time to populate the outgoing txSlaveBuffer, bad data gets read by the host.
I am trying to disable the ISR "master read" section, and just set a flag that tells the main loop to manually write the bytes out. I am hoping that the i2c_read (SYSTEM_BUS,2); will be enough to make the master block until my other code starts doing the i2c_write in a loop.
So far, my manual write routine does send back the first response but then gets stuck and the master times out.
I am re-using my polled I2C routines from my Bootloader project, and want to just set a global "time to respond to the Master" flag and detect that in this polled code. It looks like this (posted elsewhere in this forum, original version):
| Code: | unsigned int i2cSlaveRespond (void *buffer, size_t size)
{
unsigned int bytesTransferred = 0;
// NULL check.
if ((buffer == NULL) || (size == 0)) return 0;
/* this was part of the polled routine and may not be needed for this */
do
{
// Wait for START bit
while (S == 0);
// Wait for first address byte so we can tell if this is a Read or Write.
while (I2C_SYSTEM_BUS_IRQ_PENDING_BIT == 0);
// Do not clear. We may need it pending for main loop below.
// If this is a Read from Master, we are done here.
if (RW == 1) break;
// Else, it was unexpected. Master was Writing, not reading.
//DEBUG_PRINTF ("Unexpected Master Write.\r\n");
// Consume incoming message.
while (P == 0)
{
if (RBF == 0)
{
i2c_read (SYSTEM_BUS);
}
}
I2C_SYSTEM_BUS_IRQ_PENDING_BIT = 0;
return 0;
} while (TRUE); // To remove CCS warning.
// Process INCOMING address and OUTGOING data bytes.
while (TRUE) // To remove CCS warning.
{
unsigned int state = 0;
unsigned int value = 0;
// Wait for I2C interrupt pending.
//while (I2C_SYSTEM_BUS_IRQ_PENDING_BIT == 0);
while (g_i2cRead == FALSE);
g_i2cRead = FALSE;
/* i disabled this, since it looks like the ISR is handling this. */
//I2C_SYSTEM_BUS_IRQ_PENDING_BIT = 0; // Clear IRQ flag
state = i2c_isr_state(SYSTEM_BUS);
value = (state & ~BIT(7)); // Mask off high bit.
// High bit (1xxxxxxx) indicates READ, else WRITE
if ((state & BIT(7)) == 0) // High bit CLEAR (Read from Master)
{
// Read? We were trying to write!
break;
}
else // High bit SET (Slave writing to Master)
{
if (value == 0x00)
{
// 0x80 - Address match received with R/W bit set; perform
// i2c_read( ) to read the I2C address, and use i2c_write( ) to
// pre-load the transmit buffer for the next transaction (next
// I2C read performed by master will read this byte).
/* this read will be removed, since it will be done in the ISR from the first state of 0x80 */
i2c_read (SYSTEM_BUS, 2);
i2c_write (SYSTEM_BUS, buffer[0]);
}
else
{
// 0x81-0xFF - Transmission completed and acknowledged; respond
// with i2c_write() to pre-load the transmit buffer for the next
// transition (the next I2C read performed by master will read
// this byte).
i2c_write (SYSTEM_BUS, buffer[bytesTransferred]);
if (bytesTransferred < size)
{
i2c_write (SYSTEM_BUS, buffer[value]);
}
else
{
// Is Master reads more than we have, give them FFs!
i2c_write (SYSTEM_BUS, 0xff);
}
}
// TODO: This returns the number of bytes the Master read, so it
// would return => size if it sent the full response. Or, we
// could only increment it in the two cases above, so it returns
// size max. Do we care about indicating that the master tried
// to read too much?
}
// Done when STOP bit is seen.
// If Reading, we read however many bytes the Master wrote,
// or up to 'size' bytes.
// If Writing, we send however many bytes the Master reads,
// or up to 'size' bytes.
/* this is something I need to fix, since the ISR has already cleared the pending bit */
// Wait for either a stop bit, or another incoming data byte.
//while ((P == 0) && (I2C_SYSTEM_BUS_IRQ_PENDING_BIT == 0));
if (P != 0)
{
break;
}
// To avoid returining +1, we increment after the Stop bit check.
bytesTransferred++;
} // end of while (1)
return bytesTransferred;
} // end of i2cSlaveRespond ()
#endif
|
So basically, I am trying to mix the ISR code for incoming data, then run my polled code for outgoing responses.
Since the first IRQ from a Master Read is supposed to read the address then write the first response byte (which may not be available yet) before another IRQ is generated, I don't think I can do this all in the ISR.
I just started working on this, and am still trying to work out the best approach. I just started re-using code I already wrote.
Cheers.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Using: 24FJ256GA106, 24EP256GP202 and 24FJ64GA002. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19549
|
|
| Posted: Thu Oct 22, 2020 11:30 am |
|
|
Why should 'TWO' be 2?. Unless you have 'defined it to be 2, it won't be.
TWO is _not_ predefined for you.
This means that the call you have will not have the right value, Result,
won't work...
Only NULL is defined (in the chip header file). ONE and TWO do not exist.The compiler will complain that this is an undefined identifier, unless you #define
it. |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 569
Location: Des Moines, Iowa, USA
|
|
| Posted: Thu Oct 22, 2020 11:33 am |
|
|
| Ttelmah wrote: | | Why should 'TWO' be 2?. Unless you have 'defined it to be 2, it won't be. |
Basically every number is defined in a huge header file. I've never seen anyone do this before, but that is what I am working from.
I now have basic messaging working. I still have some hangs, but at least I think the concept is doable now.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Using: 24FJ256GA106, 24EP256GP202 and 24FJ64GA002. |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 569
Location: Des Moines, Iowa, USA
|
|
| Posted: Thu Oct 22, 2020 12:32 pm |
|
|
I've now simplified my design so I can keep the ISR doing all the work, but have the main loop kick off the response to the master.
It will look something like this:
| Code: | void si2c2_isr (void)
{
int state = i2c_isr_state (SYSTEM_BUS);
// This detects data is ready to be received from the master.
if (state == RECEIVE_I2C)
{
// Get a byte of data which is the address of the board.
boardAddress = i2c_read (SYSTEM_BUS);
}
// This detects data is ready to be sent to the master.
else if (state == TRANSMIT_I2C)
{
// Get a byte of data which is the address of the board, and do not release clock at end of read.
i2c_read (SYSTEM_BUS, TWO);
g_i2cRespondToMaster = TRUE;
}
// Transmits a message to the master.
else if (state > TRANSMIT_I2C)
{
// Send the data in the TX buffer to the master.
i2c_write (SYSTEM_BUS, txSlaveBuffer[slaveIndexTX++]);
}
// Receives a message from the master.
else if (state > RECEIVE_I2C)
{
// Store the message in the RX buffer.
rxSlaveBuffer[slaveIndexRX++] = i2c_read (SYSTEM_BUS);
}
else
{
//Do nothing.
}
}
|
Then there is a routine I call after I have processed the incoming message from the host and put the response in the output (txBuffer)... Since this new ISR only does the i2c_read on the 0x80 state, it needs the i2c_write done to response to the master (clock stretch wait in progress) then subsequent IRQs will come in.
The routine could be something like this:
| Code: | void routine()
{
while (g_i2cRespondToMaster == FALSE);
g_i2cRespondToMaster = FALSE;
i2c_write (SYSTEM_BUS, buffer[0]);
// The rest will be handled by the ISR.
} |
The global is non-atomic and that will be changed to an increment/decrement.
The idea here is in the code there is stuff that parses an incoming message then calls a routine that sends a response back, like:
| Code: |
case SEND_ACK_BACK:
// copy ACK message bytes into the txSlaveBuffer
// master will know how many bytes to read based on the type of response it is expecting.
routine(); // call routine to kicks start sending back to master when it is ready
break;
|
That's the work-in-progress idea. It works for a bit then there is a hang, so I am still figuring out timing.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Using: 24FJ256GA106, 24EP256GP202 and 24FJ64GA002. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19549
|
|
| Posted: Fri Oct 23, 2020 12:59 am |
|
|
I must admit I'd never try to mix between using an ISR, and polled
operation. It seems to be 'asking for trouble'. Handle everything in the ISR.
If you have not got data available, then design the I2C protocol so that
the master can 'poll' a status register and get a bit to indicate that the
slave has the data available or not.
I suspect that the act of clearing the interrupt automatically clears the
clock stretch, so it can't be held outside the ISR. The reference for the
PIC24 I2C, says that clearing the interrupt also clears ACKEN.
So you really need to decide whether you are going to use polled or
ISR operation and use one or the other, not a mix. |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 569
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Oct 23, 2020 12:00 pm |
|
|
I wish we had a choice
How could an I2C master tell if the slave was ready?
When I2C is being handled by an ISR, it's always going to respond, whether the buffer has legit data in it or not.
My new method fully uses the ISR for all incoming and outgoing data *except* the first byte of the slave response. This has allowed me to eliminate all the sleeps in the Master and greatly improve throughput.
The old Master code (Windows, using the buggy FTDI drivers) would do this:
| Code: | I2C_Write (message);
Sleep (SOME_VALUE_FOR_PIC_TO_GET_REPLY_READY);
I2C_Read(); |
Now can can get rid of all those Sleeps. The Master does a read, the ISR sees it and does the "i2c_read(x, 2)" to get the address, then clock stretching is on and the Master is held.
In the non-ISR context, as soon as the response buffer is populated, there is a manual "i2c_write()" of the first byte in the buffer, then the Master receives and does the next read request and from there on the ISR serves up the rest.
"Wish I knew then what I know now." Ended up being a very trivial change to get where we needed to be.
Now I'm stuck debugging an issue with "i2c_transfer_out" that is sending all 00s on the wire, even though the buffer pointer contains data. Maybe something just died in the chip (this is a sub-bus one of our PIC systems uses; the master bus to the host PC is still working fine).
Debugging is a never ending rabbit hole...
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Using: 24FJ256GA106, 24EP256GP202 and 24FJ64GA002. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19549
|
|
| Posted: Fri Oct 23, 2020 12:13 pm |
|
|
You do what standard I2C devices do.
You have it when you read register 0, return a device status.
This has a bit to say that data is ready.
So when your buffer is full with the data to be read, set this bit.
When the data is written to the master turn it off again, and only
set it again when new data is loaded. |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 569
Location: Des Moines, Iowa, USA
|
|
| Posted: Wed Oct 28, 2020 9:52 am |
|
|
| Ttelmah wrote: | You do what standard I2C devices do.
You have it when you read register 0, return a device status.
This has a bit to say that data is ready.
So when your buffer is full with the data to be read, set this bit.
When the data is written to the master turn it off again, and only
set it again when new data is loaded. |
This does not apply. Using the provided ISR example from CCS, there is *always* data ready. The moment the Master reads, the ISR just pulls data from a buffer, whether that buffer is populated or not.
In polled mode this is much easier -- I've already implemented that, checking for S and P bits and such.
So when the Master (a PC running FTDI USB I2C drivers) does the following:
| Code: | I2C_Write()
Sleep (x)
I2C_Read () |
...there may be nothing in the buffer yet. But, the Read hits the PIC ISR and reads the address and then does an i2c_write() back of the outgoing buffer...
My code blocks that, by deferring that i2c_write() until it is full.
But I am running in to issues, as usual. Four of our boards can survive overnight continuous testing, but one PIC board will sometimes miss something and lock the bus. Same code. I've been debugging this all week.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Using: 24FJ256GA106, 24EP256GP202 and 24FJ64GA002. |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 569
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Oct 30, 2020 12:23 pm |
|
|
I hope to share the code when I get it fully working.
More progress... The challenge was having IRQ-driven Slave I2C code for receiving and responding to a Master.
The biggest challenge was that the receive ISR wouldn't have a Stop Bit flag set yet, so you couldn't tell when the master was done writing data. I thought there was a way to get an IRQ on the Stop bit, but the datasheet disagrees for the PIC24 I am using.
In the main() loop, I am checking for the P status and then setting a flag for the state machine to start processing whatever is in the RX buffer. Similar code is done for the transmission step of the state machine to know the data has been transmitted. (Existing code clears out the TX and RX buffer.)
I have uncovered a variety of problems with the existing process. For example, if the main() code is processing and a new message comes in, by the time the 'TX message has been sent' code is hit, it then wipes out the RX buffer that is now receiving new data. I'll be fixing that.
Also, due to how long the main() loop can take, there are times when the master has already started to read (thus, P bit is cleared) before the code has had a chance to detect and parse the incoming message.
Between debugging existing code and implementing the new stuff, it's been quite the week.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Using: 24FJ256GA106, 24EP256GP202 and 24FJ64GA002. |
|
|
|
|
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
