STM32F103 I2C: When reading exactly two bytes the first byte is not acknowledged by the I2C master.

strongly-typed commented 8 years ago

I stumbled over a coarse resolution of the BMP180 sensor connected to an STM32F103C8T6 at I2cMaster1. When reading exactly two bytes the first byte is not acknowledged by the master so that the slave will not send its second byte. The value is then always 0xff and that resulted in a coarse temperature resolution.

Nevertheless the transaction returns true.

 this->transaction.configureWriteRead(buffer, 1, data.raw, 2);

When reading three or more bytes the first data read is acknowledged.

 this->transaction.configureWriteRead(buffer, 1, data.raw, 3);

With an STM32F407 discovery board the two bytes read works correctly:

salkinium commented 8 years ago

cc @Sh4rK, that's the bug you're seeing. Quickfix is to use xpcc::SoftwareI2cMaster.

We desperately need hwut.

salkinium commented 8 years ago

The reference manual between F1 and F4 is identical EXCEPT in drumroll 2B I2C Master Reception. I hate ST so much right now.

salkinium commented 8 years ago

I mean srsly? Really? Do we really need to double check every little stupid thing for them? I'm tempted to just disable HW I2C for the entire F1 series.

strongly-typed commented 8 years ago

+1 for HWUT

-1 Disabling I2C

That would be sad as we are so close to finally supporting it.

Here the Uart Trace from the I2C. I had to use xpcc::IoBuffer::DiscardIfFull and so I truncated the debug output. I2C is running at 10 kHz, Uart at 921600 baud. I adjusted the trace so that the ends match again.

STM32F4	STM32F1

###	###

c St	c St
en IRQ	en IRQ

- IRQ -	- IRQ -
SB set	SB set
W op 2	W op 2

- IRQ -	- IRQ -
addr sent	addr sent
w =2	w =2
r =0	r =0
en buf	en buf
cl ADR	cl ADR

- IRQ -	- IRQ -
tx mo	tx mo
TXE 1	TXE 1

- IRQ -	- IRQ -
tx mo	tx mo
TXE 0	TXE 0
lst tx, btf	lst tx, btf

- IRQ -	- IRQ -
lst tx, btf	lst tx, btf
BTF	BTF
BTF, write=0	BTF, write=0
STOP	STOP
disable interrupts	disable interrupts
transaction finished	transaction finished

###	###

c St	c St
en IRQ	en IRQ

- IRQ -	- IRQ -
SB set	SB set
W op 1	W op 1

- IRQ -	- IRQ -
addr sent	addr sent
w =1	w =1
r =0	r =0
en buf	en buf
cl ADR	cl ADR

- IRQ -	- IRQ -
tx mo	tx mo
TXE 0	TXE 0
lst tx, btf	lst tx, btf

- IRQ -	- IRQ -
lst tx, btf	lst tx, btf
BTF	BTF
BTF, write=0	BTF, write=0
c St	c St
en IRQ	en IRQ
restart op	restart op

- IRQ -	- IRQ -
lst tx, btf	lst tx, btf
BTF	BTF

- IRQ -
lst tx, btf
BTF

- IRQ -
lst tx, btf
BTF

- IRQ -
lst tx, btf
BTF

- IRQ -
lst tx, btf
BTF

- IRQ -

- IRQ -	- IRQ -
SB set	SB set
NACK	NACK
POS	POS
read op: reading=2	read op: reading=2

- IRQ -	- IRQ -
addr sent	addr sent
w =0	w =0
r =2	r =2
cl ADR	cl ADR

- IRQ -	- IRQ -
fourth last byte received, wa	fourth last byte received, wa
BTF	BTF
STOP	STOP
reading data1	reading data1
waiting for stop	waiting for stop
reading data2	reading data2
disable interrupts	disable interrupts
transaction finished	transaction finished

Sh4rK commented 8 years ago

Ok, I was starting to think I'm doing something really stupid. I'll try software I2C and hope it's working. (Also, I like the nooooooooooooooooooooooooo label)

strongly-typed commented 8 years ago

@Sh4rK I would strongly recommend that you use any kind of logic analyser. I've been using an FX2 based board even in my rail-office quite successfully. They are cheap and fast enough for I2C, UART, CAN and SPI. pulseview is a usable GUI (see screenshots above).

@salkinium: HWUT is not soo far away

$ sigrok-cli -d fx2lafw --config samplerate=1m --samples 100000 --channels 1=SCL,2=SDA --triggers SCL=f --wait-trigger --protocol-decoders i2c --protocol-decoder-annotations i2c=start:repeat-start:stop:ack:nack:address-read:address-write:data-read:data-write:warnings
Start
Write
Address write: 77
ACK
Data write: F4
ACK
Data write: 2E
ACK
Stop
Start
Write
Address write: 77
ACK
Data write: F6
ACK
Start repeat
Read
Address read: 77
ACK
Data read: 67
NACK
Data read: FF
NACK
Stop

If you have a recorded working sequence from an STM32F407 and compare it to the output of the actual device you will see the wrong data and the wrong NACK just by diff'ing the output of sigrok-cli.

I'm just preparing a board with some sensors (BMP180, BME280, ...), busses (I2C, SPI, UART, CAN) and a dedicated FX2 logic analyser for such tests.

salkinium commented 8 years ago

I can reproduce this problem locally using the NUCLEO-F103 and the TMP102 chip. Here is some non-truncated output for the start-write1-restart-read2-stop transaction:

callStarting
enable interrupts

--- interrupt ---
startbit set
write op: writing=1

--- interrupt ---
address sent
writing.length=1
reading.length=0
enable buffers
clearing ADDR

--- interrupt ---
tx more bytes
TXE: writing.length=0
last byte transmitted, wait for btf

--- interrupt ---
last byte transmitted, wait for btf
BTF
BTF, write=0
callStarting
enable interrupts
restart op

--- interrupt ---
startbit set
NACK
POS
read op: reading=2

--- interrupt ---
address sent
writing.length=0
reading.length=2
clearing ADDR

--- interrupt ---
fourth last byte received, wait for btf
BTF
STOP
reading data1
waiting for stop
reading data2
disable interrupts
transaction finished

salkinium commented 8 years ago

This behavior is addressed in the F10xxB Errata sheet (Section 2.13.2 on page 22). I'm trying to figure out if this is indeed the cause.

strongly-typed commented 8 years ago

Already checked the errata sheet and I'm not convinced that this describes the behaviour we observe. Actually, the data on the bus is not correct; it is not corrupted while reading it into the shift register.

the content of the shift register (data N) will be corrupted (data N is shifted 1-bit to the left).

Probably ST needs HWUT to test their séquence and this is connected to the problem we observe.

salkinium commented 8 years ago

@strongly-typed: None of the workarounds suggested in the errata sheet made any difference.

But: I have 2B reads working, but I cannot confirm with a logic analyzer. Change line 300 to this:

-    if (reading.length <= 2)
+    if (reading.length < 2)

I'll try to test this on the STM32F4 series.

strongly-typed commented 8 years ago

But: I have 2B reads working, but I cannot confirm with a logic analyzer.

Are you sure? Is the read of the second byte different from 0xff? Could you please check raw[1] in bmp085 after a read? You will observe that the temperature readings jump in steps larger than 0.1 degree (0.4 or 0.5 in my case) when the second byte is always 0xff.

Your fix kills my bus. The stop condition is not generated after the 2B read. Here the annotated cycle of the bmp180 sample. The bus is then broken.

Ping

Start
Write
Address write: 77
ACK
Stop

Read calibration

Start
Write
Address write: 77
ACK
Data write: AA
ACK
Start repeat
Read
Address read: 77
ACK
Data read: 1A
ACK
Data read: 95
ACK
Data read: FC
ACK
Data read: 0A
ACK
Data read: C6
ACK
Data read: 73
ACK
Data read: 84
ACK
Data read: 4C
ACK
Data read: 61
ACK
Data read: 97
ACK
Data read: 48
ACK
Data read: 40
ACK
Data read: 19
ACK
Data read: 73
ACK
Data read: 00
ACK
Data read: 21
ACK
Data read: 80
ACK
Data read: 00
ACK
Data read: D1
ACK
Data read: F6
ACK
Data read: 0B
ACK
Data read: 0E
NACK
Stop

Start conversion

Start
Write
Address write: 77
ACK
Data write: F4
ACK
Data write: 2E
ACK
Stop

Read temperature

Start
Write
Address write: 77
ACK
Data write: F6
ACK
Start repeat
Read
Address read: 77
ACK
Data read: 67
ACK
Data read: 75
ACK

After that the bus is broken.

Here the UART log

###

c St
en IRQ

- IRQ -
SB set
W op 1

- IRQ -
addr sent
w =1
r =0
en buf
cl ADR

- IRQ -
tx mo
TXE 0
lst tx, btf

- IRQ -
lst tx, btf
BTF
BTF, write=0
c St
en IRQ
restart op

- IRQ -
lst tx, btf
BTF

- IRQ -
SB set
ACK
POS
read op: reading=2

- IRQ -
addr sent
w =0
r =2
cl ADR

- IRQ -
fourth last byte received, wait for btf
BTF
STOP
reading data1
waiting for stop
reading data2
disable interrupts
transaction finished

###

ERROR!
disable interrupts

###

salkinium commented 8 years ago

I rewrote my example to use the BMP085 and my "fix" kills my bus too.

strongly-typed commented 8 years ago

I'm just looking into AN2824 Figure 2:

POS = 1
Disable interrupts
Clear ADDR
ACK = 0
Enable interrupts
Wait for BTF = 1
Disable interrupts
STOP = 1
Read Data1
Enable interrupts
Read Data2
Wait until STOP is cleared by hardware.
POS = 0 and ACK = 1 (to be ready for another reception)..

salkinium commented 8 years ago

Even with that application note, I just can't make it work.

Furthermore I can't debug my Nucleo-F103 due to ST-Link issues (yay).

strongly-typed commented 8 years ago

At least that hack works on STM32F103 with 2-byte reads.

diff --git a/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in b/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in
index c4018fc..61dff3d 100644
--- a/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in
+++ b/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in
@@ -297,21 +295,9 @@ I2C{{ id }}_EV_IRQHandler(void)
            reading = transaction->reading();
            nextOperation = static_cast<xpcc::I2c::Operation>(reading.next);

-           if (reading.length <= 2)
-           {
-               DEBUG_STREAM("NACK");
-               I2C{{ id }}->CR1 &= ~I2C_CR1_ACK;
-           }
-           else
-           {
-               DEBUG_STREAM("ACK");
-               I2C{{ id }}->CR1 |= I2C_CR1_ACK;
-           }
-           if (reading.length == 2)
-           {
-               DEBUG_STREAM("POS");
-               I2C{{ id }}->CR1 |= I2C_CR1_POS;
-           }
+           DEBUG_STREAM("ACK");
+           I2C{{ id }}->CR1 |= I2C_CR1_ACK;
+
            DEBUG_STREAM("read op: reading=" << reading.length);
            break;

@@ -341,13 +327,25 @@ I2C{{ id }}_EV_IRQHandler(void)
{
    starting.address = 0;
    // EV6: ADDR=1, cleared by reading SR1 register followed by reading SR2.
+
+   if (reading.length == 1)
+   {
+       DEBUG_STREAM("NACK");
+       I2C{{ id }}->CR1 &= ~I2C_CR1_ACK;
+       (void) I2C{{ id }}->SR1; // Clear ADDR
+
+   } else if (reading.length == 2) 
+   {
+       DEBUG_STREAM("POS");
+       I2C{{ id }}->CR1 |= I2C_CR1_POS; // Set POS flag (NACK position next)
+   }

    if (writing.length > 0 || reading.length > 3)
    {
        I2C{{ id }}->CR2 |= I2C_CR2_ITBUFEN;
    }
    if (!reading.length && !writing.length)
@@ -376,6 +372,11 @@ I2C{{ id }}_EV_IRQHandler(void)
        *reading.buffer++ = dr & 0xff;
        reading.length = 0;
        checkNextOperation = CHECK_NEXT_OPERATION_YES_NO_STOP_BIT;
+   } 
+   else if (reading.length == 2)
+   {
+       DEBUG_STREAM("NACK");
+       I2C{{ id }}->CR1 &= ~I2C_CR1_ACK;
    }
}

@@ -387,11 +388,11 @@ I2C{{ id }}_EV_IRQHandler(void)
    // EV8: TxE=1, shift register not empty, data register empty, cleared by writing DR
    if (writing.length > 0)
    {
        I2C{{ id }}->DR = *writing.buffer++; // write data
        --writing.length;

        checkNextOperation = CHECK_NEXT_OPERATION_NO_WAIT_FOR_BTF;
    }
@@ -399,7 +400,7 @@ I2C{{ id }}_EV_IRQHandler(void)
    if (writing.length == 0)
    {
        // disable TxE, and wait for EV8_2
        I2C{{ id }}->CR2 &= ~I2C_CR2_ITBUFEN;
    }
}
@@ -443,11 +444,6 @@ I2C{{ id }}_EV_IRQHandler(void)
        uint16_t dr = I2C{{ id }}->DR;
        *reading.buffer++ = dr & 0xff;

-       DEBUG_STREAM("waiting for stop");
-       uint_fast32_t deadlockPreventer = 100000;
-       while (I2C{{ id }}->CR1 & I2C_CR1_STOP && deadlockPreventer-- > 0)
-           ;
-
        DEBUG_STREAM("reading data2");
        dr = I2C{{ id }}->DR;
        *reading.buffer++ = dr & 0xff;

Hope I did not delete too much from the diff because my diff included the debug output truncating.

How can I make the diff so colourful as yours?

salkinium commented 8 years ago

How can I make the diff so colourful as yours?

Use diff as the language.

I'm trying to get this to work, let's see.

salkinium commented 8 years ago

(void) I2C{{ id }}->SR1; // Clear ADDR

That can't be right, that doesn't clear ADDR, it should be SR2. This will be a NOP.

strongly-typed commented 8 years ago

You are right. But I did not test 1 byte reads yet ;-)

Taken from here

strongly-typed commented 8 years ago

But consider

The interface waits for the ADDR flag to be set then cleared by reading the SR1 and SR2 status register.

from AN2824 page 5, 8, 9 and 10.

salkinium commented 8 years ago

Yes, sure, but SR1 is always read, it's the first instruction in the interrupt. So you only need to read SR2 at that time (also, there is no difference in my tests).

salkinium commented 8 years ago

So these minimal changes still work for me and don't break 1B transfers:

diff --git a/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in b/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in
index c4018fc..c714cf2 100644
--- a/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in
+++ b/src/xpcc/architecture/platform/driver/i2c/stm32/i2c_master.cpp.in
@@ -297,7 +297,7 @@ I2C{{ id }}_EV_IRQHandler(void)
                                reading = transaction->reading();
                                nextOperation = static_cast<xpcc::I2c::Operation>(reading.next);

-                               if (reading.length <= 2)
+                               if (reading.length < 2)
                                {
                                        DEBUG_STREAM("NACK");
                                        I2C{{ id }}->CR1 &= ~I2C_CR1_ACK;
@@ -377,6 +375,11 @@ I2C{{ id }}_EV_IRQHandler(void)
                        reading.length = 0;
                        checkNextOperation = CHECK_NEXT_OPERATION_YES_NO_STOP_BIT;
                }
+               else if (reading.length == 2)
+               {
+                       DEBUG_STREAM("NACK");
+                       I2C{{ id }}->CR1 &= ~I2C_CR1_ACK;
+               }
        }

strongly-typed commented 8 years ago

I can confirm correct reads with correct sequence of ACK and NACK for 1, 2 and 3 bytes with STM32F103 with a LA. Will test with STM32F4 now.

strongly-typed commented 8 years ago

No side-effects for STM32F407 discovered (yet). But a more elaborate test sequence must be put in place. I could to that parallel to my porting efforts of STM32L4.

salkinium commented 8 years ago

It works for me too on the STM32F407. Yay, I'll prepare a PR.

roboterclubaachen / xpcc

STM32F103 I2C: When reading exactly two bytes the first byte is not acknowledged by the I2C master. #127