adafruit / circuitpython

CircuitPython - a Python implementation for teaching coding with microcontrollers
https://circuitpython.org
Other
4.09k stars 1.22k forks source link

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

Open tannewt opened 11 months ago

tannewt commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

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.