SSD1306 driver no longer works for I2C displays

[jeremyherbert]

Describe the bug
I currently am unable to communicate with an SSD1306 device that works with an older version of Zephyr (approximately mid-2020). Unfortunately I no longer have the exact zephyr version for this.

I have examined the behaviour of the code running on an old device and the current zephyr version with a logic analyser using the exact same SSD1306 display (I moved it between devices). On the old device it works fine, but with the latest zephyr version it no longer works. I also tried several different SSD1306 displays and they all had the same issue.

To communicate with the SSD1306 displays over I2C, the datasheet says that one must send a control byte, and then a data byte, repeating this pair of bytes as necessary. See page 20 here: https://cdn-shop.adafruit.com/datasheets/SSD1306.pdf

In this commit: fd2087e the sending of control bytes appears to be deliberately removed except for the very first one, presumably to make the code work for SPI displays. However, this has broken i2c support in my case.

Also, the use of the i2c_burst_write function appears to insert a repeated start condition. This might still work but it is not mentioned in the datasheet as supported; it expects start and stop only.

If I revert all of the changes in that commit above, the display will then turn on, but all I see is noise.

I also need to adjust the code to replace the line here with a non-burst write:

zephyr/drivers/display/ssd1306.c

Line 241 in e18fcbb

return i2c_burst_write(driver->i2c,

as the burst write causes a repeated start to occur, and this confuses the controller. Perhaps in the past this was not the case?

Once I make those changes, the display works correctly as expected.

To Reproduce
prj.conf:

CONFIG_GPIO=y

CONFIG_HEAP_MEM_POOL_SIZE=16384
CONFIG_MAIN_STACK_SIZE=4096

CONFIG_DEBUG_OPTIMIZATIONS=y
CONFIG_ASSERT=y

CONFIG_I2C=y

CONFIG_DISPLAY=y
CONFIG_DISPLAY_LOG_LEVEL_DBG=y
CONFIG_LOG_STRDUP_BUF_COUNT=16

CONFIG_SSD1306=y
CONFIG_SSD1306_SH1106_COMPATIBLE=n

CONFIG_LOG=y

# Console
CONFIG_CONSOLE=y
CONFIG_UART_CONSOLE=n
CONFIG_STDOUT_CONSOLE=y
CONFIG_PRINTK=y
CONFIG_EARLY_CONSOLE=y
CONFIG_USE_SEGGER_RTT=y
CONFIG_RTT_CONSOLE=y

# CFB
CONFIG_CFB_LOG_LEVEL_DBG=y
CONFIG_CHARACTER_FRAMEBUFFER=y

display.overlay:

&i2c0 {
    compatible = "nordic,nrf-twim";

    status = "okay";
    clock-frequency = <400000>;

    ssd1306@3c {
        compatible = "solomon,ssd1306fb";
        reg = <0x3c>;
        label = "SSD1306";
        width = <128>;
        height = <64>;
        segment-offset = <0>;
        page-offset = <0>;
        display-offset = <0>;
        multiplex-ratio = <63>;
        segment-remap;
        com-invdir;
        prechargep = <0x22>;
    };
};

main.c:

#include <zephyr.h>
#include <device.h>
#include <drivers/display.h>
#include <display/cfb.h>
#include <stdio.h>

void main() {
    struct device *dev;
    uint16_t rows;
    uint8_t ppt;
    uint8_t font_width;
    uint8_t font_height;
    uint16_t display_height;

    dev = device_get_binding("SSD1306");

    if (dev == NULL) {
        printk("Device not found\n");
        return;
    }

    if (display_set_pixel_format(dev, PIXEL_FORMAT_MONO10) != 0) {
        printk("Failed to set required pixel format\n");
        return;
    }

    printk("initialized %s\n", "SSD1306");

    if (cfb_framebuffer_init(dev)) {
        printk("Framebuffer initialization failed!\n");
        return;
    }

    cfb_framebuffer_clear(dev, true);
    cfb_framebuffer_invert(dev);
    display_blanking_off(dev);


    rows = cfb_get_display_parameter(dev, CFB_DISPLAY_ROWS);
    ppt = cfb_get_display_parameter(dev, CFB_DISPLAY_PPT);

    for (int idx = 0; idx < 1; idx++) {
        if (cfb_get_font_size(dev, idx, &font_width, &font_height)) {
            break;
        }
        cfb_framebuffer_set_font(dev, idx);
        printk("font width %d, font height %d\n",
               font_width, font_height);
    }

    display_height = cfb_get_display_parameter(dev, CFB_DISPLAY_HEIGH);

    printk("x_res %d, y_res %d, ppt %d, rows %d, cols %d\n",
           cfb_get_display_parameter(dev, CFB_DISPLAY_WIDTH),
           display_height,
           ppt,
           rows,
           cfb_get_display_parameter(dev, CFB_DISPLAY_COLS));

    char tmpstr[50];
    uint16_t tick = 0;

    while (1) {
        cfb_framebuffer_clear(dev, false);
        sprintf(tmpstr, "%d", tick++);
        for (int i = 0; i < (display_height / font_height); i++) {
            if (cfb_print(dev,
                          tmpstr,
                          0, i * font_height)) {
                printk("Failed to print a string\n");
                continue;
            }
        }
        cfb_framebuffer_finalize(dev);
        k_sleep(K_MSEC(1000));
    }
}

Steps to reproduce the behavior (for adafruit_feather_nrf52840):

1. Add the files above to a blank project
2. build with DTC_OVERLAY_FILE=display.overlay BOARD=adafruit_feather_nrf52840 west build
3. Wire up a 128x64 SSD1306 display to the I2C bus on the board
4. flash with DTC_OVERLAY_FILE=display.overlay BOARD=adafruit_feather_nrf52840 west flash
5. See display does not work
6. Make adjustments in the diff at the end of this issue
7. Rebuild, reflash and see that display does work

Impact
Showstopper, the driver in zephyr is broken

Environment (please complete the following information):

• OS: Linux
• Toolchain: Zephyr SDK
• Commit SHA: 4ce908a

Additional context
Full diff of hacky fix, note that this will probably break SPI display support:

diff --git a/drivers/display/ssd1306.c b/drivers/display/ssd1306.c
index 08b447e3b7..3bab7ce2af 100644
--- a/drivers/display/ssd1306.c
+++ b/drivers/display/ssd1306.c
@@ -64,11 +64,13 @@ static inline int ssd1306_write_bus(const struct device *dev,
 				    uint8_t *buf, size_t len, bool command)
 {
 	struct ssd1306_data *driver = dev->data;
+	return i2c_write(driver->bus, buf, len, DT_INST_REG_ADDR(0));
 
-	return i2c_burst_write(driver->bus, DT_INST_REG_ADDR(0),
-			       command ? SSD1306_CONTROL_ALL_BYTES_CMD :
-					 SSD1306_CONTROL_ALL_BYTES_DATA,
-			       buf, len);
+
+//	return i2c_burst_write(driver->bus, DT_INST_REG_ADDR(0),
+//			       command ? SSD1306_CONTROL_ALL_BYTES_CMD :
+//					 SSD1306_CONTROL_ALL_BYTES_DATA,
+//			       buf, len);
 }
 
 #elif DT_INST_ON_BUS(0, spi)
@@ -99,60 +101,80 @@ static inline int ssd1306_write_bus(const struct device *dev,
 
 static inline int ssd1306_set_panel_orientation(const struct device *dev)
 {
-	uint8_t cmd_buf[] = {
-		(SSD1306_PANEL_SEGMENT_REMAP ?
-		 SSD1306_SET_SEGMENT_MAP_REMAPED :
-		 SSD1306_SET_SEGMENT_MAP_NORMAL),
-		(SSD1306_PANEL_COM_INVDIR ?
-		 SSD1306_SET_COM_OUTPUT_SCAN_FLIPPED :
-		 SSD1306_SET_COM_OUTPUT_SCAN_NORMAL)
-	};
+    uint8_t cmd_buf[] = {
+            SSD1306_CONTROL_BYTE_CMD,
+            (SSD1306_PANEL_SEGMENT_REMAP ?
+             SSD1306_SET_SEGMENT_MAP_REMAPED :
+             SSD1306_SET_SEGMENT_MAP_NORMAL),
+            SSD1306_CONTROL_LAST_BYTE_CMD,
+            (SSD1306_PANEL_COM_INVDIR ?
+             SSD1306_SET_COM_OUTPUT_SCAN_FLIPPED :
+             SSD1306_SET_COM_OUTPUT_SCAN_NORMAL)
+    };
 
 	return ssd1306_write_bus(dev, cmd_buf, sizeof(cmd_buf), true);
 }
 
 static inline int ssd1306_set_timing_setting(const struct device *dev)
 {
-	uint8_t cmd_buf[] = {
-		SSD1306_SET_CLOCK_DIV_RATIO,
-		(SSD1306_CLOCK_FREQUENCY << 4) | SSD1306_CLOCK_DIV_RATIO,
-		SSD1306_SET_CHARGE_PERIOD,
-		DT_INST_PROP(0, prechargep),
-		SSD1306_SET_VCOM_DESELECT_LEVEL,
-		SSD1306_PANEL_VCOM_DESEL_LEVEL
-	};
+    uint8_t cmd_buf[] = {
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_CLOCK_DIV_RATIO,
+            SSD1306_CONTROL_BYTE_CMD,
+            (SSD1306_CLOCK_FREQUENCY << 4) | SSD1306_CLOCK_DIV_RATIO,
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_CHARGE_PERIOD,
+            SSD1306_CONTROL_BYTE_CMD,
+            DT_INST_PROP(0, prechargep),
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_VCOM_DESELECT_LEVEL,
+            SSD1306_CONTROL_LAST_BYTE_CMD,
+            SSD1306_PANEL_VCOM_DESEL_LEVEL
+    };
 
 	return ssd1306_write_bus(dev, cmd_buf, sizeof(cmd_buf), true);
 }
 
 static inline int ssd1306_set_hardware_config(const struct device *dev)
 {
-	uint8_t cmd_buf[] = {
-		SSD1306_SET_START_LINE,
-		SSD1306_SET_DISPLAY_OFFSET,
-		DT_INST_PROP(0, display_offset),
-		SSD1306_SET_PADS_HW_CONFIG,
-		SSD1306_COM_PINS_HW_CONFIG,
-		SSD1306_SET_MULTIPLEX_RATIO,
-		DT_INST_PROP(0, multiplex_ratio)
-	};
+    uint8_t cmd_buf[] = {
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_START_LINE,
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_DISPLAY_OFFSET,
+            SSD1306_CONTROL_BYTE_CMD,
+            DT_INST_PROP(0, display_offset),
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_PADS_HW_CONFIG,
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_COM_PINS_HW_CONFIG,
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_MULTIPLEX_RATIO,
+            SSD1306_CONTROL_LAST_BYTE_CMD,
+            DT_INST_PROP(0, multiplex_ratio)
+    };
 
 	return ssd1306_write_bus(dev, cmd_buf, sizeof(cmd_buf), true);
 }
 
 static inline int ssd1306_set_charge_pump(const struct device *dev)
 {
-	uint8_t cmd_buf[] = {
+    uint8_t cmd_buf[] = {
 #if defined(CONFIG_SSD1306_DEFAULT)
+            SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_SET_CHARGE_PUMP_ON,
+		SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_SET_CHARGE_PUMP_ON_ENABLED,
 #endif
 #if defined(CONFIG_SSD1306_SH1106_COMPATIBLE)
+            SSD1306_CONTROL_BYTE_CMD,
 		SH1106_SET_DCDC_MODE,
+		SSD1306_CONTROL_BYTE_CMD,
 		SH1106_SET_DCDC_ENABLED,
 #endif
-		SSD1306_PANEL_PUMP_VOLTAGE,
-	};
+            SSD1306_CONTROL_LAST_BYTE_CMD,
+            SSD1306_PANEL_PUMP_VOLTAGE,
+    };
 
 	return ssd1306_write_bus(dev, cmd_buf, sizeof(cmd_buf), true);
 }
@@ -160,6 +182,7 @@ static inline int ssd1306_set_charge_pump(const struct device *dev)
 static int ssd1306_resume(const struct device *dev)
 {
 	uint8_t cmd_buf[] = {
+        SSD1306_CONTROL_LAST_BYTE_CMD,
 		SSD1306_DISPLAY_ON,
 	};
 
@@ -169,12 +192,15 @@ static int ssd1306_resume(const struct device *dev)
 static int ssd1306_suspend(const struct device *dev)
 {
 	uint8_t cmd_buf[] = {
+        SSD1306_CONTROL_LAST_BYTE_CMD,
 		SSD1306_DISPLAY_OFF,
 	};
 
 	return ssd1306_write_bus(dev, cmd_buf, sizeof(cmd_buf), true);
 }
 
+static uint8_t tmp[2048];
+
 static int ssd1306_write(const struct device *dev, const uint16_t x, const uint16_t y,
 			 const struct display_buffer_descriptor *desc,
 			 const void *buf)
@@ -206,14 +232,22 @@ static int ssd1306_write(const struct device *dev, const uint16_t x, const uint1
 		x, y, desc->pitch, desc->width, desc->height, buf_len);
 
 #if defined(CONFIG_SSD1306_DEFAULT)
-	uint8_t cmd_buf[] = {
+    uint8_t cmd_buf[] = {
+		SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_SET_MEM_ADDRESSING_MODE,
+		SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_ADDRESSING_MODE,
+		SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_SET_COLUMN_ADDRESS,
+		SSD1306_CONTROL_BYTE_CMD,
 		x,
+		SSD1306_CONTROL_BYTE_CMD,
 		(x + desc->width - 1),
+		SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_SET_PAGE_ADDRESS,
+		SSD1306_CONTROL_BYTE_CMD,
 		y/8,
+		SSD1306_CONTROL_LAST_BYTE_CMD,
 		((y + desc->height)/8 - 1)
 	};
 
@@ -222,15 +256,24 @@ static int ssd1306_write(const struct device *dev, const uint16_t x, const uint1
 		return -1;
 	}
 
-	return ssd1306_write_bus(dev, (uint8_t *)buf, buf_len, false);
+	struct ssd1306_data *driver = dev->data;
+
+	tmp[0] = SSD1306_CONTROL_LAST_BYTE_DATA;
+	memcpy(tmp + 1, buf, buf_len);
+
+	return i2c_write(driver->bus,
+			       (uint8_t *)tmp, buf_len+1, DT_INST_REG_ADDR(0));
 
 #elif defined(CONFIG_SSD1306_SH1106_COMPATIBLE)
 	uint8_t x_offset = x + DT_INST_PROP(0, segment_offset);
 	uint8_t cmd_buf[] = {
+	    SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_SET_LOWER_COL_ADDRESS |
 			(x_offset & SSD1306_SET_LOWER_COL_ADDRESS_MASK),
+        SSD1306_CONTROL_BYTE_CMD,
 		SSD1306_SET_HIGHER_COL_ADDRESS |
 			((x_offset >> 4) & SSD1306_SET_LOWER_COL_ADDRESS_MASK),
+        SSD1306_CONTROL_LAST_BYTE_CMD,
 		SSD1306_SET_PAGE_START_ADDRESS | (y / 8)
 	};
 	uint8_t *buf_ptr = (uint8_t *)buf;
@@ -244,7 +287,11 @@ static int ssd1306_write(const struct device *dev, const uint16_t x, const uint1
 			return -1;
 		}
 
-		if (ssd1306_write_bus(dev, buf_ptr, desc->width, false)) {
+		struct ssd1306_data *driver = dev->data;
+		if (i2c_burst_write(driver->bus,
+				    DT_INST_REG_ADDR(0),
+				    SSD1306_CONTROL_LAST_BYTE_DATA,
+				    buf_ptr, desc->width)) {
 			return -1;
 		}
 
@@ -283,10 +330,12 @@ static int ssd1306_set_brightness(const struct device *dev,
 
 static int ssd1306_set_contrast(const struct device *dev, const uint8_t contrast)
 {
-	uint8_t cmd_buf[] = {
-		SSD1306_SET_CONTRAST_CTRL,
-		contrast,
-	};
+    uint8_t cmd_buf[] = {
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_CONTRAST_CTRL,
+            SSD1306_CONTROL_LAST_BYTE_CMD,
+            contrast,
+    };
 
 	return ssd1306_write_bus(dev, cmd_buf, sizeof(cmd_buf), true);
 }
@@ -322,14 +371,16 @@ static int ssd1306_set_pixel_format(const struct device *dev,
 
 static int ssd1306_init_device(const struct device *dev)
 {
-	uint8_t cmd_buf[] = {
-		SSD1306_SET_ENTIRE_DISPLAY_OFF,
+    uint8_t cmd_buf[] = {
+            SSD1306_CONTROL_BYTE_CMD,
+            SSD1306_SET_ENTIRE_DISPLAY_OFF,
+            SSD1306_CONTROL_LAST_BYTE_CMD,
 #ifdef CONFIG_SSD1306_REVERSE_MODE
-		SSD1306_SET_REVERSE_DISPLAY,
+            SSD1306_SET_REVERSE_DISPLAY,
 #else
-		SSD1306_SET_NORMAL_DISPLAY,
+            SSD1306_SET_NORMAL_DISPLAY,
 #endif
-	};
+    };
 
 #if DT_INST_NODE_HAS_PROP(0, reset_gpios)
 	struct ssd1306_data *driver = dev->data;
diff --git a/drivers/display/ssd1306_regs.h b/drivers/display/ssd1306_regs.h
index 2c4f628376..edeea1f5eb 100644
--- a/drivers/display/ssd1306_regs.h
+++ b/drivers/display/ssd1306_regs.h
@@ -12,6 +12,10 @@
 #define SSD1306_CONTROL_ALL_BYTES_CMD		0x00
 /* All following bytes will contain data */
 #define SSD1306_CONTROL_ALL_BYTES_DATA		0x40
+
+#define SSD1306_CONTROL_LAST_BYTE_CMD		0x00
+#define SSD1306_CONTROL_LAST_BYTE_DATA		0x40
+
 /* The next byte will contain a command */
 #define SSD1306_CONTROL_BYTE_CMD		0x80
 /* The next byte will contain data */

[jeremyherbert]

Further digging implicates the i2c_burst_write function, the following change is all that is required to fix the issue. So it is the repeated start condition that is causing the problem; perhaps in the past for the nrf52840, i2c_burst_write did not generate a repeated start, and the regression is there?

diff --git a/drivers/display/ssd1306.c b/drivers/display/ssd1306.c
index 08b447e3b7..17dfa7d9c1 100644
--- a/drivers/display/ssd1306.c
+++ b/drivers/display/ssd1306.c
@@ -60,15 +60,16 @@ struct ssd1306_data {
 };
 
 #if DT_INST_ON_BUS(0, i2c)
+static uint8_t tmp_cmd[2048];
 static inline int ssd1306_write_bus(const struct device *dev,
                                    uint8_t *buf, size_t len, bool command)
 {
        struct ssd1306_data *driver = dev->data;
+       memcpy(tmp_cmd+1, buf, len);
+       tmp_cmd[0] = command ? SSD1306_CONTROL_ALL_BYTES_CMD :
+                                        SSD1306_CONTROL_ALL_BYTES_DATA;
 
-       return i2c_burst_write(driver->bus, DT_INST_REG_ADDR(0),
-                              command ? SSD1306_CONTROL_ALL_BYTES_CMD :
-                                        SSD1306_CONTROL_ALL_BYTES_DATA,
-                              buf, len);
+       return i2c_write(driver->bus, tmp_cmd, len+1, DT_INST_REG_ADDR(0));
 }
 
 #elif DT_INST_ON_BUS(0, spi)

[jeremyherbert]

It appears that this problem has been encountered before on the Nordic devices: e6fe542

[jfischer-no]

There are display samples, samples/subsys/display, and different SSD1306 compatible shields, boards/shields/ssd1306. It would be more than appropriate to use these for reporting bugs, especially with an I2C display controller.

for compatible = "nordic,nrf-twim"; you need to add "zephyr,concat-buf-size " to your devicetree overlay:

&arduino_i2c {                                                                                
        status = "okay";                                                                      
        zephyr,concat-buf-size = <2048>;                                                      
                                                                                              
        ssd1306@3c {                                                                          
                compatible = "solomon,ssd1306fb";                                             
                reg = <0x3c>;                                                                 
                label = "SSD1306";                                                            
                width = <128>;                                                                
                height = <64>;                                                                
                segment-offset = <0>;                                                         
                page-offset = <0>;                                                            
                display-offset = <0>;                                                         
                multiplex-ratio = <63>;                                                       
                segment-remap;                                                                
                com-invdir;                                                                   
                prechargep = <0x22>;                                                          
        };                                                                                    
}; 

But I think it is better to use compatible = "nordic,nrf-twi";. Anyway SSD1306 controller driver works fine:

[jeremyherbert]

Interesting, thank you. I have actually run those samples, but that specific line is not present in any of them, hence why I figured this was a bug. I guess it is just an implementation specific thing, but perhaps there could be a note added to the examples for us newbies to indicate that some I2C drivers may need it in order to suppress the repeated start condition?

[jfischer-no]

The samples mentioned above are generic, IMO nothing I2C controller specific should be there. "zephyr,concat-buf-size" is the property of nRF TWIM controller, not display controller.

[jeremyherbert]

Fair enough. I appreciate your help and insight on this.