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

Display: add diagnostic test_card option #6608

Merged
merged 17 commits into from
Apr 26, 2024

Conversation

nielsnl68
Copy link
Contributor

@nielsnl68 nielsnl68 commented Apr 23, 2024

What does this implement/fix?

When you start using a display, you want to know if it is showing correct etc.
This PR will show a testcard with the RGB colors, an indicator what the top left corner is and text (R,G,B) to validate how the display oriented.

The main purpose to help people that have issues with there display and want to show what is going on with there display.

Just add the option "show_testcard: true" to the display component or add it.test_card(); to your lambda code.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#3796

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# Example config.yaml
display:
  - platform: ili9xxx
    model: ili9342
    cs_pin: GPIO15
    dc_pin: GPIO2
    show_testcard: true

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.46%. Comparing base (4d8b5ed) to head (3f7e7b0).
Report is 480 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #6608      +/-   ##
==========================================
- Coverage   53.70%   53.46%   -0.25%     
==========================================
  Files          50       50              
  Lines        9408     9545     +137     
  Branches     1654     1685      +31     
==========================================
+ Hits         5053     5103      +50     
- Misses       4056     4131      +75     
- Partials      299      311      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nielsnl68 nielsnl68 changed the title add testcard when no pages or lambda is provided. Add testcard when no pages or lambda are provided. Apr 23, 2024
@jesserockz
Copy link
Member

As with all ESPHome code, we prefer not to have "magical" things happen like this.

It would make more sense if it was a function to call inside the lambda or a page:

lambda: |-
  it.test_card();

@nielsnl68 nielsnl68 closed this Apr 23, 2024
@nielsnl68 nielsnl68 reopened this Apr 23, 2024
@clydebarrow
Copy link
Contributor

clydebarrow commented Apr 23, 2024

