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

Stop moving I2C and SPI objects and remove release_displays(). #8675

Open
tannewt opened this issue Nov 30, 2023 · 7 comments
Open

Stop moving I2C and SPI objects and remove release_displays(). #8675

tannewt opened this issue Nov 30, 2023 · 7 comments

Comments

@tannewt
Copy link
Member

tannewt commented Nov 30, 2023

Right now display core will move I2C and SPI objects that they need to keep outside the VM. This is bug prone. Instead we should ensure the used object is already outside the Python VM. It can either be statically allocated (likely for use in board) or allocated dynamically to the outer heap.

This will be easier once we move to a model where only one display lives past VM reset. displayio currently lets all displays outlive the VM. In practice, this is almost always 1 because of the display limit. We can remove this limit with the split heap switch in 9 but it doesn't make it clear which display to use for the terminal. Instead, we can add an explicit supervisor.runtime.display = that allows for setting a single display to preserve outside the VM and use for the terminal. It also allows for user code to access a previously constructed display without needing to reconstruct it after release_displays(). The assignment can also validate that all objects are present outside the Python VM and won't need to be moved.

So:

  1. Introduce supervisor.runtime.display = that validates that the display and anything it uses won't need to be moved (is in `board.)
  2. Make release_displays() a no-op and release any other displays at VM end.
  3. Allocate all I2C and SPI objects off the VM heap by default so they can outlive the VM.

This will break display behavior because the "lives outside VM and is used by CP" will be explicit. Boards with displays can set this automatically though.

@jepler
Copy link
Member

jepler commented Dec 7, 2023

A note about (2): the pycamera has to release and reinitialize the display multiple times during a single application run. would it change to doing a deinit call on the specific display object instead of calling release_displays()?

@tannewt
Copy link
Member Author

tannewt commented Dec 7, 2023

Yes, you'd deinit the specific display. supervisor.runtime.display would give you access to a display created by a previous run and kept alive by CP. (It doesn't need to be board.DISPLAY.)

@bradanlane
Copy link

My comment is specific to the board.DISPLAY capability and related CircuitPython behavior; and more specific to boards with integrated ePaper displays. (I realize this is a very small segment but they pose an interesting set of scenarios.)

For a novice user - which CircuitPython has as a core persona - the behavior of displayio.release_displays() is confusing when the user did not initially use displayio to create the display.

With respect to ePaper displays, treating them as general purpose for console output is potentially damaging. Every time the user has an error in their code or hits CTRL-C the ePaper display is called upon and refreshes. This adds wear and tear to the ePaper display and may take 15 seconds or more to complete.

These are two separate issues. I am not sure if they are orthogonal or not.

Question 1: Should displayio.release_displays() effect a board level display which the user did not explicitly create and if so, how is the user to get the board level display back without the non-intuitive unplug/re-plug action.

Question 2: How will the developer of a board with an integrated display indicate the display is (or is not) suitable for console output?

@tannewt
Copy link
Member Author

tannewt commented Dec 12, 2023

For a novice user - which CircuitPython has as a core persona - the behavior of displayio.release_displays() is confusing when the user did not initially use displayio to create the display.

I agree. That's why I think it should be removed.

With respect to ePaper displays, treating them as general purpose for console output is potentially damaging. Every time the user has an error in their code or hits CTRL-C the ePaper display is called upon and refreshes. This adds wear and tear to the ePaper display and may take 15 seconds or more to complete.

We do treat eink specially internally. Specifically we respect the refresh time spacing that manufacturers suggest be 180 seconds. Showing errors is very valuable.

Note, with the proposed supervisor.runtime.display you'd need to opt an eink display in to be used. I assume boards with it built in would also use it. Showing errors is really valuable.

Question 1: Should displayio.release_displays() effect a board level display which the user did not explicitly create and if so, how is the user to get the board level display back without the non-intuitive unplug/re-plug action.

Yes, in case you need to reinitialize the display with different settings. If you need the display as-is, then don't do release displays.

Question 2: How will the developer of a board with an integrated display indicate the display is (or is not) suitable for console output?

board_init() will be able to set supervisor.runtime.display internally. I highly recommend it be set when possible because showing errors is really helpful. Especially when the device isn't connected to otherwise. (I've taken pictures of errors on my watch to get back to later for example.)

@bradanlane
Copy link

bradanlane commented Dec 13, 2023

We do treat eink specially internally. Specifically we respect the refresh time spacing that manufacturers suggest be 180 seconds. Showing errors is very valuable.

The refresh time is a problem. The user can set this when creating an epaper display using one of the libraries such as adafruit_ssd1681.mpy but for an integrated display, this is hard coded. Because of this, most boards with an integrated display choose to use as short a value as possible.

Note, with the proposed supervisor.runtime.display you'd need to opt an eink display in to be used. I assume boards with it built in would also use it. Showing errors is really valuable.

Opt-In is definitely my preference.

board_init() will be able to set supervisor.runtime.display internally. I highly recommend it be set when possible because showing errors is really helpful. Especially when the device isn't connected to otherwise. (I've taken pictures of errors on my watch to get back to later for example.)

One problem with having a the ePaper display show errors is when first using a new board, beginning with a new code project, or the initial CircuitPython tutorials.

A fresh install of CircuitPyton on a new board creates code.py with print("hello world"). This terminates to REPL and thus the REPL output rendered in the ePaper display. Similarly, when writing a new program, there are a lot of quick errors to resolve.

Having the ability to configure the board specific CircuitPython firmware build to opt-out will be a great addition.

In the meantime, we will plan to ship with some workaround.

@deshipu
Copy link

deshipu commented Dec 15, 2023

This sounds good, as long as I can still set supervisor.runtime.display in board init code to the display that got initialized there.

I wonder, though if we don't want to make release_displays() do supervisor.runtime.display.deinit() for compatibility reasons, possibly with a warning?

@tannewt
Copy link
Member Author

tannewt commented Jan 17, 2024

This sounds good, as long as I can still set supervisor.runtime.display in board init code to the display that got initialized there.

Yup, that'd be the idea.

I wonder, though if we don't want to make release_displays() do supervisor.runtime.display.deinit() for compatibility reasons, possibly with a warning?

That would be good idea for the transition release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants