Skip to content

extmod/modframebuf: Enable blit between different formats. #7682

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

Closed

Conversation

peterhinch
Copy link
Contributor

This replaces #7666 which included cruft which I was unable to remove.

@peterhinch
Copy link
Contributor Author

Please could someone advise on the AppVeyor failure: the log is completely baffling to me :-(

@stinos
Copy link
Contributor

stinos commented Aug 19, 2021

Errors are at the end of the log: '1 tests failed: extreme_exc' which is a known issue see e.g. PR 7659

@jimmo
Copy link
Member

jimmo commented Aug 19, 2021

Sorry about this, took a while to figure out (see discussion in #7659). The fix was just merged a couple of hours ago though, so you could force-push to trigger a re-test (or just happily ignore it). Thanks for re-sending the PR, still looks good :)

@@ -491,7 +491,11 @@ STATIC mp_obj_t framebuf_blit(size_t n_args, const mp_obj_t *args) {
if (n_args > 4) {
key = mp_obj_get_int(args[4]);
}

mp_obj_framebuf_t *palette = (mp_obj_framebuf_t *)MP_OBJ_NULL;
Copy link
Member

Choose a reason for hiding this comment

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

The two (mp_obj_framebuf_t *)MP_OBJ_NULL (this line and below) should be just regular NULL.

MP_OBJ_NULL is an mp_obj_t encoding of NULL. But in this case you have a pointer (mp_obj_framebuf_t*), not an mp_obj_t, so you should be using NULL.

mp_obj_cast_to_native_base will return MP_OBJ_NULL (or a valid mp_obj_t), and then MP_OBJ_TO_PTR will convert that to NULL.

(What you had works because under the hood, mp_obj_t and pointers are actually (usually) encoded exactly the same way -- MP_OBJ_NULL is actually equal to NULL).

@@ -514,6 +518,9 @@ STATIC mp_obj_t framebuf_blit(size_t n_args, const mp_obj_t *args) {
int cx1 = x1;
for (int cx0 = x0; cx0 < x0end; ++cx0) {
uint32_t col = getpixel(source, cx1, y1);
if (palette != (mp_obj_framebuf_t *)MP_OBJ_NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

This can just be if (palette)

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Aug 19, 2021
@peterhinch peterhinch force-pushed the framebuf-palette-new branch from 1af7174 to 6ad1f7b Compare August 20, 2021 08:01
@codecov-commenter
Copy link

Codecov Report

Merging #7682 (6ad1f7b) into master (44818d1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7682   +/-   ##
=======================================
  Coverage   98.25%   98.25%           
=======================================
  Files         154      154           
  Lines       20071    20077    +6     
=======================================
+ Hits        19721    19727    +6     
  Misses        350      350           
Impacted Files Coverage Δ
extmod/modframebuf.c 100.00% <100.00%> (ø)
py/obj.c 97.22% <0.00%> (-0.40%) ⬇️
py/runtime.c 99.24% <0.00%> (-0.16%) ⬇️
py/misc.h 85.71% <0.00%> (ø)
py/objfun.c 100.00% <0.00%> (ø)
py/lexer.c 98.99% <0.00%> (+<0.01%) ⬆️
py/parse.c 99.17% <0.00%> (+0.20%) ⬆️
py/bc.c 89.69% <0.00%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44818d1...6ad1f7b. Read the comment docs.

@dpgeorge
Copy link
Member

This is a very neat addition, nice and simple yet powerful. Thank you very much!

Merged in 2296df0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants