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

displayio.show() API change #7215

Merged
merged 16 commits into from
Dec 2, 2022
Merged

displayio.show() API change #7215

merged 16 commits into from
Dec 2, 2022

Conversation

FoamyGuy
Copy link
Collaborator

@FoamyGuy FoamyGuy commented Nov 16, 2022

resolves: #7213

This change allows display.root_group to be set to a group to be shown on the display rather than calling display.show(my_group).

There is example code in the linked issue that illustrates the difference in API.

Currently this change is not breaking, the display.show() function still exists and works the same as current main.

This also introduces displayio.SERIAL_GROUP which returns the Group object that contains the blinka icon and serial output text scroller. So it's possible to set the display to go back to showing that icon and serial output with code like this:

display.root_group = displayio.SERIAL_GROUP

This makes the code more descriptive of what it will do as opposed to the current None value that is used to indicate changing to the serial group in the current API.

Open questions:

  • Does anything else need to be done for the display.root_group setter? It seems to work as-is but I've only added minimal code to make it work, perhaps there is something I overlooked that still needs done.
  • Open to discussion on the name of displayio.SERIAL_GROUP I proposed displayio.TERMINAL_GROUP in the issue initially, but accidentally changed to SERIAL_GROUP when I implemented it. Would one or the other of those be most accurate? Or is there some better option?
  • Is it worth considering a name change to root_group itself? It's likely not used very widely as-is, only place I've seen it is tricks with showing displayio objects and the terminal at the same time. It's usage will increase if we start recomending it over display.show(). to me visible_group or showing_group are both more descriptive in my mind than root_group but I'm not sure if it's enough to matter or be worth more technically breaking changes even if not used too widely yet.

@Neradoc
Copy link

Neradoc commented Nov 16, 2022

How about CONSOLE_GROUP or CONSOLE ? I associate that term more with a display showing the output of the program.
(We use the term console in usb_cdc too for the REPL serial port).
I guess we don't have an official lexicon to make all the terms cohesive.

@FoamyGuy
Copy link
Collaborator Author

I am open to CONSOLE_GROUP or dropping group and using just CONSOLE. I've edited my original post with that and a few other remaining questions that I forgot to mention when I wrote it initially.

@tannewt
Copy link
Member

tannewt commented Nov 16, 2022

Open questions:

  • Does anything else need to be done for the display.root_group setter? It seems to work as-is but I've only added minimal code to make it work, perhaps there is something I overlooked that still needs done.

I think it's ok as-is. We may want to make None mean nothing is displayed rather than showing the CP terminal.

  • Open to discussion on the name of displayio.SERIAL_GROUP I proposed displayio.TERMINAL_GROUP in the issue initially, but accidentally changed to SERIAL_GROUP when I implemented it. Would one or the other of those be most accurate? Or is there some better option?

I think I like displayio.CIRCUITPYTHON_TERMINAL.

  • Is it worth considering a name change to root_group itself? It's likely not used very widely as-is, only place I've seen it is tricks with showing displayio objects and the terminal at the same time. It's usage will increase if we start recomending it over display.show(). to me visible_group or showing_group are both more descriptive in my mind than root_group but I'm not sure if it's enough to matter or be worth more technically breaking changes even if not used too widely yet.

My one concern with visible or showing is that the group itself could say it is hidden. That'd make it "showing" but still invisible. "root" is pretty jargony but does refer to the tree nature of displayio groups.

@tannewt
Copy link
Member

tannewt commented Nov 16, 2022

I guess we don't have an official lexicon to make all the terms cohesive.

The design guide is a good place to document terminology. design guide source

@FoamyGuy
Copy link
Collaborator Author

The latest commits make a few changes including

  • changing the name of the constant to displayio.CIRCUITPYTHON_TERMINAL and make it return the terminal group that is already in memory.
  • Change the internal API calls to use set_root_group instead of show all the way down into display_core (made this internal change for 3 types of displays: displayio.Display, displayio.EPaperDisplay, frambufferio.FrameBufferDisplay)
  • Added the set root group property to EPaperDisplay (tested successfully on MagTag)

One remaining open item for sure is adding the set root group property to FrameBufferDisplay and test on some of the device where that is used. I'm intending to work on that tomorrow. Once that is in place then I think this is ready to move out of draft state.

@FoamyGuy
Copy link
Collaborator Author

Okay, the latest commits add the set root group functionality for FrameBufferDisplay. I tested it successfully on Feather RP2040 with a small Sharp Display breakout, as well as Matrix Portal M4 with an RGB panel. The show() API remains in tact for now, but internally it also uses the new set_root_group stuff.

I think this is ready for further review now.

@FoamyGuy FoamyGuy marked this pull request as ready for review November 19, 2022 17:47
@@ -162,7 +162,7 @@ void displayio_display_core_set_rotation(displayio_display_core_t *self,
}
}

bool displayio_display_core_show(displayio_display_core_t *self, displayio_group_t *root_group) {
bool displayio_display_core_set_root_group(displayio_display_core_t *self, displayio_group_t *root_group) {

if (root_group == NULL) { // set the display to the REPL, reset REPL position and size
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making root_group == NULL not show anything? show() could keep this behavior but root_group would distinguish between None and CIRCUITPYTHON_TERMINAL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do like that idea. From person using the API perspective that feels like the expected behavior.

The first way that comes to mind for me to show nothing is to make a new empty Group and show that. This would visually appear like showing nothing, but is perhaps against the spirit of "Nothing" since it's also just a Group that you can then start adding things to.

Is there some better or more proper way? If it is possible would we want to actually remove the old group and just not add a new one? Or is the empty Group sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

I think you'd actually want None. You can always add an empty Group from user code if that's what you want. (This might be trickier internally and you could hide the empty internal Group its easiest. .root_group should be None though.)

I also wonder if None shouldn't trigger the terminal when user code is done. That'd make disabling the terminal easier (#2791).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may need to also make a check specifically for setting to CIRCUITPYTHON_TERMINAL as well to do this setup that sets the positon and starts the terminal.

https://github.com/FoamyGuy/circuitpython/blob/ef3398422a88f72cf5863e768f6a5f705d1ffb9e/shared-module/displayio/display_core.c#L168-L173

I will give try it out with None and see if I can get that working.

Do you know where to look to find the code causes the initialization of the terminal for the first time when the device boots up? I think something somewhere may call show(None) to get the terminal showing after boot up, if so it'll need to set it to CIRCUITPYTHON_TERMINAL instead.

Copy link
Member

Choose a reason for hiding this comment

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

Do you know where to look to find the code causes the initialization of the terminal for the first time when the device boots up? I think something somewhere may call show(None) to get the terminal showing after boot up, if so it'll need to set it to CIRCUITPYTHON_TERMINAL instead.

I think the first time is in common_hal_displayio_display_construct (board_init calls it.) Subsequent sets back to terminal are done in reset_displays I believe.

Copy link
Collaborator Author

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

The latest commits implement the display.root_group = None behavior this is now showing nothing on the display instead of the CIRCUITPYTHON_TERMINAL

However I have noticed that after you set root_group to None it will raise an AttributeError if you then try to access root_group before setting it to something else. I'm working on resolving that. I am making a single test script that will test and report all behaviors as well to make it easy to verify things are working as expected in all cases.

shared-module/displayio/display_core.c Show resolved Hide resolved
@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Nov 28, 2022

The latest commit returns None properly after it's been set as the root_group. This script tests and reports each behavior:

import time
import displayio
import board
import terminalio
from adafruit_display_text import bitmap_label
from displayio import Group

WAIT_TIME = 1  # second(s)
main_group = Group()

def check_root_group():
    print(f"root_group is None ? {board.DISPLAY.root_group is None}")
    print(f"root_group == displayio.CIRCUITPYTHON_TERMINAL ? {board.DISPLAY.root_group == displayio.CIRCUITPYTHON_TERMINAL}")
    print(f"root_group == main_group ? {board.DISPLAY.root_group == main_group}")
    print("\n")

print("before showing anything:")
check_root_group()

text = "displayio"
text_area = bitmap_label.Label(terminalio.FONT, text=text, scale=2)
text_area.x = 10
text_area.y = 10
main_group.append(text_area)
time.sleep(WAIT_TIME)

print("setting root_group to None:")
board.DISPLAY.root_group = None
check_root_group()
time.sleep(WAIT_TIME)

print("setting root_group to custom Group:")
board.DISPLAY.root_group = main_group
check_root_group()
time.sleep(WAIT_TIME)

print("setting root_group to terminal:")
board.DISPLAY.root_group = displayio.CIRCUITPYTHON_TERMINAL
check_root_group()
time.sleep(WAIT_TIME)

print("showing custom Group:")
board.DISPLAY.show(main_group)
check_root_group()
time.sleep(WAIT_TIME)

print("showing None:")
board.DISPLAY.show(None)
check_root_group()
time.sleep(WAIT_TIME)

while True:
    pass

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

I think you need one more case to be backwards compatible. show(None) should still set the display back to the terminal. root_group = None will not.

@FoamyGuy
Copy link
Collaborator Author

The new commit makes show(None) go back to current behavior of showing the terminal. I've updated the tester code in the previous comment with a few additional cases that test and check the show() function.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I suggested a different version that doesn't add an additional function call. It'll need to be moved to other show() functions for the other display classes too.

shared-module/displayio/Display.c Outdated Show resolved Hide resolved
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@tannewt tannewt merged commit 9e104c0 into adafruit:main Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Display.show() with assignment to Display.root_group
3 participants