Skip to content

zephyr: Introduce Zephyr displays #17939

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VynDragon
Copy link
Contributor

@VynDragon VynDragon commented Aug 16, 2025

Summary

Allows using zephyr displays as framebuf

Testing

SSD1327 + RP2040
and SSD1357 + RP2040

Copy link

github-actions bot commented Aug 16, 2025

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@VynDragon VynDragon force-pushed the zephyr_display branch 3 times, most recently from 7a89e4d to 48e8d5b Compare August 17, 2025 17:50
Copy link

codecov bot commented Aug 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (b7cfafc) to head (48e8d5b).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #17939   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22297    22297           
=======================================
  Hits        21938    21938           
  Misses        359      359           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Gadgetoid
Copy link
Contributor

Looking over this briefly, the duplicate methods with delegation to framebuf feel like they would be a nuisance to maintain. Anyone changing framebuf would need to check for compatibility breaks in Zephyr Display and I'm not sure that's feasible.

Is it possible to retrieve the buffer from a Zephyr Display and use that to initialise a vanilla instance of framebuf module? If so, some of the complexity of doing so could be swept under the rug with a Python module that sets the correct pixel type, dimensions, etc.

In such a case, ZephyrDisplay should just handle providing the buffer and some basic display config information and it would be compatible with not just framebuf, but any means by which a user cares to set pixels (ulab's numpy implementation for example.)

@VynDragon
Copy link
Contributor Author

VynDragon commented Aug 18, 2025

Zephyr doesnt provide a buffer to write into so it's not possible to use that directly.

But it sounds like maybe it should be possible to have the zephyrdisplay allocate the appropriate buffer and provide that instead, with other attributes to provide to framebuf (display format, etc? that's what you mean i think)

Maybe though, allocating the framebuf in the zephyrdisplay, and then having a method return that framebuf instead would be a better solution?

@VynDragon
Copy link
Contributor Author

Actually thinking about it there really is no point allocating the buffer for the user anyway if it's not fully automated /subclassing framebuf, instead the API from zephyr should be duplicated more closely, and allow mpy to retrieve the display infos, then send whatever buffer the user wants to send at x,y coordinate and of z size, like the zephyr API.

Maybe both should coexist (ZephyrDisplayFramebuf and ZephyrDisplay?)

@josuah
Copy link

josuah commented Aug 18, 2025

Possibly relevant: LVGL Zephyr integration for MicorPython: modlvzephyr.c in lv_binding_micropython
https://github.com/lvgl/lv_binding_micropython/tree/master/driver/zephyr

@Gadgetoid
Copy link
Contributor

Zephyr doesnt provide a buffer to write into

Ah! In that case yes - FrameBuf should allocate the buffer, and ZephyrDisplay should supply the dimensions and format of buffer the display needs. (Looks like it supplies compatible formats and the user picks one, which is nice)

the API from zephyr should be duplicated more closely, and allow mpy to retrieve the display infos, then send whatever buffer the user wants to send at x,y coordinate and of z size, like the zephyr API.

Absolutely this. It should be completely agnostic to whatever provides the framebuffer, so long as it is in the correct (or a supported?) pixel format and dimensions. It could be a numpy.array or a bytearray, it's all just bytes.

It looks like Zephyr's display interface (this? https://docs.zephyrproject.org/latest/doxygen/html/group__display__interface.html) is already well suited to this pattern.

For stuff like supported_pixel_formats I'd just to return the Zephyr ones and - as you suggest - supply a ZephyrDisplayFrameBuf module. Crucially I'd make this a Python module (which should be added to micropython-lib) since that's sort of the prevailing pattern for HAL vs glue code/user-facing API stuff (See Bluetooth for example).

So yes- agnostic, 1:1 implementation of the Zephyr display interface, and a micropython-lib library to glue FrameBuf and ZephyrDisplay together feels right in my very humble opinion.

Nice work though, FrameBuf needs more love 😆

@VynDragon VynDragon force-pushed the zephyr_display branch 2 times, most recently from d2f97e1 to 1be5c40 Compare August 19, 2025 15:22
@VynDragon
Copy link
Contributor Author

@Gadgetoid Updated to be like the API

@VynDragon VynDragon changed the title zephyr: Introduce Zephyr displays as framebuf zephyr: Introduce Zephyr displays Aug 19, 2025
@VynDragon VynDragon force-pushed the zephyr_display branch 2 times, most recently from 843e04d to 5d85ef2 Compare August 19, 2025 15:49
Copy link

@josuah josuah left a comment

Choose a reason for hiding this comment

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

I will need to revive my MicroPython memory and give it another look...
I hope this helps in the meantime.

Comment on lines 89 to 91
#if defined(CONFIG_DISPLAY) && defined(MICROPY_PY_FRAMEBUF)
{ MP_ROM_QSTR(MP_QSTR_Display), MP_ROM_PTR(&zephyr_display_type) },
#endif
Copy link

Choose a reason for hiding this comment

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

Now that there is no inheritance from framebuf, the && defined(MICROPY_PY_FRAMEBUF) can go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, forgot to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 64 to 65
mp_arg_check_num(n_args, n_kw, 1, 1, false);
int id = mp_obj_get_int(args[0]);
Copy link

Choose a reason for hiding this comment

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

Looking at other drivers, they use strings for creating new instances:

const struct device *dev = zephyr_device_find(args[ARG_id].u_obj);

const struct device *dev = zephyr_device_find(args[0]);

o->dev = zephyr_device_find(args[0]);

It seems like the first PR that uses DT_CHOSEN() mapped to numbers instead of strings, but it feels the right way to map chosen nodes IMHO.

To help the user figure whether to use a string or integer, some rule of thumb would be something like this?

  • Display(0) valid whenever a module supports DT_CHOSEN() (like you did now)
  • Display("labelname") always available to get a Zephyr device by string name

Using strings can be easier to implement as it's just a zephyr_device_find().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it can do both depending on input type, so ill add the string method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


if (mp_obj_get_int(arg) > MP_QSTR_ROTATION_270 || mp_obj_get_int(arg) < 0) {
mp_raise_ValueError("Invalid orientation");
}
Copy link

Choose a reason for hiding this comment

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

Maybe it is just me who misunderstood QSTR, but it looks like this is a comparison between a string index and a value as an integer here?

Maybe you meant to have an extra set of define like this:

#define DISPLAY_ROTATION_NONE 0
#define DISPLAY_ROTATION_90   1
#define DISPLAY_ROTATION_180  2
#define DISPLAY_ROTATION_270  3

And then use them in the zephyr_display_locals_dict_table:

    { MP_ROM_QSTR(MP_QSTR_ROTATION_NONE), MP_ROM_INT(DISPLAY_ROTATION_NONE) },
    { MP_ROM_QSTR(MP_QSTR_ROTATION_90), MP_ROM_INT(DISPLAY_ROTATION_90) },
    { MP_ROM_QSTR(MP_QSTR_ROTATION_180), MP_ROM_INT(DISPLAY_ROTATION_180) },
    { MP_ROM_QSTR(MP_QSTR_ROTATION_270), MP_ROM_INT(DISPLAY_ROTATION_270) },

So that they can be compared here as integers?

if (mp_obj_get_int(arg) > DISPLAY_ROTATION_270 ||
    mp_obj_get_int(arg) < DISPLAY_ROTATION_NONE) {

Copy link
Contributor

@Gadgetoid Gadgetoid Aug 19, 2025

Choose a reason for hiding this comment

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

Agreed. Using MP_QSTR_ROTATION_270 like this might actually work, but isn't very idiomatic of MicroPython bindings.

(Of course now I'm tempted to try.)

Edit: Of course I'm talking nonsense, mp_obj_get_int(arg) would raise a TypeError when given a string, which of course makes sense. And there's no guarantee that MP_QSTR_ROTATION_270 == 270 or even that the QSTR integer values are in any particular order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is supposed to be DISPLAY_ORIENTATION_ROTATED_270 ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to expose those constants to MicroPython too, perhaps with the same naming scheme so Zephyr users are at home?

    { MP_ROM_QSTR(MP_QSTR_DISPLAY_ORIENTATION_NORMAL), MP_ROM_INT(DISPLAY_ORIENTATION_NORMAL) },
    { MP_ROM_QSTR(MP_QSTR_DISPLAY_ORIENTATION_ROTATED_90), MP_ROM_INT(DISPLAY_ORIENTATION_ROTATED_90) },
    { MP_ROM_QSTR(MP_QSTR_DISPLAY_ORIENTATION_ROTATED_180), MP_ROM_INT(DISPLAY_ORIENTATION_ROTATED_180) },
    { MP_ROM_QSTR(MP_QSTR_DISPLAY_ORIENTATION_ROTATED_270), MP_ROM_INT(DISPLAY_ORIENTATION_ROTATED_270) },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


typedef struct _zephyr_display_obj_t {
mp_obj_base_t base;
uint8_t id;
Copy link

Choose a reason for hiding this comment

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

If wanting to support displays from other sources than chosen { zephyr,display = &xyz; }; for any reason, then this can be defined as const struct device *dev; like done elsewhere:

const struct device *dev;

const struct device *dev;

Then the code can be a bit shorter using self->dev instead of zephyr_display_devices[self->id].
There might be other reasons to use IDs that I missed though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just scrolled up here to make this same point! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

} zephyr_display_obj_t;

static void zephyr_display_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) {
zephyr_display_obj_t *self = self_in;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs MP_OBJ_TO_PTR I think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

len = sx * sy * DISPLAY_BITS_PER_PIXEL(capabilities.current_pixel_format) / 8;

if (len > bufinfo.len) {
mp_raise_ValueError("Buffer is shorter than size requires");
Copy link
Contributor

Choose a reason for hiding this comment

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

This and other errors should use MP_ERROR_TEXT so it doesn't explode the build if/when MicroPython's error text compression is enabled, eg:

mp_raise_ValueError(MP_ERROR_TEXT("Buffer is shorter than size requires"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

default:
return mp_obj_new_int(0);
}
return mp_obj_new_int(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unreachable return.

Possibly better to return mp_const_none in default: or even raise an exception so the user isn't left with a valid-looking colour in the exceptional case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
if (format < 0) {
mp_raise_msg(&mp_type_RuntimeError, "Display is not of a compatible pixel format");
return mp_const_none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unreachable return, there's no need to return after an exception.

I'm not totally sure a RuntimeError is the right pattern here, either. Should probably just return mp_const_none and let documentation indicate this means "incompatible." Otherwise it bails user code before they have a chance to (maybe?) set a compatible pixel format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


display_get_capabilities(zephyr_display_devices[self->id], &capabilities);

return mp_obj_new_int(capabilities.supported_pixel_formats);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe unpack this into a tuple so it's more idiomatic of Python? eg:

if display.FORMAT_MONO01 in display.zephyr_formats():
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@VynDragon VynDragon force-pushed the zephyr_display branch 3 times, most recently from 1b4f28e to 111420b Compare August 19, 2025 19:55
{ MP_ROM_QSTR(MP_QSTR_FORMAT_RGB_565), MP_ROM_INT(PIXEL_FORMAT_RGB_565) },
{ MP_ROM_QSTR(MP_QSTR_FORMAT_BGR_565), MP_ROM_INT(PIXEL_FORMAT_BGR_565) },
{ MP_ROM_QSTR(MP_QSTR_FORMAT_L_8), MP_ROM_INT(PIXEL_FORMAT_L_8) },
{ MP_ROM_QSTR(MP_QSTR_FORMAT_AL_88), MP_ROM_INT(PIXEL_FORMAT_AL_88) },
Copy link
Contributor

Choose a reason for hiding this comment

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

PIXEL_FORMAT_AL_88 is undefined since it was only added five days ago:

zephyrproject-rtos/zephyr@8c6c761

Copy link

Choose a reason for hiding this comment

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

Something like this to work around?

    { MP_ROM_QSTR(MP_QSTR_FORMAT_L_8), MP_ROM_INT(PIXEL_FORMAT_L_8) },
#ifdef PIXEL_FORMAT_AL_88 /* will be in Zephyr 4.3 */
    { MP_ROM_QSTR(MP_QSTR_FORMAT_AL_88), MP_ROM_INT(PIXEL_FORMAT_AL_88) },
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I work off main, applied josuah's solution.

Copy link

Choose a reason for hiding this comment

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

I forgot to indent the #ifdef MicroPython-style as you noticed.

mp_arg_check_num(n_args, n_kw, 1, 1, false);
const struct device *dev;

if (mp_obj_get_type(args[0]) == &mp_type_str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

mp_obj_is_type() is more conventional in these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Allows using zephyr displays.

Signed-off-by: Vdragon <mail@massdriver.space>
@VynDragon
Copy link
Contributor Author

Also fyi you can just give it the framebuf due to buffer protocol it's really nice:

disp = zephyr.Display("ssd1357@0")
fb =framebuf.FrameBuffer(bytearray(64*128*2), 64, 128, framebuf.RGB565)
fb.fill(0xF0F0)
disp.write(fb, 0, 0)

Copy link

@josuah josuah left a comment

Choose a reason for hiding this comment

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

Just a few extra checks...


if (id < 0 || id >= DT_ZEPHYR_DISPLAYS_COUNT) {
mp_raise_ValueError(MP_ERROR_TEXT("Invalid display ID"));
return mp_const_none;
Copy link

Choose a reason for hiding this comment

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

Following @Gadgetoid's previous comment, I discovered that mp_raise_msg() returns in a different function using MicroPython magic, via longjmp().

Comment on lines +80 to +81
mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("Display is not ready"));
return mp_const_none;
Copy link

Choose a reason for hiding this comment

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

AFAIU, return is not needed after mp_raise_msg().

Comment on lines +168 to +169
mp_raise_ValueError(MP_ERROR_TEXT("Not a framebuf pixel format"));
return mp_const_none;
Copy link

Choose a reason for hiding this comment

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

AFAIU, return is not needed after mp_raise_...().

Comment on lines +95 to +100
int x = mp_obj_get_int(args_in[2]);
int y = mp_obj_get_int(args_in[3]);
int sx;
int sy;

mp_get_buffer_raise(args_in[1], &bufinfo, MP_BUFFER_READ);
Copy link

Choose a reason for hiding this comment

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

At line 134 it is possible to have between 2 and 6 arguments.

Maybe you meant to allow disp.write(fb) and disp.write(fb, 0)?

Suggested change
int x = mp_obj_get_int(args_in[2]);
int y = mp_obj_get_int(args_in[3]);
int sx;
int sy;
mp_get_buffer_raise(args_in[1], &bufinfo, MP_BUFFER_READ);
int x = 0;
int y = 0;
int sx;
int sy;
mp_get_buffer_raise(args_in[1], &bufinfo, MP_BUFFER_READ);
if (n_args > 2) {
x = mp_obj_get_int(args_in[2]);
}
if (n_args > 3) {
y = mp_obj_get_int(args_in[3]);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is a mistake, it should be able to take only the buffer argument.

@josuah
Copy link

josuah commented Aug 20, 2025

Looking at other display drivers around:

  • microypython-lib has only 2 drivers with different APIs
  • circuitpython have their circuitpython framebufferio thing tightly bound to their drawing library, not really applicable there.
  • LVGL uses a framebuffer and a custom callback to flush, it looks like it would work. It's possible to use it through LVGL Zephyr binding anyway.

I like how it uses the buffer protocol now, it feels very native to MicroPython. :)

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.

3 participants