-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
tile palette mapper module and class #10113
base: main
Are you sure you want to change the base?
Conversation
a handful of atmel boards overflowed and I disabled the new module in them. I am not sure how to fix the zephyr builds that failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The palette and the ansi code is pushing the same boards so it probably makes sense to link the parameter flags
@@ -555,6 +555,9 @@ CFLAGS += -DCIRCUITPY_TERMINALIO=$(CIRCUITPY_TERMINALIO) | |||
CIRCUITPY_FONTIO ?= $(call enable-if-all,$(CIRCUITPY_DISPLAYIO) $(CIRCUITPY_TERMINALIO)) | |||
CFLAGS += -DCIRCUITPY_FONTIO=$(CIRCUITPY_FONTIO) | |||
|
|||
CIRCUITPY_TILEPALETTEMAPPER ?= $(CIRCUITPY_DISPLAYIO) | |||
CFLAGS += -DCIRCUITPY_TILEPALETTEMAPPER=$(CIRCUITPY_TILEPALETTEMAPPER) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than setting two parameters in the atmel-samd/boards mpconfigboard.mk files, I think the CIRCUITPY_TERMINALIO_VT100 I recently created should default to your new parameter. (After #10108 was merged, it's being set immediately following CIRCUITPY_TERMINALIO, but doesn't show up here?). I'm also setting the VT100 parameter to 0 for all samd21 boards in the atmel-samd mpconfigport.mk which could be removed if you make this change and decide to disable the new palette for all samd21 boards 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I think about it, the two features aren't technically, necessarily linked so perhaps two parameters is appropriate. Trade-offs... 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping them separate makes sense, but I also don't have a strong preference one way or the other. If it's possible to set it in one spot as disabled for all SAMD21 boards that sounds more convenient than adding to each of board individually though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting your assignment here will turn it off for all samd21 boards by default. The board files can still turn it back on if needed.
circuitpython/ports/atmel-samd/mpconfigport.mk
Lines 60 to 62 in e8de36a
# Turn off a few more things that don't fit in 192kB | |
CIRCUITPY_TERMINALIO_VT100 = 0 |
Edit: There were a couple of SAMD51 boards you overflowed as well though 😁 If you want to turn it off for all samd21 & samd51 boards you can use the same file and just move it outside the if block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I do want to keep it on for any samd51s that have room. The pyportal titano for instance is what all my testing was on and it seems fine on that device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on!
I'm thinking over how to organize the memory internally. The array of uint16_t for every tile is nice because it has a bunch of small allocations instead of one large one. It does cost the array of pointers though. This could be optimized to allocate the tile mapping only when changed. If the pointer is null then it'd just map 1:1 (or have a default mapping).
Definitely try with a palette larger than the input bitmap has. We'll want that for coloring text in addition to just inverting the colors.
To fix Zephyr you'll need to add tilepalettemapper = false
to all of the autogen_board_info.toml
s. Adding a python script may actually be quickest. https://github.com/adafruit/circuitpython/blob/main/ports/zephyr-cp/boards/nordic/nrf5340dk/autogen_board_info.toml#L92
self->needs_refresh = false; | ||
int mappings_len = width * height; | ||
self->tile_mappings = (uint16_t **)m_malloc(mappings_len * sizeof(uint16_t *)); | ||
uint32_t palette_len = common_hal_displayio_palette_get_len(self->palette); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Let's take in the number of input indices and then adjust the value length based on the number of colors in the palette.
# Conflicts: # ports/atmel-samd/boards/openbook_m4/mpconfigboard.mk # ports/atmel-samd/boards/uartlogger2/mpconfigboard.mk
…t_color to work with more than 2 colors, disable for samd21 instead of individaul boards, input_color_count arg.
The latest commit contains a bunch of the requested changes, as well as fixing the functionality for more colors, it turned out that was not behaving correctly before. This is the updated test script i've been using the test the color functionality:
There are also commented out lines that I've used to test the functionality when the TPM size does not match the TileGrid size. @tannewt I think the two remaining things that you mentioned that haven't been implemented yet are ColorConverter support, and optimizing by using null to mean "no mapping" for a tile. I'm wondering if it's okay to work on those as a followup to this? I'd love to get the palette support solidified and exercised more with user code before venturing into ColorConverter if possible. The null for no mapping efficiency improvement I'd like to attempt as well, but I don't have high confidence that I'll do it successfully without wrecking the functionality a bit first. I'll probably start that on a new branch anyhow. In addition to keeping this one functional it would also let me reset my local repo state which seems to have gotten some unrelated changes intermixed in that I'm having to uncheck boxes for every time I commit. |
Yup, totally! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more small things. Thanks for the iteration! It's getting close.
Co-authored-by: Scott Shawcroft <[email protected]>
Co-authored-by: Scott Shawcroft <[email protected]>
Co-authored-by: Scott Shawcroft <[email protected]>
Thank you, updated all of those in the latest commits. |
All testing was performed on a pyportal titano with this script: