Encapsulate the buffers within PixelBuf #2550
Conversation
| obj_obj = MP_OBJ_FROM_PTR(obj); | ||
| } | ||
| mp_convert_member_lookup(obj_obj, type, elem->value, lookup->dest); | ||
| mp_convert_member_lookup(MP_OBJ_FROM_PTR(obj), type, elem->value, lookup->dest); |
There was a problem hiding this comment.
If this has no unexpected side effects this is a nice little cleanup!
There was a problem hiding this comment.
I expect side effects so they won't be unexpected. :-) We can hunt them down as they come up.
There was a problem hiding this comment.
The unit tests pass... 🤷♂ Probably some edge cases in inheritance again. I wish we could exercise all the examples in learn!
| // mutable by the code itself. | ||
| uint8_t* transmit_buffer = (uint8_t*) o->data; | ||
| memcpy(transmit_buffer, header, header_len); | ||
| memcpy(transmit_buffer + header_len + pixel_len, trailer, trailer_len); |
There was a problem hiding this comment.
Correct me if I'm wrong, but doesn't this make the header and trailer fixed at PixelBuf construction? While this is fine for Dotstar, what if some other protocol needs to manipulate the header or trailer? (Not that this is necessarily a bad thing).
There was a problem hiding this comment.
It does fix it. We can deal with other protocols when we find one that needs it.
| memcpy(transmit_buffer + header_len + pixel_len, trailer, trailer_len); | ||
| self->post_brightness_buffer = transmit_buffer + header_len; | ||
|
|
||
| if (self->byteorder.is_dotstar) { |
There was a problem hiding this comment.
While we're abstracting things, perhaps we should move this kind of initialization to another function or method. It's always bothered me that the base PixelBuf has to know the details of DotStar, rather than having a subclass or implementation that provides this.
There was a problem hiding this comment.
Ya, I agree this is weird. I don't really want to tackle it now though. My main concern was the buffer management.
| pixelbuf_pixelbuf_obj_t* self = native_pixelbuf(self_in); | ||
| self->brightness = brightness; | ||
| size_t pixel_len = self->pixel_count * self->bytes_per_pixel; | ||
| if (self->pre_brightness_buffer == NULL) { |
There was a problem hiding this comment.
Couldn't this malloc and copy be avoided if brightness >= 1.0?
If this malloc fails, will the error that happens be easy to understand?
There was a problem hiding this comment.
Yup, in fact I need to add this. Otherwise we'll always allocate it because the constructor calls it. Will do now. :-)
There was a problem hiding this comment.
Ok, this is updated. I confirmed with gc.mem_frees and a large number of pixels that the allocation happens once brightness is set to something besides 1.0.
|
Ok, this is ready for another look. I got some space back by tweaking a couple error messages. |
| // self->buffer, so do it manually. (However, as long as internal | ||
| // pointers like this are NOT moved, allocating the buffer | ||
| // in the long-lived pool is not strictly necessary) | ||
| self->buffer = (uint8_t *) gc_alloc(self->buffer_length * sizeof(uint8_t), false, true); |
There was a problem hiding this comment.
Why not use m_malloc here? You'd get the error message for free.
There was a problem hiding this comment.
So that we can deinit the UART before throwing the error.
py/objlist.c
Outdated
|
|
||
| STATIC void list_print(const mp_print_t *print, mp_obj_t o_in, mp_print_kind_t kind) { | ||
| mp_obj_list_t *o = MP_OBJ_TO_PTR(o_in); | ||
| //mp_obj_list_t *o = mp_instance_cast_to_native_base(o_in, &mp_type_list); |
There was a problem hiding this comment.
Ah, I thought I might need it. Will remove it.
|
Removed and merged in latest master. |
jepler
left a comment
jepler
left a comment
There was a problem hiding this comment.
I built this and adapted a neopixel demo of my own to use it. It resulted in nearly doubling my framerate, according to time.monotonic, which is rather nice given that it's a fairly intensive on FP arithmetic. That means a LOT of performance improvement via this upgrade, nearly doubling my updates per second (18Hz -> 34Hz on a 96-pixel strip)
(the superseded neopixel release 4.1.0 did not work for me but---aside from the branch of code for parsing byte order as a tuple---it was not hard to sort out. Expect some work to be needed there, I guess, not just reinstatement of the original version)
As for the code, I posted a couple of nits (I accidentally did them as single comments). which are already sorted out.
If the concerns about subtle breakage come true, we need to remember to add testsuite tests for it when they are found.
3 participants
Also changes MicroPython so that native methods get a subclass instance instead of the native object directly. This may break things.