Calling setBrightness() reverses invertDisplay()

[jasonacox]

Describe the bug

The setContrast() and setBrightness() functions essentially always call normalDisplay(). This means that if you are using invertDisplay() (e.g. black on white background), a call to adjust contrast or brightness will reverse the display (back to white on black background). You can see the sendCommand(NORMALDISPLAY) as part of the contrast function here:

esp8266-oled-ssd1306/src/OLEDDisplay.cpp

Lines 771 to 790 in 70921e5

	void OLEDDisplay::invertDisplay(void) {
	sendCommand(INVERTDISPLAY);
	}
	void OLEDDisplay::normalDisplay(void) {
	sendCommand(NORMALDISPLAY);
	}
	void OLEDDisplay::setContrast(uint8_t contrast, uint8_t precharge, uint8_t comdetect) {
	sendCommand(SETPRECHARGE); //0xD9
	sendCommand(precharge); //0xF1 default, to lower the contrast, put 1-1F
	sendCommand(SETCONTRAST);
	sendCommand(contrast); // 0-255
	sendCommand(SETVCOMDETECT); //0xDB, (additionally needed to lower the contrast)
	sendCommand(comdetect); //0x40 default, to lower the contrast, put 0
	sendCommand(DISPLAYALLON_RESUME);
	sendCommand(NORMALDISPLAY);
	sendCommand(DISPLAYON);
	}

To Reproduce

This generally works but you will see a flicker as it toggles between normal and inverted mode, especially if this is part of a loop:

#include "SSD1306Wire.h"

SSD1306Wire display(0x3c, D3, D4);

display.setBrightness(100);
display.invertDisplay();

Expected behavior

The expectation is that the setBrightness() call would adjust the inverted Display's brightness level rather than reverting it to normal display.

Workaround

Bypassing the private methods, you can reproduce setContrast() without the call to set NORMALDISPLAY:

#define private public
#include "SSD1306Wire.h"
#undef private

int brightness = 30;
display.sendCommand(SETPRECHARGE);  //0xD9
display.sendCommand(241);           //0xF1 default, to lower the contrast, put 1-1F
display.sendCommand(SETCONTRAST);
display.sendCommand(brightness * 1.171);     // 0-255
display.sendCommand(SETVCOMDETECT);  //0xDB, (additionally needed to lower the contrast)
display.sendCommand(brightness / 8);         //0x40 default, to lower the contrast, put 0
display.sendCommand(DISPLAYALLON_RESUME);
// sendCommand(NORMALDISPLAY);
display.sendCommand(DISPLAYON);

Proposed Solution

I would submit a PR to remove the sendCommand(NORMALDISPLAY) but while an edge case, it could be a breaking change for anyone how coded their projects to use setBrightness() or setContrast() to switch the display to "normal" mode.

I'll submit a PR for an alternative approach to preserve the API by adding a boolean true defaulted parameter to setContrast() and setBrightness() so that a person could override the call to NORMALDISPLAY - e.g.:

class OLEDDisplay {
public:
  void setContrast(
    uint8_t contrast,
    uint8_t precharge,
    uint8_t comdetect,
    bool normalDisplay = true   //  optional, defaults to true
  );
};

void OLEDDisplay::setContrast(
  uint8_t contrast,
  uint8_t precharge,
  uint8_t comdetect,
  bool normalDisplay
) {
  sendCommand(SETPRECHARGE);      // 0xD9
  sendCommand(precharge);         // 0xF1 default, to lower contrast put 0x1–0x1F
  sendCommand(SETCONTRAST);
  sendCommand(contrast);          // 0–255
  sendCommand(SETVCOMDETECT);     // 0xDB
  sendCommand(comdetect);         // 0x40 default
  sendCommand(DISPLAYALLON_RESUME);
  if (normalDisplay) {
    sendCommand(NORMALDISPLAY);
  }
  sendCommand(DISPLAYON);
}

[marcelstoer]

The expectation is that the setBrightness() call would adjust the inverted Display's brightness level rather than reverting it to normal display.

I fully agree. The display going back into normal mode feels like an unintended side-effect to me.

it could be a breaking change for anyone how coded their projects to use setBrightness() or setContrast() to switch the display to "normal" mode.

It feels like fixing a bug to me. setBrightness() should set the brightness and nothing else.

[jasonacox]

I agree @marcelstoer and if this were a new library I would recommend just removing that sendCommand(NORMALDISPLAY); command as the fix. However, I respect that this 6+ year old library has been deployed and integrated in many systems. My PR provides a solution without breaking the API contract. Not as clean as redefining the API, but again, honors the existing (suboptimal) behavior with an API extension to work around it.

If the maintainers are willing to update the API, I'll adjust my PR... 😀

[marcelstoer]

I discussed this with the other maintainer. We're on the same page here.

sendCommand(NORMALDISPLAY) being part of setting brightness or contrast is a bug to us. Removing that call doesn't break any API. Yes, the behavior will change but the API remains as-is.

[jasonacox]

Excellent! I'll update the PR.