MCU slave receiver I2C shifts incoming data by one bit when replying with ACK

Question

I am communicating with I2C/TWI between a MCU (Atmega328p) as Slave receiver and the raspberry pi 5 as master transmitter. I2C is at 100kHz configuration (the slower the Rpi5 can go).

I have an issue when, if my MCU after it receives the transmitted byte and replies with ACK , it looks like it shifts the whole received byte by 1 bit to the left. Example: 0xFF (11111111) is sent, the MCU receives 0xFE (11111110).

If, after the MCU receives a byte, responds with NACK, the MCU reads the byte as normal, I do not face any issue using NACK. And I cannot understand why is that.

I looked at it on the oscilloscope. 0xFF is sent from the PI, and the two plots are:

When NACK returned from the MCU:

When ACK returned from the MCU:

Plots look fine and since this is the only issue with the I2C bus, makes me believe that the MCU's firmware is not correct.

Raspberry pi's software: (writes the data, shows error only when MCU replies with NACK, but data are send correctly as shown in the oscilloscope):

#include <stdio.h> #include <fcntl.h> #include <unistd.h> #include <sys/ioctl.h> #include <linux/i2c-dev.h> #include <stdint.h> int main() { printf("make file\n\r"); int file; printf("Make filename\n\r"); char filename[] = "/dev/i2c-1"; printf("Pick address\n\r"); int addr = 0x08; // Replace with your I2C device address uint8_t buffer[1]; buffer[0]=0xFF; printf("Open the I2C device file\n\r"); // Open the I2C device file if ((file = open(filename, O_RDWR)) < 0) { perror("Failed to open the I2C bus"); return 1; } printf("Set the I2C device address\n\r"); // Set the I2C device address if (ioctl(file, I2C_SLAVE, addr) < 0) { perror("Failed to acquire bus access and/or talk to slave"); return 1; } printf("Write data\n\r"); // Write data // Example: Writing a single byte if (write(file, buffer, 1) != 1) { perror("Failed to write to the I2C bus"); return 1; } return 0; }

MCU's I2C setup:

 BIT_SET(SREG, BIT(7));//Interrupt bit in SREG set //slave TWAR = 0B00010001;//Device’s Own Slave Address is 0x08 (0001000X), responds to general call(XXXXXX1) BIT_CLEAR(TWCR, BIT(TWIE));//I2C Interrupt disable BIT_CLEAR(TWCR, BIT(TWSTA));//Clear TWSTA to become slave BIT_CLEAR(TWCR, BIT(TWSTO));//Stop condition disabled BIT_SET(TWCR, BIT(TWEA));//acknowledge own address BIT_SET(TWCR, BIT(TWEN));//TWI Enable

MCU's Main Loop (the "sw" function is serialWrite to the COM port of the MCU):

 /*After its own slave address and the write bit have been received, the TWINT Flag is set and a valid status code can be read from TWSR*/ if (BIT_GET(TWCR, BIT(TWINT)))//if TWI interrupt flag is set { static uint8_t twStatus,receivedData; twStatus = TWSR;//read the status BIT_CLEAR(twStatus, BIT(0));//clear prescaler values BIT_CLEAR(twStatus, BIT(1)); sw("TWSR status code is ",1); swn(twStatus,16,0,1); //serialWrite the current status sw(":->",1); //serialWrite //Slave receiver if (twStatus == 0x60) { sw("Own SLA+W received, ACK returned\n\r",1); BIT_CLEAR(TWCR, BIT(TWSTA)); BIT_CLEAR(TWCR, BIT(TWSTO)); //Data byte will be received and ACK will be returned. BIT_SET(TWCR, BIT(TWEA)); } else if (twStatus == 0x80) { sw("Prev addressed with SLA+W, data received, ACK returned\n\r",1); BIT_CLEAR(TWCR, BIT(TWSTA)); BIT_CLEAR(TWCR, BIT(TWSTO)); //Data byte will be received and ACK will be returned. BIT_SET(TWCR, BIT(TWEA)); sw("Data: ",1); receivedData = TWDR; swn(receivedData,16,1,1); sw("\n\r",1); } else if (twStatus == 0x88) { sw("Prev addressed with own SLA+W, data received, ACK returned\n\r",1); //Switch to non addressed slave, Own SLA recognized. BIT_CLEAR(TWCR, BIT(TWSTA)); BIT_CLEAR(TWCR, BIT(TWSTO)); BIT_SET(TWCR, BIT(TWEA)); //sw("Data: ",1); receivedData = TWDR; swn(receivedData,16,1,1); sw("\n\r",1); } else if (twStatus == 0xA0) { sw("Stop or repeated start received while being slave.\n\r",1); //Switch to non addressed slave, Own SLA recognized. BIT_CLEAR(TWCR, BIT(TWSTA)); BIT_CLEAR(TWCR, BIT(TWSTO)); BIT_SET(TWCR, BIT(TWEA)); sw("Data: ",1); receivedData = TWDR; swn(receivedData,16,1,1); sw("\n\r",1); } /*clearing this flag starts the operation of the TWI, so all accesses to the TWI Address Register (TWAR), TWI Status Register (TWSR), and TWI Data Register (TWDR) must be complete before clearing this flag.*/ BIT_SET(TWCR, BIT(TWINT));//WRITE 1 to TWINT to clear it

