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

Add graphical display menu #4105

Merged
merged 74 commits into from Dec 12, 2023

Conversation

MrMDavidson
Copy link
Contributor

@MrMDavidson MrMDavidson commented Nov 28, 2022

What does this implement/fix?

Adds a new component graphical_display_menu / GraphicalDisplayMenu which implements DisplayMenuComponent. Provides a mechanism for DisplayBuffer based displays (einks, etc) to render menus. Code currently supports non-fixed-height menu options.

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#2572

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040

Popup Example config.yaml:

# Example config.yaml
display:
  - platform: ...
    id: my_display

font:
  - file: ...
    id: my_font
    size: 16

graphical_display_menu:
  id: my_menu
  display: my_display
  # Optional: For displays that use `update_interval: never` this provides a trigger when redraw would have fired
  on_redraw:
    then:
      component.update: my_display
  font: my_font
  active: true
  items:
    - type: label
      text: Normal label
    - type: menu
      text: Child Menu
      items:
        - type: label
          text: A child menu item
        - type: back
          text: Back    
    

Adavanced Drawing Mode Example config.yaml

# Example config.yaml
display:
  - platform: ...
    id: my_display
    pages:
      - id: menu_page
        lambda: |-
          const auto display_width = it.get_width();
          const auto display_height = it.get_height();

          it.print(0, 0, id(my_font), COLOR_ON, TextAlign::TOP_LEFT, "Other drawing example");

          const auto font_height = 20;
          it.menu(0, font_height, id(my_menu), display_width , display_height - font_height);

font:
  - file: ...
    id: my_font
    size: 16

graphical_display_menu:
  id: my_menu
  # Optional: You'll want this to trigger the drawing of the lambda
  on_redraw:
    then:
      component.update: my_display
  font: my_font
  active: true
  items:
    - type: label
      text: Normal label
    - type: menu
      text: Child Menu
      items:
        - type: label
          text: A child menu item
        - type: back
          text: Back    
    

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:

Derives from DisplayMenuComponent to provide similar functionality as lcd_menu but for displays which are not character/row driven. Eg. eink displays and anything else providing a DisplayBuffer
@probot-esphome
Copy link

Hey there @MrMDavidson,
Thanks for submitting this pull request! Can you add yourself as a codeowner for this integration? This way we can notify you if a bug report for this integration is reported.
In __init__.py of the integration, please add:

CODEOWNERS = ["@MrMDavidson"]

And run script/build_codeowners.py

(message by NeedsCodeownersLabel)

@MrMDavidson
Copy link
Contributor Author

MrMDavidson commented Nov 28, 2022

I'd be curious about your thoughts on this @numo68. One thought I had in starting this implementation is that the rendering/layout of menu items might be better suited to the menu items themselves rather than the menu. I'd then imagine basically having a specialised hierarchy of menu items (eg. GraphicalDisplayMenuItemBase vs LcdDisplayMenuItemBase) that provides the basic drawing but someone could come along and add MyVeryParticularGraphicalDisplayMenuItem. For dynamically sized items the menu item itself would also know if the potential size had been changed or not so could invalidate a cached measurement of the item rather than having to calculate the size every draw loop.

@nielsnl68
Copy link
Contributor

Hi @MrMDavidson,

Did you know i am working on a GUI (switchplate) version for esphome.

Do you think we could intergrade your menu into my switchplate code at https://github.com/nielsnl68/switchplate

@numo68
Copy link
Contributor

numo68 commented Nov 30, 2022

Hi @MrMDavidson ,

nice to see someone stepping up and extending this for graphical displays, thanks :)

One thought I had in starting this implementation is that the rendering/layout of menu items might be better suited to the menu items themselves rather than the menu.

My idea was to have the menu item manage the data and states/transitions only, and the menu having the responsibility to render it. I still think that it is a clean way to separate the concerns.

I can well imagine the menu rendering engine attaching some data to the menu items for caching purposes or similar (transparent to the item), or the menu item setting some kind of a 'dirty' flag indicating that the value to display changed so the positions/dimensions might need to be recalculated. Maintaining a parallel hierarchy for both menu and menu items depending on the type of display to use seem harder to maintain to me.