Nice idea! A couple of suggestions - as well as the fade_to_{white,black} areas (which don't need to be repeated 7 times IMHO) something equivalent to this lambda is useful to diagnose mis-wired RGB interfaces, since it allows identification of individual bits for each color.

      it.filled_rectangle(0, 0, 40, 40, Color(0x80, 0, 0));
      it.filled_rectangle(0, 60, 40, 40, Color(0x40, 0, 0));
      it.filled_rectangle(0, 120, 40, 40, Color(0x20, 0, 0));
      it.filled_rectangle(0, 180, 40, 40, Color(0x10, 0, 0));
      it.filled_rectangle(0, 240, 40, 40, Color(0x8, 0, 0));

      it.filled_rectangle(40, 0, 40, 40, Color(0, 0x80, 0));
      it.filled_rectangle(40, 60, 40, 40, Color(0, 0x40, 0));
      it.filled_rectangle(40, 120, 40, 40, Color(0, 0x20, 0));
      it.filled_rectangle(40, 180, 40, 40, Color(0, 0x10, 0));
      it.filled_rectangle(40, 240, 40, 40, Color(0, 0x8, 0));
      it.filled_rectangle(40, 320, 40, 40, Color(0, 0x4, 0));

      it.filled_rectangle(80, 0, 40, 40, Color(0, 0, 0x80));
      it.filled_rectangle(80, 60, 40, 40, Color(0, 0, 0x40));
      it.filled_rectangle(80, 120, 40, 40, Color(0, 0, 0x20));
      it.filled_rectangle(80, 180, 40, 40, Color(0, 0, 0x10));
      it.filled_rectangle(80, 240, 40, 40, Color(0, 0, 0x8));
Screenshot 2024-04-23 at 12 23 25 pm

@nielsnl68
Copy link
Contributor Author

Nice idea! A couple of suggestions - as well as the fade_to_{white,black} areas (which don't need to be repeated 7 times IMHO) something equivalent to this lambda is useful to diagnose mis-wired RGB interfaces, since it allows identification of individual bits for each color.

the reason i used the fade_to is to see of the display driver runs in 8 or 16 bit mode. Having just a couple of blocks does not show that 100%. And it cost not that much time extra. The testcard is just called once after that the poller is stopt.

@nielsnl68
Copy link
Contributor Author

As with all ESPHome code, we prefer not to have "magical" things happen like this.

It would make more sense if it was a function to call inside the lambda or a page:

lambda: |-
  it.test_card();

Yes, i will do this.

@clydebarrow
Copy link
Contributor

the reason i used the fade_to is to see of the display driver runs in 8 or 16 bit mode. Having just a couple of blocks does not show that 100%.

Each repetition is the same, I'm suggesting keeping that, with say 2 repetitions over half the area, add the single-bit colours in the other half. This number of copies of the same gradients seems unnecessary:
Screenshot 2024-04-23 at 12 38 56 pm

@nielsnl68
Copy link
Contributor Author

the reason i used the fade_to is to see of the display driver runs in 8 or 16 bit mode. Having just a couple of blocks does not show that 100%.

Each repetition is the same, I'm suggesting keeping that, with say 2 repetitions over half the area, add the single-bit colours in the other half. This number of copies of the same gradients seems unnecessary.

Is this how you see it on your display?

@clydebarrow
Copy link
Contributor

Is this how you see it on your display?

Yes. Found the reason - https://github.com/esphome/esphome/pull/6608/files#r1575898035

Co-authored-by: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com>
@nielsnl68
Copy link
Contributor Author

Is this how you see it on your display?

Yes. Found the reason - https://github.com/esphome/esphome/pull/6608/files#r1575898035

Okay thanks. your suggestion is added.

@nielsnl68
Copy link
Contributor Author

Thinking about it a bit more i decided to make it much easier to popup the testcard.
Now you can just add show_testcard: true to the display component configuration and it shows up at the first moment the display is updated.

esphome/components/display/__init__.py Outdated Show resolved Hide resolved
esphome/components/display/__init__.py Outdated Show resolved Hide resolved
@esphome esphome bot marked this pull request as draft April 24, 2024 01:29
@esphome
Copy link

esphome bot commented Apr 24, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@nielsnl68 nielsnl68 marked this pull request as ready for review April 24, 2024 12:48
@esphome esphome bot requested a review from clydebarrow April 24, 2024 12:48
@nielsnl68
Copy link
Contributor Author

Is this how you see it on your display?

Yes. Found the reason - https://github.com/esphome/esphome/pull/6608/files#r1575898035

Clyde could you hummer me, why your fix works better then what i did? I just cant understand. And i want to learn from my mistake. :D
while i was testing it the c value nicely from 0 to 255. But when i see your image it looks as if it did do the same loop multiple times.

@clydebarrow
Copy link
Contributor

Clyde could you humour me, why your fix works better then what i did?

In your original code you had this:

    if (w > h) {
      image_w = w > 330 ? 310 : w - 20;
      image_h = h > 280 ? 255 : h - 20;
    } else {
      image_h = h > 330 ? 310 : h - 20;
      image_w = w > 280 ? 255 : w - 20;
    }

So image_h could be (and was in my test cases) greater than 255. But, you then passed that value to esp_scale():

      int c = esp_scale(i, image_h);

and the arguments to esp_scale() are defined as uint8_t:

inline static uint8_t esp_scale(uint8_t i, uint8_t scale, uint8_t max_value = 255) { return (max_value * i / scale); }

So e.g. a value for image_h of 280 gets truncated to 8 bits and would be seen by esp_scale() as 24, so as you go around the loop 280 times, the colour value cycles within a range of only 24 distinct values, so the pattern gets repeated numerous times.

Any portrait mode with height greater than 255 would fail to display as expected (as would landscape mode between 256 and 280.) So I assume you did not test in portrait mode, or you used a really small display.

@nielsnl68
Copy link
Contributor Author

thanks for the explanation. I was looking at the wrong place. and could not figure out what was wrong.

esphome/components/display/__init__.py Outdated Show resolved Hide resolved
esphome/components/display/__init__.py Show resolved Hide resolved
@esphome esphome bot marked this pull request as draft April 25, 2024 04:32
@nielsnl68 nielsnl68 changed the title Add testcard when no pages or lambda are provided. Alkowing ro show a testcard to validated the working of a display Apr 25, 2024
nielsnl68 and others added 2 commits April 26, 2024 01:24
Co-authored-by: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com>
@nielsnl68 nielsnl68 marked this pull request as ready for review April 25, 2024 23:27
@esphome esphome bot requested a review from clydebarrow April 25, 2024 23:27
Copy link
Contributor

@clydebarrow clydebarrow left a comment

Choose a reason for hiding this comment

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

Working fine here.

@nielsnl68
Copy link
Contributor Author

Working fine here.

Could you make a picture to add to the Display doc?

@clydebarrow clydebarrow changed the title Alkowing ro show a testcard to validated the working of a display Display: add diagnostic test_card option Apr 26, 2024
@clydebarrow clydebarrow merged commit 031e26a into esphome:dev Apr 26, 2024
101 checks passed
@nielsnl68 nielsnl68 deleted the nvds-display-testcard branch April 26, 2024 09:58
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants