Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support SSD1306 128x32 (IDFGH-11296) #12450

Closed
wants to merge 1 commit into from

Conversation

storoj
Copy link
Contributor

@storoj storoj commented Oct 23, 2023

SSD1306 may have different sizes and resolutions: 128x32, 128x64, 96x16
and others.

SSD1306_CMD_SET_MULTIPLEX (0xA8) initialization command effectively
sets the height of the display's resolution. The value for this command
is the height in pixels minus one.

Set MUX ratio to N+1 MUX
N=A[5:0] : from 16MUX to 64MUX, RESET= 111111b (i.e. 63d, 64MUX)
A[5:0] from 0 to 14 are invalid entry.

SSD1306_CMD_SET_COMPINS (0xDA) configures the communication pins.
According to the datasheet it should be 0x2 with other optional bits
set.

A[4]=0b, Sequential COM pin configuration
A[4]=1b(RESET), Alternative COM pin configuration

A[5]=0b(RESET), Disable COM Left/Right remap
A[5]=1b, Enable COM Left/Right remap

@github-actions
Copy link

github-actions bot commented Oct 23, 2023

Messages
📖 Good Job! All checks are passing!

👋 Welcome storoj, thank you for your first contribution to espressif/esp-idf project!

📘 Please check Contributions Guide for the contribution checklist, information regarding code and documentation style, testing and other topics.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for espressif/esp-idf project.

Pull request review and merge process you can expect

Espressif develops the ESP-IDF project in an internal repository (Gitlab). We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

  1. An internal issue has been created for the PR, we assign it to the relevant engineer
  2. They review the PR and either approve it or ask you for changes or clarifications
  3. Once the Github PR is approved, we synchronize it into our internal git repository
  4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing
    • At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
  5. If the change is approved and passes the tests it is merged into the master branch
  6. On next sync from the internal git repository merged change will appear in this public Github repository

Generated by 🚫 dangerJS against 8f75c4c

@espressif-bot espressif-bot added the Status: Opened Issue is new label Oct 23, 2023
@github-actions github-actions bot changed the title Support SSD1306 128x32 Support SSD1306 128x32 (IDFGH-11296) Oct 23, 2023
@storoj
Copy link
Contributor Author

storoj commented Oct 23, 2023

This change fixes the issue #9277.

@storoj
Copy link
Contributor Author

storoj commented Oct 23, 2023

Sorry, there's one more initialization command that's missing. I will update my PR shortly.

@storoj storoj force-pushed the ssd1306_128x32 branch 2 times, most recently from a1d7062 to 5a130d5 Compare October 23, 2023 14:33
SSD1306 may have different sizes and resolutions: 128x32, 128x64, 96x16
and others.

`SSD1306_CMD_SET_MULTIPLEX` (0xA8) initialization command effectively
sets the height of the display's resolution. The value for this command
is the height in pixels minus one.

```
Set MUX ratio to N+1 MUX
N=A[5:0] : from 16MUX to 64MUX, RESET= 111111b (i.e. 63d, 64MUX)
A[5:0] from 0 to 14 are invalid entry.
```

`SSD1306_CMD_SET_COMPINS` (0xDA) configures the communication pins.
According to the datasheet it should be `0x2` with other optional bits
set.

```
A[4]=0b, Sequential COM pin configuration
A[4]=1b(RESET), Alternative COM pin configuration

A[5]=0b(RESET), Disable COM Left/Right remap
A[5]=1b, Enable COM Left/Right remap
```

* https://github.com/adafruit/Adafruit_SSD1306;
* https://www.digikey.com/htmldatasheets/production/2047793/0/0/1/ssd1306.html
*
* esp_lcd_panel_ssd1306_config_t ssd1306_config = {
* .width = 128,
* .height = 32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this example usage is different from the driver code

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But compared to the

typedef struct {
   /**
    * @brief Multiplex ratio: (0..63)
    *
    * Display's height minus one.
    */
   uint8_t mux_ratio;

   /**
    * @brief Enables alternative COM pin configuration.
    *
    * When unset then Sequential COM pin configuration is used.
    */
   bool com_pin_alt;

   /**
    * @brief COM Left/Right remap.
    */
   bool com_lr_remap;
} esp_lcd_panel_ssd1306_config_t;

This example version looks more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suda-morris thanks for your comments, good catch on mismatched documentation comment. I had several iterations on the driver and forgot to update the docs.

First I wanted to provide simpler interface with just width and height to make it easier to use. However later I reconsidered this approach because:

  • width seems to never be needed;
  • I'm not sure that height is enough to reliably configure a display;
  • ... which may be even more confusing to developers when they first try the driver with the default settings (128x64), it fails, then they find this config, set the correct values and it fails again because for example it's a display from another manufacturer. In that case it will be quite hard to override lcd commands sent to display.

I decided to switch to more "generic" (but less intuitive) API to provide developers more flexibility while still being high-level enough to not send raw lcd commands. Maybe I should have mentioned typical config parameters for the most popular display sizes (128x64, 128x32, 96x16 and maybe others) in the documentation header.

Or it could be a good idea to provide public constants like

extern const esp_lcd_panel_ssd1306_config_t esp_lcd_panel_ssd1306_128x32 = {
  .mux_ratio = 0x2,
  .com_pin_alt = false,
  .com_lr_remap = false
}

... but I'm not quite sure about naming and overall coding style, that's my very first touch of the embedded area.

@suda-morris what do you think I should prefer:

  • mention typical configs for different screens in the documentation comment;
  • provide constants with configs for known display types;

I lean towards the constants path. If you agree could you please guide me on the coding style and naming I should use? I struggle with that so far.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @storoj I would prefer the first approach. As you also said, different manufacturers may require different settings. We can't provide the setting for all known display types in esp-idf as well, because that's the job of a BSP package. Ideally, the user will define his own BSP package like these, and in there, he can predefine the value of esp_lcd_panel_ssd1306_config_t according to his manufacture.

@espzav
Copy link
Collaborator

espzav commented Oct 27, 2023

@storoj Please install and use pre-comit hook for pass tests: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/contribute/install-pre-commit-hook.html

@suda-morris
Copy link
Collaborator

sha=8f75c4c5249c3a85d1e6e89cd7bd744183155d35

@suda-morris suda-morris added the PR-Sync-Merge Pull request sync as merge commit label Nov 6, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed labels Nov 6, 2023
@suda-morris
Copy link
Collaborator

merged in c4dc16c

movsb pushed a commit to movsb/esp-idf that referenced this pull request Dec 1, 2023
@nicklasb
Copy link

nicklasb commented Jan 9, 2024

For those that don't want to use another library just to get their 32px stuff going until this is this code is actually released and included everywhere... I just did this workaround that worked fine to me (stole the code from the esp-bsp SSD1306 library if it looks familiar): https://github.com/RobustoFramework/Robusto/blob/main/components/robusto/ui/src/robusto_32px.c

I call it here, just after the initialization to overwrite some of the settings the normal flow sends:

    ESP_ERROR_CHECK(esp_lcd_panel_init(panel_handle));
    ssd1306_init(CONFIG_ROBUSTO_UI_I2C_PORT, CONFIG_ROBUSTO_UI_I2C_HW_ADDR);
    ESP_ERROR_CHECK(esp_lcd_panel_disp_on_off(panel_handle, true));

It is not very invasive and easy to remove when done.
It doesn't restrict the screen size or anything like that, so you'd have to keep within the top 32 px, obviously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants