sparkfun / SparkFun_Qwiic_OLED_Arduino_Library

Arduino Library for SparkFun's OLED Qwiic (I²C) boards
https://docs.sparkfun.com/SparkFun_Qwiic_OLED_Arduino_Library/introduction/
Other
6 stars 7 forks source link

Don't send I2C restart #6

Closed LeeLeahy2 closed 2 years ago

LeeLeahy2 commented 2 years ago

Subject of the issue

Corrupted display, specifically clear is not working. I have traced to the OLED controller (SSD1306) is not handling the I2C restart bit properly.

Your workbench

Steps to reproduce

  1. Build the RTK Surveyor firmware with version v1.0.4 of the SparkFun_Qwiic_OLED_Arduino_Library.
  2. Upload the firmware to the RTK Express

Expected behavior

The display should properly display the splash screen and following screens.

Actual behavior

The screen did not get cleared. Garbage is being displayed around the edges of the splash screen. Garbage is displayed around the edges of the main Rover screen.

The Change

With the restart bit, the OLED controller seems to ignore write data prior to the final restart bit. A few bits seem to be written and the rest of the display is left with its previous results. This causes the clear operation which uses the restart bit to fail to clear the display RAM. Other large writes also fail when the restart bit is sent.

This change removes the use of the I2C restart bit and sends a stop bit instead. The OLED controller (SSD1306) seems to be remembering where the data should be placed and continues the writing the next data segment to display RAM properly. As such, the clear operation is broken into sever I2C transfers each bounded by a start and stop bit. Transmitting the data segments with the start and stop bits causes the OLED controller to properly clear the display RAM.

Where is the Issue?

Limited testing indicates the is the OLED controller's (SSD1306) handling of the I2C restart bit. The GNSS library is using the I2C restart bit in communications with the GPS. Removing the use of the restart bit for that library, causes the GNSS library to fail all communications with the GPS. As such, the ESP32 must be sending the restart bit successfully and the GPS must be receiving it successfully and processing it accordingly.

What can break?

If the ESP32 is being used in a multi-master design, then not using the restart allows another bus master to potentially slip something between the two segments of the transfer. This would delay the transmission of the next segment of data. If two or more masters were writing to the OLED display then not using the restart opens up a window where another master could send a command to the SSD1306 and change the current column or page. This would cause the subsequent portion of data to be placed in the wrong location and possibly write past the end of display memory.

As long as a single master is on the I2C bus, I2C timing changes slightly due to the addition of the stop bit, but no other impact to the devices in the I2C bus should be seen.

nseidle commented 2 years ago

Thanks for the PR!

I can confirm this issue is only on the ESP32. This issue does not happen with any other platforms (tested on Uno and Apollo3 platforms).

I can confirm pixel trash appearing using the ESP32 v2.0.2 of the core. v2.0.1 also shows trash. v2.0.0 is clean. It's the ESP32 core, not this lib.

The ESP32 I2C driver has had a few issues. I was hoping @PaulZC PR #6537 would fix this but it does not.

Note to my future self: the screen can get itself into a 'clean' state. I had to hold the ESP32 in reset when power cycling the OLED to get pixel trash to appear.

I've added a platform gate for the ESP32. Merging.

PaulZC commented 2 years ago

PR #6537 is a fix for the ESP32 clock-stretching issue, not I2C restarts. The clock-stretching issue and restart issue are separate but did appear at the same time (on the u-blox GNSS library) when Espressif updated the ESP32 core.

The fixes we added to the GNSS library for the restart issue are these (plus some extra ones in pushRawData):

https://github.com/sparkfun/SparkFun_u-blox_GNSS_Arduino_Library/blob/7ab87aeac9df9bafb098575fb589f8910287acbe/src/SparkFun_u-blox_GNSS_Arduino_Library.cpp#L57-L63

https://github.com/sparkfun/SparkFun_u-blox_GNSS_Arduino_Library/blob/7ab87aeac9df9bafb098575fb589f8910287acbe/src/SparkFun_u-blox_GNSS_Arduino_Library.cpp#L4588

I think the changes in this PR do the same thing?

nseidle commented 2 years ago

Ah - indeed it is the same fix.