Skip to content
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

extmod/modframebuf.c: Expose width(), height(), format(), and stride() accessors on framebuf.Framebuffer #7828

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

theidealist
Copy link

@theidealist theidealist commented Sep 20, 2021

It can be helpful when building up native python libraries that utilize external framebuf.Framebuffers to know the width and height of those objects. This PR adds four simple methods to framebuf.Framebuffer that permit retrieval of width, height, format and stride.

Tested locally on the ESP32 GENERIC board port:

>>> w = 400
>>> h = 300
>>> buf = bytearray(w * h // 8)
>>> fb = framebuf.FrameBuffer(buf, w, h, framebuf.MONO_HLSB)
>>> fb.width()
400
>>> fb.height()
300
>>> fb.format()
3
>>> fb.format() == framebuf.MONO_HLSB
True
>>> fb.stride()
400

@peterhinch
Copy link
Contributor

I provide access in a subclass. If you are going to do it in the base class I would recommend also providing access to the format. This would aid use of the blit method: to construct a palette you need to know the destination frame buffer's format.

@theidealist
Copy link
Author

theidealist commented Sep 21, 2021

Thanks for the feedback, @peterhinch!

I provide access in a subclass.

Yes, I also do this, but then I wanted to reimplement blit in my subclass (for rotating/inverting) and wanted to maintain the same interface, insofar as I could, anyway, and the blit method accepts just a framebuffer. Maybe this framebuffer is coming from another library and I don't have easy access to it's width/height? It just felt like something the base class should have, I guess.

If you are going to do it in the base class I would recommend also providing access to the format. This would aid use of the blit method: to construct a palette you need to know the destination frame buffer's format.

This is a good suggestion. I'll do this, too.

@theidealist theidealist changed the title extmod/modframebuf.c: Expose width and height functions on framebuf.Framebuffer extmod/modframebuf.c: Expose width(), height(), format(), and stride() accessors on framebuf.Framebuffer Sep 21, 2021
@volkerjaenisch
Copy link

Thanks for the feedback, @peterhinch!

I provide access in a subclass.

But what is the point to this? Again memory consumption?

I was right this evening driven to make a subclass of FrameBuffer for testing my new Framebuffer.get_rect(x, y, w, h) functionality. The subclass makes the convoluted test (lots of parameter combinations) even more complicated and tests should be simple.

Also it is IMHO not really Pythonic if an Instance property is write only. In any case it is error prone to have state double accounted. And the double accounting of state becomes not better if it is distributed between two languages.

Used in production a subclass will take IMHO more runtime RAM in the python space than the Data access handlers of @theidealist in the C-Space, where the information (w, h, buf, ..) is already stored.

@theidealist
Can you please send some numers to support or dismiss my assumptions.

@peterhinch
Do the access handlers contributing to the already in C stored data so much RAM of code that it is justified press the users to subclass, any time they like to access data that is already stored? Can you please elaborate on this question as a senior Micropython developer.

cetero censeo : Part1
For RAM critical application, which do not need a framebuffer at all, it is no problem to just disable the framebuffer via

undef MICROPY_PY_FRAMEBUF

and compile a lean micropython, without.
For applications that use a framebuffer IMHO RAM is not so critical. If it were the user would use character displays instead.

cetero censeo : Part2
Let us push framebuf and other modules (the extmod category is named so but sadly is not external) out of the micropython core.
This would have two immediate benefits:

  1. The monolithic micropython would be Pythonically modularized
  2. It would effectively prevent such negative discussions which just stall the further creative development by driving away interested developer that like to contribute

Cheers,
Volker

@theidealist
Copy link
Author

Thanks for the response, @volkerjaenisch

@theidealist
Can you please send some numers to support or dismiss my assumptions.

I agree with your assessment (if I understand it correctly), but I'm not sure how to provide numbers to support it. Do you mean you'd like to have numbers that show how much RAM a FrameBuffer subclass in Python that stores the width and height in Python uses compared to a FrameBuffer like mine, which just exposes the already-stored data via accessors? I am new to micropython and so am not sure how to get such numbers.

Let us push framebuf and other modules (the extmod category is named so but sadly is not external) out of the micropython core.
This would have two immediate benefits:

  1. The monolithic micropython would be Pythonically modularized
  2. It would effectively prevent such negative discussions which just stall the further creative development by driving away interested developer that like to contribute

I apologize if this has become a "negative discussion" somewhere. I truly did not mean it as such -- I was only trying to contribute to a project I find immensely useful. I, personally, am using the esp32 port on a few devices with various small displays and was, for what it's worth, delighted that FrameBuffer support was provided "out of the box" and that I only had to get into learning how to build and deploy my own firmware once I was ready. I did not intend to stifle anyone's creativity and am happy to have the pull-request denied if it is displeasing to senior developers. I've got my own fork -- I can always do with that whatever I want (open source FTW!).

@volkerjaenisch
Copy link

volkerjaenisch commented Sep 22, 2021

@theidealist
Sorry for troubling the waters for you. That was not my intend.

Like you I proposed a simple, reasonable extension to Micropython. Like you I was not greeted with open arms but deflection.

Cheers,
Volker

@peterhinch
Copy link
Contributor

I provide access in a subclass.

@volkerjaenisch I was merely stating a fact. Not advocating against doing it in the base class.

I would recommend also providing access to the format.

This surely offers support for doing it in the base class, which I happen to think is a good idea.

@volkerjaenisch
Copy link

@peterhinch
Thank you for clarification.

@theidealist
I do like your PR quite a lot.

Cheers,
Volker

@theidealist
Copy link
Author

Is there anything I can or should do to have this PR approved or rejected? Anyone know an appropriate maintainer to reach out to?

Not trying to be pushy, just curious.

@theidealist
Copy link
Author

@dpgeorge -- I see you've done a lot of the work in the extmod/modframebuf git log. What are your thoughts on this PR?

@theidealist
Copy link
Author

@peterhinch / @dpgeorge -- do you think I should just abandon this PR or is there a chance it will be reviewed and accepted?

@peterhinch
Copy link
Contributor

@theidealist I am not a maintainer, but I think they are busy at the moment. There is quite a queue of pending PR's.

@theidealist
Copy link
Author

@dpgeorge -- do you think I should just abandon this PR or is there a chance it will be reviewed and accepted?

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

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

Successfully merging this pull request may close these issues.

None yet

5 participants