SuperHouse / esp-open-rtos

Open source FreeRTOS-based ESP8266 software framework
BSD 3-Clause "New" or "Revised" License
1.53k stars 491 forks source link

ssd1306: allow SPI3 to be disabled, compact device descriptor #631

Closed quietboil closed 6 years ago

quietboil commented 6 years ago

I added SPI4W protocol to the driver. It is a flavor of the SPI4 protocol with HW driven CS and no MISO (pin is reconfigured as GPIO and used for D/C). That's how my device is wired :smile: Hopefully others will like this option too as it saves a GPIO pin (or two, depends on the counting method :smile: ) if one does not mind wiring CS and D/C to the predefined pins.

There are two additional small changes:

The changed driver has been tested by compiling and running both ssd1306 examples - example and fps - on my device.

Zaltora commented 6 years ago

I am disagree with this pull request. The protocols correspond to the different types of driver communication. This is the default esp8266 spi configuration that is problematic here. But you can do the same when your program start without need change the library:

    while (ssd1306_init(&dev) != 0) {
        printf("%s: failed to init SSD1306 lcd\n", __func__);
        vTaskDelay(SECOND);
    }
    spi_init(SPI_BUS, SPI_MODE0, SPI_FREQ_DIV_8M, true, SPI_LITTLE_ENDIAN, false);
    gpio_set_iomux_function(12, IOMUX_GPIO12_FUNC_GPIO);

The good idea is too remove the SPI initialisation from the driver. disable CS use for some value (e.g: 0xFF).

quietboil commented 6 years ago

You are right. In the hindsight it is clear that the new protocol is an overkill as the workaround is quite simple.

What about the other changes though? The device descriptor carries quite a few never used bytes and at the moment the driver cannot be compiled without the SPI3 protocol.

Zaltora commented 6 years ago

Save Ram is ok ^^. We do not lose a lot of readability for screens and protocols selection. I do not understand why we can not compile without the SPI3 protocols. this line fix the problem ? :

+#if (SSD1306_SPI3_SUPPORT)    
     uint8_t j;
+#endif
quietboil commented 6 years ago

why we can not compile without the SPI3 protocols

component.mk does not define a C macro for the SPI3 in ssd1306_CFLAGS like it does for I2C and SPI4. This makes SSD1306_SPI3_SUPPORT undefined when config.h is looking for it, so it adds it as enabled. That's what the very first commit has addressed.

this line...

+#if (SSD1306_SPI3_SUPPORT)    
     uint8_t j;
+#endif

This change was added to address the consequence of being able to exclude SPI3. When the latter is not compiled, that j sticks out and compiler is complaining about the unused variable.

The alternative, IMHO, was to make SPI3's case a block and move it down there where it is actually used, but that to me felt contrary to the overall style of this driver and I did not want to take it that far.

quietboil commented 6 years ago

I decided to redo it piecemeal. After the protocol was taken out the remaining changes do not need to be joined at the hip.