Skip to content

Add initial handling for long I2C reads.#751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lonerzzz merged 1 commit into from
Oct 23, 2017
Merged

Add initial handling for long I2C reads. #751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 70 additions & 52 deletions cores/esp32/esp32-hal-i2c.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,6 +31,8 @@
#define DR_REG_I2C_EXT_BASE_FIXED 0x60013000
#define DR_REG_I2C1_EXT_BASE_FIXED 0x60027000

#define COMMAND_BUFFER_LENGTH 16

struct i2c_struct_t{
i2c_dev_t * dev;
#if !CONFIG_DISABLE_HAL_LOCKS
Expand DownExpand Up@@ -127,26 +129,25 @@ void i2cSetCmd(i2c_t * i2c, uint8_t index, uint8_t op_code, uint8_t byte_num, bo
i2c->dev->command[index].op_code = op_code;
}

void i2cResetCmd(i2c_t * i2c){
int i;
void i2cResetCmd(i2c_t * i2c){
uint8_t i;
for(i=0;i<16;i++){
i2c->dev->command[i].val = 0;
}
}

void i2cResetFiFo(i2c_t * i2c)
{
void i2cResetFiFo(i2c_t * i2c){
i2c->dev->fifo_conf.tx_fifo_rst = 1;
i2c->dev->fifo_conf.tx_fifo_rst = 0;
i2c->dev->fifo_conf.rx_fifo_rst = 1;
i2c->dev->fifo_conf.rx_fifo_rst = 0;
}

i2c_err_t i2cWrite(i2c_t * i2c, uint16_t address, bool addr_10bit, uint8_t * data, uint8_t len, bool sendStop)
i2c_err_t i2cWrite(i2c_t * i2c, uint16_t address, bool addr_10bit, uint8_t * data, uint16_t len, bool sendStop)
{
int i;
uint8_t index = 0;
uint8_t dataLen = len + (addr_10bit?2:1);
uint16_t index = 0;
uint16_t dataLen = len + (addr_10bit?2:1);
address = (address << 1);

if(i2c == NULL){
Expand DownExpand Up@@ -244,12 +245,25 @@ i2c_err_t i2cWrite(i2c_t * i2c, uint16_t address, bool addr_10bit, uint8_t * dat
return I2C_ERROR_OK;
}

i2c_err_t i2cRead(i2c_t * i2c, uint16_t address, bool addr_10bit, uint8_t * data, uint8_t len, bool sendStop)
uint8_t inc( uint8_t* index )
{
uint8_t i = index[ 0 ];
if (++index[ 0 ] == COMMAND_BUFFER_LENGTH)
{
index[ 0 ] = 0;
}

return i;
}

i2c_err_t i2cRead(i2c_t * i2c, uint16_t address, bool addr_10bit, uint8_t * data, uint16_t len, bool sendStop)
{
address = (address << 1) | 1;
uint8_t addrLen = (addr_10bit?2:1);
uint8_t index = 0;
uint8_t cmdIdx;
uint8_t amountRead[16];
uint16_t index = 0;
uint8_t cmdIdx = 0, currentCmdIdx = 0, nextCmdCount;
bool stopped = false, isEndNear = false;
uint8_t willRead;

if(i2c == NULL){
Expand All@@ -269,86 +283,90 @@ i2c_err_t i2cRead(i2c_t * i2c, uint16_t address, bool addr_10bit, uint8_t * data
i2cResetCmd(i2c);

//CMD START
i2cSetCmd(i2c, 0, I2C_CMD_RSTART, 0, false, false, false);
i2cSetCmd(i2c, cmdIdx++, I2C_CMD_RSTART, 0, false, false, false);

//CMD WRITE ADDRESS
Copy link
Contributor

@stickbreakerstickbreakerOct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 bit address are sent high byte, low byte. Also the address is modified before it is sent.
Philips UM10204 pg.15 3.1.11 10-bit addressing:
The first 7 bits of the first byte are the combination of 1111 0xx of which the last two bits (xx) are the two Most-Significant Bits (MSB) of the 10-bit address; the eighth bit of the first byte is the R/W bit that determines the direction of the message.
So, This section should be changed to:

// address contains left shifted address with bit0 set for READif(addr_10bit){i2c->dev->fifo_data.val =((address>>8)& 0x06) | 0xF1; // send a9:a8 and read bit with 1111 0xxR mask i2c->dev->fifo_data.val=((address>>1) & 0xFF); // send a7:a0 } else{i2c->dev->fifo_data.val=(address & 0xFF) ; // send a6:a0 with READ bit }

Copy link
ContributorAuthor

@lonerzzzlonerzzzOct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code shown is filling the transmission pipeline, it is not about the addressing, but rather the COMMAND buffer that holds the series of I2C actions to perform.

I assume you are referring to the next block for CMD WRITE ADDRESS, I see your point. I didn't even look at this code and just moved it as I was focused on the long reads and my hardware only uses 7 bit addresses. You should raise a separate issue for the addressing issue.

Copy link
Contributor

@stickbreakerstickbreakerOct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonerzzz Ok, I'll create a pull with just this 10bit addressing in the i2cWrite() and i2cRead().

Copy link
ContributorAuthor

@lonerzzzlonerzzzOct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stickbreaker, I'll put in your 10 bit fix following mine if @me-no-dev accepts my fix. Let me know if there are concerns.

Copy link
Contributor

@stickbreakerstickbreakerOct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, Just replace the existing code for 10bit addresses in BOTHi2cWrite() and i2cRead() with my snippet. That way any 10bit address will match Industry specs.

Chuck.

i2c->dev->fifo_data.val = address & 0xFF;
if(addr_10bit){
i2c->dev->fifo_data.val = (address >> 8) & 0xFF;
}
i2cSetCmd(i2c, 1, I2C_CMD_WRITE, addrLen, false, false, true);
i2cSetCmd(i2c, cmdIdx++, I2C_CMD_WRITE, addrLen, false, false, true);
nextCmdCount = cmdIdx;

while(len){
cmdIdx = (index)?0:2;
willRead = (len > 32)?32:(len-1);
if(cmdIdx){
i2cResetFiFo(i2c);
}

if(willRead){
i2cSetCmd(i2c, cmdIdx++, I2C_CMD_READ, willRead, false, false, false);
}

if((len - willRead) > 1){
i2cSetCmd(i2c, cmdIdx++, I2C_CMD_END, 0, false, false, false);
} else{
willRead++;
i2cSetCmd(i2c, cmdIdx++, I2C_CMD_READ, 1, true, false, false);
if(sendStop){
i2cSetCmd(i2c, cmdIdx++, I2C_CMD_STOP, 0, false, false, false);
}
}

//Clear Interrupts
i2c->dev->int_clr.val = 0xFFFFFFFF;

//START Transmission
i2c->dev->ctr.trans_start = 1;
//Clear Interrupts
i2c->dev->int_clr.val = 0x00001FFF;

//START Transmission
i2c->dev->ctr.trans_start = 1;
while (!stopped){
//WAIT Transmission
uint32_t startAt = millis();
while(1){
//have been looping for too long
if((millis() - startAt)>50){
log_e("Timeout! Addr: %x", address >> 1);
if((millis() - startAt)>50){
Copy link
Contributor

@stickbreakerstickbreakerOct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you exit right here, the I2C State Machine is Where? I think some Reset/Cleanup action need to be executed. The I2C protocol allows the Slave device to pause the Master by holding SCL low. I have not been able to determine the 'Official' maximum pause. Philips UM10204: pg.48 says 'Tlow min 4.7us, max = unlimited. So, theoretically, a valid transaction could take until the death of the Universe!

Copy link
ContributorAuthor

@lonerzzzlonerzzzOct 21, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are exiting at that point, it is because the state machine has failed or the slave is not responding. The program needs to decide how to handle such as by calling i2cReset to reset the hardware state machine.

The problem with the current ESP32 hardware implementation of I2C is that it really doesn't support the slave holding the SCL low for very long at all. There is a timer in i2cInit that I have set to its longest value on line 455. I tried setting it longer with no additional benefit so it appears to be at its actual maximum. Numerous people have commented on this implementation flaw but the current version of the chip is what it is. In the IDF forums, making the ESP I2C implementations compliant to the spec is one of the big requests for the next chip revision.

Copy link
Contributor

@stickbreakerstickbreakerOct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonerzzz Have you thought about address NAK, and Arbitration Lost errors? If you setup a list of cmd ops:

  1. start
  2. write address (to select the address of the Slave)
  3. Read block of bytes
  4. Stop

And then you get a address NAK, does that cause the I2C StateMachine(SM)to halt? or does is continue with the READ command? The Docs are very Fuzzy!
I was just thinking that if an Address NAK event happened, you should cleanup the CMD Stack and ReInit the SM back to an Idle condition. Then return the NAK error to the APP.

Copy link
ContributorAuthor

@lonerzzzlonerzzzOct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, the docs need some work. When I finally got through to the espressif folks a week ago, they indicated they were working on I2C code fixes as well as documentation and would like my fix to include so I was trying to get it done amongst other tasks. The good news is at least we know some more improvements are coming. They are definitely needed.

To your question. my understanding is that most errors do cause the SM to halt as one would expect but I have seen situations where that was not the case but 'reproducability' was the biggest obstacle to getting to the root of it.

You are correct. We could clean up on the NAK if we can confirm that the SM is stable enough to properly stop the pipeline. My assumption so far has been that in these situations, I let the application layer call i2cReset and reset the whole state machine simply because I am not sure of what all the problems with the SM are.

log_e("Timeout! Addr: %x, index %d", (address >> 1), index);
I2C_MUTEX_UNLOCK();
return I2C_ERROR_BUS;
}

//Bus failed (maybe check for this while waiting?
if(i2c->dev->int_raw.arbitration_lost){
log_e("Bus Fail! Addr: %x", address >> 1);
log_e("Bus Fail! Addr: %x", (address >> 1));
I2C_MUTEX_UNLOCK();
return I2C_ERROR_BUS;
}

//Bus timeout
if(i2c->dev->int_raw.time_out){
log_e("Bus Timeout! Addr: %x", address >> 1);
log_e("Bus Timeout! Addr: %x, index %d", (address >> 1), index );
I2C_MUTEX_UNLOCK();
return I2C_ERROR_TIMEOUT;
}

//Transmission did not finish and ACK_ERR is set
if(i2c->dev->int_raw.ack_err){
log_w("Ack Error! Addr: %x", address >> 1);
while((i2c->dev->status_reg.bus_busy) && ((millis() - startAt)<50));
I2C_MUTEX_UNLOCK();
return I2C_ERROR_ACK;
}

if(i2c->dev->command[cmdIdx-1].done){
break;
// Save bytes from the buffer as they arrive instead of doing them at the end of the loop since there is no
// pause from an END operation in this approach.
if((!isEndNear) && (nextCmdCount < 2)){
if (willRead = ((len>32)?32:len)){
if (willRead > 1){
i2cSetCmd(i2c, cmdIdx, I2C_CMD_READ, (amountRead[ inc( &cmdIdx ) ] = willRead -1), false, false, false);
nextCmdCount++;
}
i2cSetCmd(i2c, cmdIdx, I2C_CMD_READ, (amountRead[ inc( &cmdIdx ) ] = 1), (len<=32), false, false);
nextCmdCount++;
len -= willRead;
} else{
i2cSetCmd(i2c, inc( &cmdIdx ), I2C_CMD_STOP, 0, false, false, false);
isEndNear = true;
nextCmdCount++;
}
}
}

int i = 0;
while(i<willRead){
i++;
data[index++] = i2c->dev->fifo_data.val & 0xFF;
if(i2c->dev->command[currentCmdIdx].done){
nextCmdCount--;
if (i2c->dev->command[currentCmdIdx].op_code == I2C_CMD_READ){
while(amountRead[currentCmdIdx]>0){
data[index++] = i2c->dev->fifo_data.val & 0xFF;
amountRead[currentCmdIdx]--;
}
i2cResetFiFo(i2c);
} else if (i2c->dev->command[currentCmdIdx].op_code == I2C_CMD_STOP){
stopped = true;
}
inc( &currentCmdIdx );
break;
}
}
len -= willRead;
}
I2C_MUTEX_UNLOCK();

return I2C_ERROR_OK;
}

Expand DownExpand Up@@ -425,7 +443,7 @@ i2c_t * i2cInit(uint8_t i2c_num, uint16_t slave_addr, bool addr_10bit_en)
DPORT_SET_PERI_REG_MASK(DPORT_PERIP_CLK_EN_REG,DPORT_I2C_EXT1_CLK_EN);
DPORT_CLEAR_PERI_REG_MASK(DPORT_PERIP_RST_EN_REG,DPORT_I2C_EXT1_RST);
}

I2C_MUTEX_LOCK();
i2c->dev->ctr.val = 0;
i2c->dev->ctr.ms_mode = (slave_addr == 0);
Expand All@@ -434,7 +452,7 @@ i2c_t * i2cInit(uint8_t i2c_num, uint16_t slave_addr, bool addr_10bit_en)
i2c->dev->ctr.clk_en = 1;

//the max clock number of receiving a data
i2c->dev->timeout.tout = 400000;//clocks max=1048575
i2c->dev->timeout.tout = 1048575;//clocks max=1048575
//disable apb nonfifo access
i2c->dev->fifo_conf.nonfifo_en = 0;

Expand Down
4 changes: 2 additions & 2 deletions cores/esp32/esp32-hal-i2c.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -48,8 +48,8 @@ i2c_err_t i2cDetachSCL(i2c_t * i2c, int8_t scl);
i2c_err_ti2cAttachSDA(i2c_t*i2c, int8_tsda);
i2c_err_ti2cDetachSDA(i2c_t*i2c, int8_tsda);

i2c_err_ti2cWrite(i2c_t*i2c, uint16_taddress, booladdr_10bit, uint8_t*data, uint8_tlen, boolsendStop);
i2c_err_ti2cRead(i2c_t*i2c, uint16_taddress, booladdr_10bit, uint8_t*data, uint8_tlen, boolsendStop);
i2c_err_ti2cWrite(i2c_t*i2c, uint16_taddress, booladdr_10bit, uint8_t*data, uint16_tlen, boolsendStop);
i2c_err_ti2cRead(i2c_t*i2c, uint16_taddress, booladdr_10bit, uint8_t*data, uint16_tlen, boolsendStop);

voidi2cReset(i2c_t*i2c);

Expand Down