I am however not very good at the best practices on how to implement GUIs so I guess I let others comment on what approach is optimal.

@landonr
Copy link
Contributor

landonr commented Dec 2, 2022

excited to see the progress @MrMDavidson! I've been working on a lcd menu for esphome on my project here https://github.com/landonr/esphome-remote and I want to make it more useful for general esphome users as a component. I've been looking at https://github.com/Spirik/GEM for inspiration.

  • my ui supports menu hierarchy, checkboxes, sliders, and has a scrollbar.
  • the menu items in my project have a state and a type so the ui can draw checkboxes, or icons
  • my project has a ton of custom code for managing media players that I need to split up from the ui
  • it would be nice if the menu code was separate for the display rendering so people could use tft_espi or lvgl if they want to
  • how do you plan on viewing sensor states inside of a child menu? can you update your example

@nagyrobi
Copy link
Member

nagyrobi commented Dec 2, 2022

@MrMDavidson the menu component works exceptionally well on Character LCD displays and having the option to display the same logic on graphical displays is very welcome.

Will this be full-screen only, or will this be able to be shown among other items on the screen, eg. in a rectangle to constrain it within a space, and/or on a separate page?

@MrMDavidson
Copy link
Contributor Author

MrMDavidson commented Dec 2, 2022

Will this be full-screen only, or will this be able to be shown among other items on the screen, eg. in a rectangle to constrain it within a space

@nagyrobi: It's all positioned dynamically so adding a bounding box to constrain it isn't too hard.

on a separate page?

Currently it creates its own DisplayPage on setup to take ownership of to draw the menu. From memory I don't think I'd be able to draw over the top of an existing page that is already defined as I need to access the display_writer_t. It might still be worth while allowing users to pass in a specific DisplayPage so they can skip it easier for navigation purposes, or whatever

@MrMDavidson
Copy link
Contributor Author

My idea was to have the menu item manage the data and states/transitions only, and the menu having the responsibility to render it. I still think that it is a clean way to separate the concerns.

I can well imagine the menu rendering engine attaching some data to the menu items for caching purposes or similar (transparent to the item), or the menu item setting some kind of a 'dirty' flag indicating that the value to display changed so the positions/dimensions might need to be recalculated.

@numo68 I think the current approach works well as a sort of flyweight pattern and if there's no new menus. The problem I see is that if someone wants to add a new menu type they have to add a new MenuItemType, create the MenuItem derivative, and update the DisplayMenuComponent. If you flip it over so the MenuItem draws itself draw_menu() basically just becomes;

int current_y = 0;

for (size_t i = 0; i < this->displayed_item_->items_size(); i++) {
  auto *item = this->displayed_item->get_item(i);
  int x = 0;
  int y = current_y;
  int width = 0;
  int height = 0;
  item->draw(x, y, &width, &height);
  current_y += height;
}

And any new styles of menu just require a new implementation of MenuItem. We could then have MenuItem contain basic state, LcdMenuItem derive from it and basically take what's in the current draw_item so the current menu items don't need to be changed. And then GraphicalDisplayMenuItem from MenuItem.

Maintaining a parallel hierarchy for both menu and menu items depending on the type of display to use seem harder to maintain to me.

The parallel hierarchy should only be an issue for the python code in menu_item_to_code.

I am however not very good at the best practices on how to implement GUIs so I guess I let others comment on what approach is optimal.

I might branch from this if I get a chance and have a quick hack at an example so we can see it in practice.

@gaaf
Copy link
Contributor

gaaf commented Nov 7, 2023

i have some minor problems with popup mode:

1.  the text is overdrawn, like the code thinks the display is bigger then it is. display is SH1106.

A fix for the out-of-bounds draw is here: Check MrMDavidson#125

@gaaf
Copy link
Contributor

gaaf commented Nov 7, 2023

I'm trying to get the menu to popup on the bottom half of the display. If I set the menu to active in the yaml, the menu will show on the bottom half due to the display draw lambda. But after a leave and then an enter, the menu covers the whole screen. Same if menu starts as inactive initially and gets activated by a display_menu.show.

Unfortunately I'm away from my PC for the next couple of weeks so will be unable to look into this issue until then.