And the serial output of the MCU is:

TWSR status code is 60:->Own SLA+W received, ACK returned

TWSR status code is 80:->Prev addressed with SLA+W, data received, ACK returned

Data: fe

Side note: Because my MCU uses 5V logic while the Pi uses 3.3V logic, I use LSF0102DCUR to convert the 5V to 3.3V and vise versa. I was probing the side where the RPI is looking at on the above plots. The MCU's side is the same (no jittering or any funny business on the bus so I thought its not worth mentioning, I probe and leave the MCU's side (using ACK) below:

Schematic/3D:

Layout (I tried to put all the components as close as possible):

Wiring:

@Justme done. I will take a better look at the level shifter and the bus signals then. — Christianidis Vasileios
– Christianidis Vasileios, Commented Jan 29, 2024 at 15:34
By the looks of it, you did not send either repeated start or stop bit immediately after the 2nd ACK/NACK. — kaosad
– kaosad, Commented Jan 29, 2024 at 15:48
@kaosad can raspberry pi send a Stop/start via software/code? It should be handled by the hardware. If you do know of a way to send the stop command after the ACK/NACK, let me know — Christianidis Vasileios
– Christianidis Vasileios, Commented Jan 29, 2024 at 16:06
By right the read()/write() functions should handle the stop bit for you. Looking at your CLK signal, it appears that the slave was holding it low thus preventing the master to continue clocking. — kaosad
– kaosad, Commented Jan 29, 2024 at 18:20
@Lundin The bit set and clear macros are used because it was the Atmel AVR notation and early GCC compiler did not generate an opcode to set or clear a single bit if you used C logical operators to and/or a single bit. That code is perfectly readable to any AVR C programmer, but maybe not to a generic C or ARM C programmer. Bit macro is also used because Atmel headers used bit positions 0..7, not bit masks 0x01.. 0x80, and TWSTA is a macro with value of 7 so if you do what you recommend no one can use default AVR-GCC/Atmel/Microchip headers any more. — Justme
– Justme, Commented Feb 8, 2024 at 15:03

Justme · Accepted Answer · 2024-02-08 14:58:41Z

Thanks to the other answer that pastes the data sheet, the problem is now clear why clearing or setting bits individually to TWCR register did not work.

The TWINT bit will be set to logic 1 when TWI hardware has finished the current job and is waiting for software response.

The TWINT bit is used to start the next operation by clearing the bit to logic 0, but it is cleared by writing a logic 1 to this bit in the TWCR register.

So when you tried to set or clear any of the other bits, the TWCR register sees a read-modify-write sequence.

And it means that since the TWINT bit is high when reading, and you did not modify it, it is written back as high, telling the TWI hardware to continue with the next operation.

But since not all control bits were properly set before TWI hardware was told to start, and control bits are set after during the operation, the outcome of the operation will be wrong.

Christianidis Vasileios · Accepted Answer · 2024-02-08 14:12:58Z

I solved it. The issue was on the MCU's firmware. I needed to set the whole TWCR register at once, not one bit at a time.

The datasheet of atmega328 states about TWINT flag in TWCR register:

Also note that clearing this flag starts the operation of the TWI, so all accesses to the TWI Address Register (TWAR), TWI Status Register (TWSR), and TWI Data Register (TWDR) must be complete before clearing this flag.

I find this misleading. It sounds like one can write the bits in TWCR register one by one and then clear the TWINT bit, but this is not the case.
In the datasheet also says:

After all TWI Register updates and other pending application software tasks have been completed, TWCR is written. When writing TWCR, the TWINT bit should be set. Writing a one to TWINT clears the flag. The TWI will then commence executing whatever operation was specified by the TWCR setting.

Indicating that TWCR must be written as a whole byte at once, OR, (as I understand from the datasheet's examples as well), one can clear the TWINT bit only, which is the correct way to set the TWCR register.

What one should NOT do, is write the bits in TWCR one by one, and then clear the TWINT flag. (And this was my mistake there).

So my code changes as show. I commented out the part(s) where I set the bits one by one in the TWCR register, and I set those bits in twcrMask variable instead. Then, I load this variable in the TWCR register at once:

/*After its own slave address and the write bit have been received, the TWINT Flag is set and a valid status code can be read from TWSR*/ if (BIT_GET(TWCR, BIT(TWINT)))//if TWI interrupt flag is set { static uint8_t twStatus,receivedData; //Add mask (or byte to be loaded to TWCR) here: static uint8_t twcrMask; twcrMask = 0B00000101 twStatus = TWSR;//read the status BIT_CLEAR(twStatus, BIT(0));//clear prescaler values BIT_CLEAR(twStatus, BIT(1)); sw("TWSR status code is ",1); swn(twStatus,16,0,1); //serialWrite the current status sw(":->",1); //serialWrite //Slave receiver if (twStatus == 0x60) { sw("Own SLA+W received, ACK returned\n\r",1); //BIT_CLEAR(TWCR, BIT(TWSTA)); //BIT_CLEAR(TWCR, BIT(TWSTO)); //Data byte will be received and ACK will be returned. //BIT_SET(TWCR, BIT(TWEA)); //Set the mask: BIT_CLEAR(twcrMask, BIT(5));//TWSTA BIT_CLEAR(twcrMask, BIT(4));//TWSTO BIT_SET(twcrMask, BIT(6));//TWEA BIT_SET(twcrMask, BIT(7));//TWINT } else if (twStatus == 0x80) { sw("Prev addressed with SLA+W, data received, ACK returned\n\r",1); //BIT_CLEAR(TWCR, BIT(TWSTA)); //BIT_CLEAR(TWCR, BIT(TWSTO)); //Data byte will be received and ACK will be returned. //BIT_SET(TWCR, BIT(TWEA)); //Set the mask: BIT_CLEAR(twcrMask, BIT(5));//TWSTA BIT_CLEAR(twcrMask, BIT(4));//TWSTO BIT_SET(twcrMask, BIT(6));//TWEA BIT_SET(twcrMask, BIT(7));//TWINT sw("Data: ",1); receivedData = TWDR; swn(receivedData,16,1,1); sw("\n\r",1); } else if (twStatus == 0x88) { sw("Prev addressed with own SLA+W, data received, ACK returned\n\r",1); //Switch to non addressed slave, Own SLA recognized. //BIT_CLEAR(TWCR, BIT(TWSTA)); //BIT_CLEAR(TWCR, BIT(TWSTO)); //BIT_SET(TWCR, BIT(TWEA)); //Set the mask: BIT_CLEAR(twcrMask, BIT(5));//TWSTA BIT_CLEAR(twcrMask, BIT(4));//TWSTO BIT_SET(twcrMask, BIT(6));//TWEA BIT_SET(twcrMask, BIT(7));//TWINT //sw("Data: ",1); receivedData = TWDR; swn(receivedData,16,1,1); sw("\n\r",1); } else if (twStatus == 0xA0) { sw("Stop or repeated start received while being slave.\n\r",1); //Switch to non addressed slave, Own SLA recognized. //BIT_CLEAR(TWCR, BIT(TWSTA)); //BIT_CLEAR(TWCR, BIT(TWSTO)); //BIT_SET(TWCR, BIT(TWEA)); //Set the mask: BIT_CLEAR(twcrMask, BIT(5));//TWSTA BIT_CLEAR(twcrMask, BIT(4));//TWSTO BIT_SET(twcrMask, BIT(6));//TWEA BIT_SET(twcrMask, BIT(7));//TWINT sw("Data: ",1); receivedData = TWDR; swn(receivedData,16,1,1); sw("\n\r",1); } /*clearing this flag starts the operation of the TWI, so all accesses to the TWI Address Register (TWAR), TWI Status Register (TWSR), and TWI Data Register (TWDR) must be complete before clearing this flag.*/ //Comment out the TWINT bit set here. //BIT_SET(TWCR, BIT(TWINT));//WRITE 1 to TWINT to clear it //Load the mask/Byte at once: TWCR = twcrMask;

""It sounds like one can write the bits in TWCR register one by one" Why would anyone do that, though. Register access should always be done as a single byte read or write. Realize that all registers are declared as volatile - access to them can't be optimized. So if you would set the bit one by one, you don't just change the meaning of the code but you also write to the register 8 times instead of one, which just slows down the program a lot for no good reason. — Lundin
– Lundin, Commented Feb 8, 2024 at 14:42
I think you can write the bits one at a time. The problem is that no matter which bit other than TWINT you accessed and which way you wanted to set it, the code performed a read-modify-write operation to the register, and TWINT will be read as '1', it will be also written back as '1', and TWINT is cleared by writing '1' to it so it started the operation even if you did not intend to. However, as said, I don't see why you would even want to write one bit at a time there, or even reading to a shadow register and flipping them individually before writing it back. Write directly what you need. — Justme
– Justme, Commented Feb 8, 2024 at 14:45
@ChristianidisVasilis Writing to a temporary variable in RAM like in your answer is common good practice btw. Gather everything there, then just write to the actual register with a single instruction. Check out this example with full optimization for AVR godbolt.org/z/Tq111edro. I made up some bogus register but it doesn't matter. The first access bit by bit directly to the register results the majority of the generated code, whereas the instructions to a temporary RAM variable gets optimized into just some load, modify, store. — Lundin
– Lundin, Commented Feb 8, 2024 at 14:59

Stack Exchange Network

MCU slave receiver I2C shifts incoming data by one bit when replying with ACK

2 Answers 2

Hot Network Questions

MCU slave receiver I2C shifts incoming data by one bit when replying with ACK

2 Answers 2

Related

Hot Network Questions