Proof of concept for configurable OLED display support (#166)

[recursinging]

Hi,

this is a suggestion for adding support for configurable OLED displays to libDaisy.

In advance, I'm a Java dev by profession, and not at all versed in the best practices of C++, so please take this merely as a best attempt.

My primary motivation to get configurable support for OLED displays is my kxmx_bluemchen Eurorack module, which makes use of a 64x32 SSD1306 OLED via I2C. This obviously doesn't work without modifying libDaisy, so I thought I'd try my hand at a contribution.

As suggested by @TheSlowGrowth in #166, I've used templates to provide configurable specialization to the OledDisplay Class. After experimenting with this idea, I determined that an additional abstraction might be useful, as the drawing functions of the OledDisplay class, are not really specific to the SSD1306/SSD1309. Since there are a other monochrome OLED driver chips, such as the SH1106, I found it prudent to provide both a driver and a transport abstraction. The result is a more complicated, but more flexible initialization process. For example the Daisy Patch:

// Declaration (daisy_patch.h)
OledDisplay<SSD130xDriver<SSD130xSPITransport>> display;

// Transport config
dsy_gpio_pin pincfg[SSD130xSPITransport::NUM_PINS];
pincfg[SSD130xSPITransport::DATA_COMMAND] = seed.GetPin(PIN_OLED_DC);
pincfg[SSD130xSPITransport::RESET]  = seed.GetPin(PIN_OLED_RESET);
SSD130xSPITransport display_transport;
display_transport.Init(pincfg);

// Driver config
SSD130xDriver<SSD130xSPITransport> display_driver;
display_driver.Init(display_transport);

// Display config
display.Init(display_driver);

...I have a tendency to over-engineer things, which might be the case here, but I find the flexibility appealing. In the case of the kxmx_bluemchen, the initialization routine now looks like this:

// Declaration (kxmx_bluemchen.h)
OledDisplay<SSD130xDriver<SSD130xI2CTransport>> display; 

// Transport config
I2CHandle::Config i2c_config;
i2c_config.periph = I2CHandle::Config::Peripheral::I2C_1;
i2c_config.speed = I2CHandle::Config::Speed::I2C_1MHZ;
i2c_config.pin_config.scl = seed.GetPin(PIN_I2C_SCL);
i2c_config.pin_config.sda = seed.GetPin(PIN_I2C_SDA);
SSD130xI2CTransport display_transport;
display_transport.Init(i2c_config);

// Driver config
SSD130xDriver<SSD130xI2CTransport> display_driver;
display_driver.Init(display_transport, SSD130xDriver<SSD130xI2CTransport>::PixelDimensions::W64_H32);

// Display config
display.Init(display_driver); 

There is one aspect of this design with which I am not satisfied, and that is the display buffer. Because the template does not communicate any dimension information, there is no way (that I am aware of) to statically allocate a display buffer sized to the display dimensions. In this PR, I've simply allocated the max possible (128x64) space to the buffer, which is partially unused if the display is smaller, as is the case for my kxmx_bluemchen module. Perhaps there is another way to realize this?

The pattern used here also lends itself for future implementations of specialized drawing classes like ColorOledDisplay<Driver<Transport>>, GrayscaleOledDisplay<Driver<Transport>> LcdTftDisplay<Driver<Transport>>, LcdGraphicDisplay<Driver<Transport>> or LcdCharacterDisplay<Driver<Transport>>.

Please advise if this idea has legs.

Sam

[stephenhensley]

Just glancing over everything quickly. This looks great.

It'll take me a few days to get to a full review, but here are just a few things I've noticed briefly looking it over:

• We should probably set up the Transport Initializer with a similar Config struct as many of the peripherals have been getting lately. This provides a non-breaking way to introduce configuration down the road if necessary, and we can organize the pins a bit more nicely. (See i2c, or SAI for examples) of this.
• For handling various size displays we could have a templatized DisplayBuffer<size_t width, size_t height> struct that could be passed in along with the config struct. I could see us wanting to have this in the DMA-friendly region of memory so that we can freely use the DMA for display transactions.
• Similar in some ways to both above points, the PixelDimensions could be determined from whatever DisplayBuffer is passed in at initialization. Depending on if there's any other potential configuration we may want to add a Config struct for the SSD130xDriver, but I'm drawing a blank on anything else that would be configurable for the device itself.

Overall, this looks pretty solid already. I'll be able to test with the existing SPI display pretty quickly, but I'll have to see about getting one hooked up on I2C. So if you've already validated that, that'd be good to know as well.

Thanks a ton for the contribution!

[recursinging]

Thanks for the feedback, I'll have a look at the points you mention later this week. The implementation in its current form has been tested on three (simultaneous) SSD1306 OLEDs - I2C_1, I2C_4 and SPI:

Everything works fine and the refresh rates are good, except a 2px strip of junk on the right of the 128x64 display... I have no idea what is causing that, I'm looking into it.

[stephenhensley]

Oh that's excellent weird about the noise in the last two columns of pixels on that bigger display. If that's the one you have connected with SPI I'll be able to verify that pretty quickly (that looks identical to the displays I have here).

Looking forward to your thoughts on the other point. No rush, I probably won't have time to do any testing/verification on my end until Monday or so anyway.

[TheSlowGrowth]

@recursinging Good job, this looks very good to me! Your approach is pretty much exactly what I had in mind. My time budget didn't allow me to tackle such a large task lately, but it seems like I can cross this off my todo list because you did an amazing job here!
I'm slowly working on a eurorack module that I hope to be able to make open source eventually. I'll try to integrate your display driver into my current code and see how it goes.

Thanks for pushing this forward, this is great to see!

[TheSlowGrowth]

I tried it out - it was pretty much plug and play. I'm using the SPI transport with a SSD1306 0.96" display. I'll post a video in slack later.

Good job!

[recursinging]

@TheSlowGrowth - glad to hear this PR worked well for you. Out of curiosity, is your 0.96" display also 124x68? In your video, I didn't see the same 2px strip of noise on the right side that I've been getting on mine. It might just be a hardware issue on my end.

So I made some changes based on suggestions from @stephenhensley. The result ist relatively straightforward, even if a little verbose. Usage examples now look like this for SPI:

// Declaration (daisy_patch.h)
OledDisplay<SSD130xDriver<128, 64, SSD130x4WireSpiTransport>> display; 

// Transport config
SSD130x4WireSpiTransport display_transport;
SSD130x4WireSpiTransport::Config display_transport_config;
display_transport_config.pin_config.dc = seed.GetPin(PIN_OLED_DC);
display_transport_config.pin_config.reset = seed.GetPin(PIN_OLED_RESET);
display_transport.Init(display_transport_config);

// Driver config
SSD130xDriver<128, 64, SSD130x4WireSpiTransport> display_driver;
SSD130xDriver<128, 64, SSD130x4WireSpiTransport>::Config display_driver_config; 
display_driver_config.transport = display_transport;
display_driver.Init(display_driver_config);

// Display config
OledDisplay<SSD130xDriver<128, 64, SSD130x4WireSpiTransport>>::Config display_config;
display_config.driver = display_driver;
display.Init(display_config);

...and for I2C:

// Declaration (kxmx_bluemchen.h)
OledDisplay<SSD130xDriver<64, 32, SSD130xI2CTransport>> display;

// Transport config using "Defaults()"
SSD130xI2CTransport::Config display_transport_config;
display_transport_config.Defaults();
SSD130xI2CTransport display_transport;
display_transport.Init(display_transport_config);

// Driver config
SSD130xDriver<64, 32, SSD130xI2CTransport> display_driver;
SSD130xDriver<64, 32, SSD130xI2CTransport>::Config display_driver_config; 
display_driver_config.transport = display_transport;
display_driver.Init(display_driver_config);

// Display config
OledDisplay<SSD130xDriver<64, 32, SSD130xI2CTransport>>::Config display_config;
display_config.driver = display_driver;
display.Init(display_config);

I've decided against a general DisplayBuffer<size_t width, size_t height> struct simply because it is an implementation detail specific to this driver. Looking to other types of displays, particularly color TFTs, the drivers often have their own pixel addressable display buffers. I chose instead to include the dimensions in the SSD130xDriver template, but I'm not all too convinced that this is the best solution, as it allows arbitrary (and invalid) dimensions - a potential foot-gun.

In terms of code ergonomics - a point where libDaisy really (really) excels in comparison to Arduino/Teensy land, and which I'd like to maintain - I'm on the fence about this design, as it exposes a lot of abstractions. Not everyone feels at home in angle-bracket land as I do. I'd definitely be open to suggestions to make this a bit more beginner friendly.

@stephenhensley do you use a specific formatting tool, or definition for VSCode? Also, are there any specifics about code documentation you'd like me to take into account?

[stephenhensley]

@recursinging The init is definitely straightforward. Looks nice.

I agree it is a bit verbose, and I agree that a lot of people aren't very comfortable with templates. What we could do is typedef a few standard configurations and their transports. That doesn't necessarily simplify much, but it would make it look less confusing to those coming from C or other languages.

In terms of simplification I don't have a ton of ideas, but if we're not using a separate buffer type, and there isn't any inherent configuration to be done on the OledDisplay that couldn't be done on the SSD130xDriver then we could lose a layer of configuration. That would really only remove a few lines from the above, but it is something.

My reasoning behind wanting to provide the buffer externally was so that it could be located in the uncacheable SRAM if/when we want to add DMA transactions to the transport classes.

---

The tricky thing with libdaisy in general is being able to provide the high level functions that might be familiar, or even comfortable for beginners/newcomers, while still exposing a mid level hierarchy that can be used for maximum flexibility.

Right now with the breakout boards, I think we accomplish that quite well. It's just once you're working on a custom board, or the seed on a breadboard, I can see how things can become daunting pretty quickly.

I'm always open to ideas to making everything more accessible.

---

As for formatting, we use clang-format to do code styling. There is a .clang-format file in the repo. So if you have the "Clang-Format" extension by Xaver in VS Code, you can format while you work. Otherwise we have a fix_style.sh script in the base level of libdaisy that can be run to clean it up after-the-fact.

You can also verify that it will pass CI by using the following helper script:
./ci/run-clang-format.py -r src/

It'll return any issues, or nothing if there are no problems.

[recursinging]

Thanks for the feedback, I'll check out the formatting tools.

Pardon my ignorance, but why can the SSD130xDriver member buffer not be initialized to a DMA memory region? I know very little about it, and I'm certain it will become necessary to use DMA when implementing other drivers. I don't want to block that path.

[stephenhensley]

No ignorance to pardon, we really don't have any high-level documentation that covers the details of the internal memory use yet.

libDaisy enables the Data Cache within the processor to benefit from the added performance it gives. However, any memory section that is cached does not play well with the DMA without adding cache-maintenance functions (clean/invalidate). To simplify this, we have a chunk of memory in the D2 SRAM dedicated to buffers that need access to the DMA in the linker, and configured as non-cache memory by the MPU.

The current way to declare memory in this way is using the gcc attribute for memory sections. Since a class is a contiguous block of memory, individual members cannot be declared in a different section of memory.

So one could have the global object that contains the OledDisplay, or the OledDisplay itself declared in that memory section, but then all of the non-buffer data would lose any cache benefits as well. So having the buffer be a separate entity (whether a simple array, or a custom type like in the Pca9685LedDriver) that can be passed in at Init time would be a benefit that would allow that flexibility.

But again, that does make the init process a bit more complicated (similar to how the led driver is).

[recursinging]

Well now I understand. Thanks for the clear explanation. It's important to make sure access to DMA memory is available for this and other future display drivers. I'll mull it over and make some changes when I get a chance.

[stephenhensley]

Sounds good. I'll think it over as well. This seems like it's going to become a very common thing for device drivers for every peripheral.

[TheSlowGrowth]

I have a couple of points and suggestions for a simplification, especially for new users without template-experience.

Right now, I see three main difficulties for a new user:

1. Lifetime management: The various configs, transports, etc. have to be allocated alongside the actual display class and passed in - lifetime of these objects must "outlive" the display object. I can imagine new users not understanding this and passing temporary stack-allocated objects and wondering why things break.
2. Complexity and number of different types involved: We can't easily typdef-away the complexity because all the various classes need to be handled by the user directly.
3. Order of initialisation: The user must call the various Init()s of the various classes on the call-site, which could potentially happen in the wrong order or be confusing to the user.

Here's an idea how to resolve this:

The main goal for me is to have the driver as a member of the display class - and the transport as a member of the driver class. This resolves issue 1 (lifetime management) and 3 (init order).

To do this, give the display driver class a Config struct like this:

class DisplayDriver 
{
public: 
    // this struct is expected here - must be implemented in every DisplayDriver 
   struct Config 
   {
       // ...
   }
   // the init functions is expected to have this exact definition:
   // note: I'm not passing a const reference here to make clear that the Config struct
   // is just a temporary plain-old-data type and doesn't need to be held alive.
   void Init(Config config);
};

Then the display class could look like this:

template<typename DisplayDriverType>
class OledDisplay 
{
public:
    void Init(DisplayDriverType::Config driverConfig)
    {
        dirver_.Init(config);
    }
private:
    DisplayDriverType driver_;
};

Note that the Display OWNS the driver and driver-dependent configuration is entirely realised via the Config struct.

If the driver needs no specific transport abstraction, we're done already. Otherwise THE SAME idea can be used to configure the transport for the driver:

class DisplayTransport
{
public: 
    // this struct is expected here - must be implemented in every DisplayTransport
   struct Config 
   {
       // ...
   }
   // the init functions is expected to have this exact definition:
   // note: I'm not passing a const reference here to make clear that the Config struct
   // is just a temporary plain-old-data type and doesn't need to be held alive.
   void Init(Config config);
};

template<typename TransportType>
class DisplayDriver 
{
public: 
    // this struct is expected here - must be implemented in every DisplayDriver 
   struct Config 
   {
       // The transport config is placed in the driver Config
       TransportType::Config transportConfig;
       // ...
   }
   void Init(Config config)
    {
        transport_.Init(config.transportConfig);
        // ...
    }
private:
   TransportType transport_;
};

Now we can typedef-away all the complexity for a display like this (using the Daisy Field here as an example, not sure if that's actually correct but you get the idea).

using FieldOledDisplayTransportType = daisy::SSD130x4WireSpiTransport;
using FieldOledDisplayDriverType = daisy::SSD130xDriver<128, 64, FieldOledDisplayTransportType>;
using FieldOledDisplay = daisy::OledDisplay<FieldOledDisplayDriverType>;

Then all the user has to do is this:

FieldOledDisplay  display;

void main()
{
    FieldOledDisplay::config displayCfg;
    displayCfg.transportCfg.pin_config.dc = { DSY_GPIOC, 12 };
    displayCfg.transportCfg.pin_config.reset = { DSY_GPIOB, 4 };
    display.Init(displayCfg);
}

Now the complexity is hidden nicely and the ordering and lifetime problems are gone.

[stephenhensley]

@TheSlowGrowth ouu. This is well thought out, and definitely provides the user-level simplicity that I think we're aiming for.

Definitely a little bit of rework to get us there, but I like it. What do you think @recursinging? Also, if we need more hands on this specific feature, I can create an interim branch for us to work off.

[recursinging]

@stephenhensley - Agreed, the suggestions from @TheSlowGrowth reduced the user facing complexity (and potential foot-guns) considerably. I'd say it's about as intuitive as it can be, considering the abstractions built it, and code-completion of an IDE quickly aids in finding you way there.

I've pushed the changes suggested, but I haven't addressed the DMA issue yet. After looking over dev/leddriver.h, I feel it would be a huge win for code ergonomics If there were some way to initialize and address DMA memory withing the implementation. Not just in this case, but as @stephenhensley mentioned, generally for any device using a peripheral. After reading up a bit about the topic, I can't say I came up with any clever ideas. It's just way outside my core competence.

I think at this point it would be best to switch to an interim branch, as I'm not sure how much help I can be anymore. I do plan on someday trying to port a TFT driver (ILI9341 or ST7735) to this architecture, just to see how it fits. Unfortunately, the Kindergarten just closed again here, so I don't have a lot of spare time at the moment. I can gladly verify changes against my breadboard setup in the future.

[TheSlowGrowth]

@recursinging This looks great! I like that you type-def'd various typical configurations. I think this is probably the simplest solution we could possibly have while still supporting various transports and display types. I REALLY like that the transport is separated from the actual driver like this.

I'll give it a try later today.

Assuming that it works, IMO, this could be merged as-is.

[TheSlowGrowth]

I feel it would be a huge win for code ergonomics If there were some way to initialize and address DMA memory withing the implementation.

I agree, but the core problem is that all member variables of a class have to be in a continuous memory region. There IS a possible solution to this, but I'm not sure how it would work performance-wise: You can request the D-cache to manually flush / reload a region of memory. We could have the transmit buffers as members of the class and then flush them to memory right before we start the DMA transmission. Given that we would write to those buffers a lot, it might even improve performance compared to the existing solution:

If the buffers sit in the D-cache-disabled section of memory, then each access to the buffer is potentially slowed down compared to accessing some memory where the cache is enabled. This is probably not a problem for the led driver but it could be a problem for the display, where the buffer is much larger and drawing operations could cause a lot of accesses to the buffer.
If we, instead, would have the buffers as member variables of the OledDisplay class (or the driver behind it) then all drawing operations would benefit form the D-cache speed bump. We would only have to make sure we flush the cache contents to the SRAM before we start the DMA.

[recursinging]

We could have the transmit buffers as members of the class and then flush them to memory right before we start the DMA transmission.

I had the same though after I saw dsy_dma_clear_cache_for_buffer(uint8_t* buffer, size_t size). I have no idea what kind of performance implications this brings, nor how to profile it (can you verify a cache hit/miss by access time?).

Alternatively, I was wondering if it might be possible to have template peripheral handle classes which allocate their own transmit and receive buffers sized by the template type(s). The entire peripheral handle class instance could be placed in .sram1_bss without incurring a performance penalty (assuming these classes don't make use of the cache). It seems to be a more logical location, but I'm unsure how this would work with multiple devices on a single bus, so I dismissed the idea.

[TheSlowGrowth]

Let's discuss this in an issue and keep the display driver PR free of noise. #335

[stephenhensley]

This is fantastic. I also like the typedefs for typical display types. I agree that it can't really get a whole lot simpler.

I agree that pending some testing/verification this is good to merge as-is, and we can handle the concerns with DMA separately (since SPI still doesn't have DMA transfers set up yet anyway).

Really great work and thanks a ton for the contribution! I haven't had a chance to run this on my field or patch, but if you both have tested it I'll be okay with merging it as well. That said, I might be able to find sometime this afternoon to do a quick test on the field or something.

[stephenhensley]

Actually, I was able to flash up one of the examples (Quad Mixer) and drop it into the patch and field to make sure the display lights up, and doesn't have any noise, etc.

So I think I'm signing off here. If you guys have tried it and haven't found any other issues, or changes we should make, I think it's ready to merge.

[stephenhensley]

Alright. I'm taking the rocket-ship, and otherwise silence as a sign that neither of you have found any other issues.

Merging!

Thanks again, @recursinging for the excellent work, and @TheSlowGrowth for the feedback and testing. You both rock!