I found the cause. Still had a display: configured in the menu's config. After removing that,it works as expected.

Item would be drawn below the y2-bound (h).
…phical-display-menu

# Conflicts:
#	CODEOWNERS
#	esphome/core/defines.h
@MrMDavidson MrMDavidson requested a review from a team as a code owner December 3, 2023 00:17
MrMDavidson and others added 8 commits December 4, 2023 23:32
Fix out of bounds draw of last menu item(s)
Padding is only drawn when there are multiple items.
Use only the index as loop variable, don't use the size(counter).
Using mixed index/counter is very confusing to read.
Scroll an additional line to make the selected item fully visible
Otherwise, all items would be draw but clipped.
@beyondthemind91an
Copy link

Hi! How's this project going? I'm really excited about this, finally having a menu 🙂
I'm still quite new to GitHub, so I'm not sure how everything works. But, is it just a review that needs to be done for it to be accessible for use?

@MrMDavidson
Copy link
Contributor Author

MrMDavidson commented Dec 7, 2023

Hi! How's this project going? I'm really excited about this, finally having a menu 🙂 I'm still quite new to GitHub, so I'm not sure how everything works. But, is it just a review that needs to be done for it to be accessible for use?

Nope - you can use it as is now (provided you're okay with alpha code) as an external component. You can find details of doing this in my earlier comment - #4105 (comment) . Once the PR gets reviewed and accepted it'll go into a future release.

Interim docs can be found in the PR for the docs project at esphome/esphome-docs#2572

Finally, until ESPHome 2023.12 is released you will need to be using the ESPHome Dev plugin for HomeAssistant (you can run it along side the release version)

@beyondthemind91an
Copy link

Hi! How's this project going? I'm really excited about this, finally having a menu 🙂 I'm still quite new to GitHub, so I'm not sure how everything works. But, is it just a review that needs to be done for it to be accessible for use?

Nope - you can use it as is now (provided you're okay with alpha code) as an external component. You can find details of doing this in my earlier comment - #4105 (comment) . Once the PR gets reviewed and accepted it'll go into a future release.

Interim docs can be found in the PR for the docs project at esphome/esphome-docs#2572

Finally, until ESPHome 2023.12 is released you will need to be using the ESPHome Dev plugin for HomeAssistant (you can run it along side the release version)

Thank you! I got it to work, finally, on my SSD1306. I first had some trouble with getting the fonts to work, but that was related to my HA setup and not graphical display. I will also try it on a ST7789, if that's supported.

Looking forward to the official release!

@MrMDavidson
Copy link
Contributor Author

Thank you! I got it to work, finally, on my SSD1306. I first had some trouble with getting the fonts to work, but that was related to my HA setup and not graphical display. I will also try it on a ST7789, if that's supported.

Yup - because the underlying abstraction is the one that all of the displays implement this will work for any of the graphical style displays. I personally use it with the Waveshare eink screens where it works pretty well!

Looking forward to the official release!

Me too! If you encounter any issues please drop a note in here.

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing.
Just a few minor python nitpicks, but otherwise good to go.

esphome/components/graphical_display_menu/__init__.py Outdated Show resolved Hide resolved
esphome/components/graphical_display_menu/__init__.py Outdated Show resolved Hide resolved
esphome/components/graphical_display_menu/__init__.py Outdated Show resolved Hide resolved
esphome/components/graphical_display_menu/__init__.py Outdated Show resolved Hide resolved
@@ -116,6 +116,7 @@ esphome/components/gp8403/* @jesserockz
esphome/components/gpio/* @esphome/core
esphome/components/gps/* @coogle
esphome/components/graph/* @synco
esphome/components/graphical_display_menu/* @MrMDavidson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jesserockz Thoughts on this? Should I set it to someone else?

Copy link
Member

Choose a reason for hiding this comment

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

Why would it be set to someone else? This is your PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Mentioned it because the linter complains as I don't have write access to the repo. Not sure if that's an actual issue or not.

Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com>
@jesserockz jesserockz merged commit b30430b into esphome:dev Dec 12, 2023
45 checks passed
@jesserockz jesserockz mentioned this pull request Dec 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
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