STM32F446 I2C peripheral won't assert repeated start condition

Question

I am writing some register level firmware for my STM32F446 to use the I2C peripheral to talk to an accelerometer. The sequence of events i need to do is as follows

1) START
2) SLAVE ADDRESS+Write
3) SUBADDRESS (command byte to specify which register address on the peripheral i want to read from)
4) REPEAT START
5) SLAVE ADDRESS+Read
6) Collect data from slave
7) STOP

On a logic analyzer i can see that i am correctly doing steps 1-3, but when I need to assert the repeated start it seems to not do so correctly. I am setting the I2C1->CR1 register bit 8 (from zero) high with this function:

void repeat_start_I2C1(){
  //set start bit
  I2C1->CR1 |= (0x1 << 0x8); 
}

which seems to work for my first start but it is not generating the repeated start, because it is not generating the repeated start my read function (used in step number 5) gets stuck in a while loop waiting for the status register bit SB to go high before proceeding (SB goes high after each start/repeat start and it is necessary to clear before sending slave address):

void address_read_I2C1(uint8_t addr){
  //wait for SB bit to be set
  while(!(I2C1->SR1 & 0x1));

  //write slave address with LSB set for receive mode
  I2C1->DR = (addr << 1) | 0x1;
}

Here is an image of the full transmission as it it occurs up until the false repeated start which occurs directly after 0x2D+ACK (the subaddress command byte, i.e., step 3).

Here is a close up of the false repeated start

As you can see at the +1us tick mark the SCL (top line) and SDA (Bottom line) go low at the exact same time (up to the resolution of the logic analyzer) instead of SDA->SCL as needed in a start condition (which can be properly seen in the first image.

I paused during debug while it was stuck in the while loop and noticed that the Status Register 2 BTF bit was high indicating that transmission was finished and this may be the reason why it is unable to assert start correct? But i don't understand the procedure i must take to fix it so that it can repeat start.

EDIT:

a user suggested i might be leaving out a step to clear the address status bit before sending a command byte, but i simpyl forgot to show this function

//Write byte to I2C1
void write_I2C1_byte(uint8_t cmd){
  uint8_t tmp;

  //wait for ADDR=1 then clear by reading SR1 and SR1
  while(!(I2C1->SR1 && 0x02));
  tmp = I2C1->SR1;
  tmp = I2C1->SR2;

  //wait for TxE=1 to denote data register ready to accept transmission data
  while(!(I2C1->SR1 && 0x40)); 
  I2C1->DR = cmd;
}

This function carries out step 3 in my list of steps by clearing ADDR by reading the SR registers and then sending a byte to DR. I compile with no optimizations so tmp does not get cut from the code for not being used.

EDIT 2:

A user asked what pull-up resistors are set to for the gpio connected to the peripheral, here is the init function that set the pull-up resistors to pull lines high (spaced out relevant lines):

inline void init_i2c1() {

  //set GPIO to correct pin modes for I2C1
  //PB_8 IC21_SCL
  //PB_9 I2C1_SDA

  // enable GPIOB clock
  //RCC->AHB1ENR |= RCC_AHB1ENR_GPIOBEN;
  RCC->AHB1ENR |= 1 << (1); //set GPIOB clock enable bit
  //enable I2C1 Clock
  //RCC->APB1ENR |= RCC_APB1ENR_I2C1EN;
  RCC->APB1ENR |= 1 << (21); //set I2C1 clock enable bit

  //PB_8 IC21_SCL
  // set the mode to alternate funtion
  GPIOB->MODER &= ~(0b11 << (8 * 2));     // clear bit field using 2 bit mask
  GPIOB->MODER |= 0b10 << (8 * 2); //set bit filds to 0b10 to enable alternate function mode
  // set output mode to open drain so it can be pulled low
  GPIOB->OTYPER |= (1 << (8));
  // set GPIO speed
  GPIOB->OSPEEDR &= ~(0b11 << (8 * 2)); //clear
  GPIOB->OSPEEDR |= (0b10 << (8 * 2)); //fast speed - 50MHz

  //******************************************************
  // Set Pull up Resistors
  GPIOB->PUPDR &= ~(0b11 << (2 * 2)); //clear
  GPIOB->PUPDR |= (0b01 << (2 * 2)); // pull up
  //******************************************************


  //Set Alternate Function I2C1 using Alternate Function Register High
  GPIOB->AFR[1] &= ~(0b1111 << (0*4)); //clear field
  GPIOB->AFR[1] |= (0b0100 << (0*4)); //set for alternate function 4 (I2C 1)

  //PB_9 I2C1_SDA
  GPIOB->MODER &= ~(0b11 << (9 * 2));     // clear bit field using 2 bit mask
  GPIOB->MODER |= 0b10 << (9 * 2); //set bit filds to 0b10 to enable alternate function mode
  // set output mode to open drain so it can be pulled low
  GPIOB->OTYPER |= (1 << (9));
  // set GPIO speed
  GPIOB->OSPEEDR &= ~(0b11 << (9 * 2)); //clear
  GPIOB->OSPEEDR |= (0b10 << (9 * 2)); //fast speed - 50MHz


  //******************************************************
  // Set Pull up Resistors
  GPIOB->PUPDR &= ~(0b11 << (2 * 2)); //clear
  GPIOB->PUPDR |= (0b01 << (2 * 2)); // pull up
  //******************************************************


  //Set Alternate Function I2C1 using Alternate Function Register High
  GPIOB->AFR[1] &= ~(0b1111 << (1*4)); //clear field
  GPIOB->AFR[1] |= (0b0100 << (1*4)); //set for alternate function 4 (I2C 1)


  //set up I2C1
  //disable I2C1 to set up
  I2C1->CR1 &=~ (1 << 0); //Peripheral enable bit
  //set clock frequency to 100kHz
  I2C1->CR2 &=~ (0b111111);
  I2C1->CR2 |= (0b001000); //set I2C peripheral frequency to 8lMHz
  I2C1->CCR &=~ (1 < 15); //clear the 15th bit to set it to Sm mode

  //Calculate the Clock Control Bits
  // CCR[11:0] in decimal = ((1/2)(1/targetFrequency))/(1/PeripheralFrequency)
  // it is 1/2 the target period because the CRR defines Thigh or Tlow (half the period)
  // CCR = ((1/2)(1/100E3))/(1/8E6) decimal to hex
  // CCR = 0x28

  I2C1->CCR &=~ (0b111111111111); //clear the lowest 12 bits
  I2C1->CCR |= (0x28); //clear the lowest 12 bits

  //Set TRISE
  //TRise is the maximum rise time divided by the peripheral clock period + 1
  //TRISE = floor(Trise_max/Tpclk)+1
  //For Sm mode the max rise time is 1000ns
  //thus we have ((1000E-9/(1/8E6))+1) = 9
  I2C1->TRISE = 0b001001; //set TRISE to 9

  //Program the I2C_CR1 register to enable the peripheral
  I2C1->CR1 |= (1 << 0); //Peripheral enable bit

}

EDIT 3:

According to the STM32F446 Errata sheet section titled 2.4.3 Mismatch on the “Setup time for a repeated Start condition” timing parameter Apparently there is some cases where the setup time for repeated start is violated in I2C standard mode speeds between 88-100KHz (which i am currently operating in since i am at 100KHz). The required minimum Repeated Start setup time is 4.7us but looking at my logic analyzer output i have less than 3us between SCL going high and when the SDA line seems to attempt a repeated start. Though this problem is not supposed to occur if the slave is not stretching the clock and the SCL rise time is less than 300ns which my pictures both indicate so i am unsure why the setup is being violated.

I will try lowering the frequency below 88KHz as suggested workaround in the Errata, and will report back.

EDIT4: reducing SCL to below 80KHz did not do anything, in fact it made it worse. Now the first start condition has the same behavior (glitching downward at the exact same time as SDA). But this only happens intermittently, sometimes it works and sometimes it does not...

EDIT5: SOLVED. External pull-ups are very necessary... stupid nucleo board...

Answer 1

I've just checked against my code for the F401 - I hope that the I2C peripheral is still the same for the F446.

Your general approach is a match with mine (which works fine after heavy rework of the code from an interrupt driven approach - which was horrible).

But I do have an additional step between 2) and 3) which you haven't mentioned:

1) START
2) SLAVE ADDRESS+Write
--> 2.5) Clear Address Bits (read SR1 and SR2)
3) SUBADDRESS 
4) REPEAT START
5) SLAVE ADDRESS+Read
6) Collect data from slave
7) STOP

I found the I2C peripheral to be quite brittle and ugly to use. Bbeware not to read too much or too little from the registers as the internal state machine seems to be heavily dependent on the correct sequence of the read and the writes to the registers.

Especially the part about reading the registers can give you a debugging hell as the read of the debugger will trigger the state machine just like a read of your code, so as soon as you stop and inspect the registers (sometimes the debugger does it all automatically) the read happens and the peripheral will do something again.

If you need to know a value at a certain point, store it inside a variable as you cannot rely on what the debugger is showing if you halt at that point. But then again that read might alter the behavior already. Read the reference manual very very carefully.

---

In response to the edit:

//Write byte to I2C1
void write_I2C1_byte(uint8_t cmd){
  uint8_t tmp;

  //wait for ADDR=1 then clear by reading SR1 and SR1
  //// check address sent succeeded, data bytes transmitted, bus busy, and in master mode)
  //while(!( (I2C2->SR1 && 0X02) && (I2C2->SR2 && 0x07) ));
  while(!(I2C1->SR1 && 0x02));
  tmp = I2C1->SR1;
  tmp = I2C1->SR2;

  //wait for TxE=1 to denote data register ready to accept transmission data
  while(!(I2C1->SR1 && 0x40)); 
  I2C1->DR = cmd;
}

This code contains bugs - the waiting for the bits. You accidentally use && instead of & to check for a set bit.

And you commented that tmp isn't getting optimized away because you have compile with no optimizations. tmp wouldn't get optimized away as the registers are (if nothing went wrong there) declared volatile, thus the compiler is not allowed to optimize away the read from SR1 or SR2.

---

After inspecting the captured waveforms a bit closer, it looked like there is a glitch on the SCL on the restart condition. This might just be a wrong capture or a real glitch, which could very well throw off the peripheral as it is closely coupled to what is going on on the SCL line.

After clearing up, that the internal pull-ups were used, I suggested to use external 10k pull-up resistors (in a comment).

The reason why is that I have encountered strange behavior of GPIO pins under control of a peripheral. Namely some of the SPI pins wen't into high impedance mode after a transmission (which caused EMI problems). So I'm not trusting that the internal pull-up will remain always active.

It could also be just that the internal pull-up is just not strong enough. They typically range in between 30k and 60k ohm, so they are quite a bit weaker.