Allow setting of CIRCUITPY_DISPLAY_LIMIT in settings.toml #10214
Conversation
| if (displays[i].epaper_display.base.type != &epaperdisplay_epaperdisplay_type || | ||
| displays[i].epaper_display.core.current_group != &circuitpython_splash) { | ||
| // Skip regular displays and those not showing the splash. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Do this after you've gotten the display object to inspect. Maybe add an actual function to get display X that can switch been the static array and the dynamic one.
There was a problem hiding this comment.
Do this after you've gotten the display object to inspect.
Thanks! moving it is much cleaner.
Maybe add an actual function to get display X that can switch been the static array and the dynamic one.
When you suggest an actual function do you mean the macro functions I'm using in the other modules? I left it out here because there were only a couple instances and thought it was easier to understand without the macro. Or are you suggesting I add a new common_hal function that returns a separate display object from the appropriate memory structure and use that instead of the macros?
main.c
Outdated
| #if CIRCUITPY_DISPLAYIO && CIRCUITPY_OS_GETENV && CIRCUITPY_SET_DISPLAY_LIMIT | ||
| // If number of displays has been overridden in settings.toml allocate memory and | ||
| // move existing displays | ||
| malloc_display_memory(); |
There was a problem hiding this comment.
Since you are doing this really early, can you just have one dynamically allocated array? It'll make all of the other accessing code simpler.
There was a problem hiding this comment.
That was my original attempt, however the boards that had displays defined as board.DISPLAY didn't like the dynamic memory allocation. I tried multiple pointer configurations to try and get the compile to work but didn't look into modifying how board.DISPLAY was defined.
I can go back and dig into the board.DISPLAY setup and try and get it to work with the dynamically allocated memory structure if you think that makes sense.
There was a problem hiding this comment.
Ah, right. board.DISPLAY is static.
I wonder if it'd be more fruitful to just try and allocate displays to the CP heap first and then move them over if needed.
There was a problem hiding this comment.
@tannewt I thought you mentioned somewhere else about replacing board.DISPLAY with something else in 10 or 11? Would board.display() or similar be helpful?
There was a problem hiding this comment.
I still don't have a working understanding of the complete CP memory model so I'm not sure how I would do that, is there a mechanism like port_malloc that would allocate heap space?
There was a problem hiding this comment.
Well the user code displays seem to be working with m_malloc now. I've held off working on moving displays to the static storage as I think that will be related to the primary display features and I thought it would be better to work on that in a follow up PR.
There was a problem hiding this comment.
Sounds good! Can you have the first display use the single static allocation?
There was a problem hiding this comment.
You mean if it hasn't been used by picodvi, board.DISPLAY, or anything else yet. Yea, if I do that it will save allocating the dynamic storage for a single user display. I'll give it a go 😁
There was a problem hiding this comment.
Actually maybe I'm misunderstanding, I think it already grabs the static storage first.
Edit: Yep, just tested and first user screen survives VM (ctrl-D) but I wasn't setting the primary display properly, just fixed with a new commit.
There was a problem hiding this comment.
Ok, sorry about that. I hadn't looked at the updated code. Will do shortly!
|
Switching to draft while I track down an issue with I2C displays not showing up until after a ctrl-D restart. I think it has something to do with the bus not being a dedicated display bus but I'm going to need to stare at it for a bit more.... |
|
Making active again, this is the second time the SH1107 I2C display has sent me down a wild goose troubleshooting path. I'll open an issue on the library and hopefully remember now that when the I2C display starts acting flaky that it might not be the core code.... |
There was a problem hiding this comment.
We just chatted about this. I'd like to see the environment variable removed and the dynamic array removed as well. Instead, we should return the static allocation for the first display allocation and then return dynamic ones after. The dynamic ones should deinit on their own when finalized. This should remove much of the code and remove the dynamic cap entirely.
Thanks for working on this @RetiredWizard!
|
After talking with Tannewt I'm going to try and get dynamically allocated displays working without the VM managed display and bus arrays. I don't think that approach is going to bear much resemblance to this attempt so I'm closing this now and will hopefully have a new PR within the next week or so. |
Set the maximum supported number of displayio displays. This value overrides the CIRCUITPY_DISPLAY_LIMIT compile-time flag.
Resubmitting earlier PR #10158 now against main rather than 9.x.x. This PR addresses #1760 but I think there are probably follow-up PRs needed before that issue can be considered closed.