PyDevices / pydisplay

Display, touch and encoder drivers for MicroPython, CircuitPython and Python
Other
34 stars 9 forks source link

I like to contribute a driver for a Waveshare E-Ink Display #9

Closed volkerjaenisch closed 2 months ago

volkerjaenisch commented 5 months ago

Just stumbled over this project tonight. Looks really promising.

Some background

Questions

Cheers, Volker

bdbarnett commented 5 months ago

Hi Volker,

I look forward to your contributions! If you can, please grab a 16-bit LCD display with either an ESP32-S3 or RP2040 built in so you can get to know how MPDisplay works with LCDs before you try to make it work with E-Ink. Make sure the bus is either SPI or i80 parallel, NOT RGB or I2C.

The documentation should be much easier to follow, and much more accurate, this time next week!

Thank you very much!

volkerjaenisch commented 5 months ago

Dear Brad!

I dug a bit into the MPDisplay code and have some questions.

The st7701 driver looks quite similar to my code. There is TX functionality defined _tx_cmd and _tx_data. But in the base class BusDisplay these functions are not required? Other drivers like st7789 not even implement them.

So I do not have found a real API that has to be implemented for a "driver".

From my naive view there should be like four levels of abstraction/API: 1) Pins, Interfacing 2) Low level communication: command/data 3) Drawing functions: Blit, clear, fill, copy, framebuffers 4) High level operations like lines, circles, fonts, scrolling, etc.

But I am not able to identify them in your project. This may be because probably we have chosen a different approach: Me coming from the wires, you coming from the platforms.

"My" code which is ported from Waveshare demo codes has all the features I have seen in your code.

So there should be no problem to port the code to MPDisplay - but I need help find the API I have to implement.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear Brad! An additional remark. The EInk-Displays are splitting pixels in low an high information.

See "Programing Principle"

So a specialized frame buffer could bring additional performance. A simple Black/White picture could be two times faster that a gray scale one.

This targets your "Area" approach. If two "Areas" are added the combination is the bounding box of both "Areas". If there are two minimal Drawing events wide spread (One Character left/down, One Character right/top) the bounding box becomes large and the drawing will be really slow, since the complete screen is redrawn.

On LCD/TFT this does not matter but on E-Ink it is important.

So it may be more efficient for E-ink displays to store the list of individual areas of change than to expand them to their common bounding box. This breaks with the common frame buffer approach.

I will dig into this problem and do some measurements.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear Brad!

I did a measurement and the outcome is conclusive: A more sophisticated frame buffer is not needed. The E-Ink display is in fact a frame buffer. You write into it and if you refresh it the frame buffer will become active.

Cheers, Volker

bdbarnett commented 5 months ago

Hi Volkers,

The ST7701 driver is a bit misleading. The board I developed it for is the t-rgb_2.1in_full_circle. On that board, the display is connected by a 16-bit RGB bus. RGB buses only carry pixel data. All setup register accesses have to be done through an additional SPI connection. Due to strict timing requirements of RGB bus, I don't think it is possible to implement an RGB bus connection in Python. @kdschlosser is writing ESP32 bus drivers in C as part of his work on lvgl_micropython and I plan to make BusDisplay communicate with that bus when he has it finished. The _tx_data and _tx_cmd in the ST7701 driver are only for register access / setup / configuration, not pixel data. I recommend staying away from RGB bus (for now) because they don't behave like other displays. The don't have Graphics RAM (GRAM) to use as an internal framebuffer like SPI and i80 do.

The layers of abstraction you mentioned are very similar to what I have implemented, although I have blurred the lines between item 3 and item 4. This is how they are implemented:

I suspect the reason you had trouble identifying how things are implemented in my code is not because of a "naive view", you obviously know what you're talking about, but because I chose to leave the lcd_bus bus drivers out of MPDisplay and make them a separate repository. I did that to make it clear to any possible contributors that they aren't the only bus drivers, or even the best bus drivers, that will work with MPDisplay. I very much would like for community members to create their bus drivers written in C that take advantage of the SDK's from specific chips, like the STM32H7 or MIMRTX.

Another issue that may have made my code difficult to understand is that the methods that do the heaving lifting, self._tx_param and self._tx_color, are assigned, not defined. See this section of code:

https://github.com/bdbarnett/mpdisplay/blob/main/lib/mpdisplay/_busdisplay.py#L199-L207

You said "but I need help find the API I have to implement." Your display seems to have the same specs as this one from Adafruit. That module has an IL0373 chip. Start with that driver and edit it, replacing displayio with mpdisplay. Then create a subclass of either _BaseDisplay or BusDisplay (whichever you think makes more sense) as EPaperDisplay and add the missing methods that are present in all 4 Display types, particularly fill_rect and blit_rect. Your code would mimic displayio.EPaperDisplay. You may find it necessary to split BusDisplay into 2 parts: an intermediate class that has all the code that can be shared with EPaperDisplay and the remaining code becomes the new BusDisplay. EPaperDisplay and the new BusDisplay could both subclass the new intermediate class. I'll leave that to you to implement, but I'm glad to discuss it with you. I just finished 6 or 8 months of work getting here, and I'm switching over to supporting the code, not adding new features, for the time being. Plus I'm going to try to pay more attention to my wife :)

kdschlosser commented 5 months ago

The RGB Bus driver is working for the ESP32 in the bus drivers I wrote. You would not be able to make a bus driver for RGB in Python. It's simply going to be too slow. There is also issues related to the framebuffer being in SPIRAM and using DMA memory coupled with interrupts to read the buffer data for transmitting. Timing is a big issue with the RGB bus and if it is not right you end up with all kinds of artifacts on the display.

I am also going to argue the circuit python being an easier API. With ease of use come assumptions that get made and sacrifices in performance also happen. This is seen in Circuit Python. It has ZERO support for DMA memory buffers and that is a HUGE performance hit. Using double buffering with DMA memory results in the frame rate almost doubling. The assumptions that get made also limit settings a user is able to adjust.

bdbarnett commented 5 months ago

@kdschlosser I agree with you on all accounts my friend. I'm glad to hear your RGB drivers are working. I haven't had any luck getting the board I expected to be the easiest working, the Adafruit Qualia with MicroPython, to display any video. I have several others to try as well, but it will likely be a few weeks before I can try them. I've been watching your development on lvgl_micropython and see you have A LOT going on, so I've just focused on my project. I'll reach out to you if I have any issues when I dig in with those new boards. Thanks!

kdschlosser commented 5 months ago

no worries m8. Have fun getting the SPI working properly as it is a real ball buster. Getting the XPT2046 working when the display is used SPI and they are on a shared bus is a complete joy to have to deal with.

volkerjaenisch commented 5 months ago

Dear Brad!

Having a ST7789v based Display (Waveshare 2inch). Still trying to get the mpdisplay code working.

But I understand your code structure, now. A few questions:

Let's call it mpdisplay.EPaperDisplay. EPaperDisplay will need to subclass mpdisplay._BaseDisplay, and each chip-specific driver will need to subclass EPaperDisplay .

I will do this. Will fork your repository for this purpose.

Cheers, Volker

bdbarnett commented 5 months ago

Hello Volker!

I made the constructor syntax for mpdisplay.BusDisplay the same as CircuitPython's busdisplay.BusDisplay, even though things are very different inside the class itself. I think it would be best to do the same for EPaperDisplay. Here is EPaperDisplay for Blinka, which is their implementation written in Python as opposed to the implementation compiled into CircuitPython written in C. In their implementation, the pin is called busy_pin and it is saved as the class attribute self._busy. So, to answer your question, if you can, please handle it in EPaperDisplay, not SPIBus.

I just created and uploaded st7789vw. I hope it helps you. Let me know if you need help creating a board_config.py.

Thank you sir!

kdschlosser commented 5 months ago

@bdbarnett

as a suggestion, you really do not want to do this...

_INIT_SEQUENCE = [
        (0x36, b"\x00", 0),
        (0x3A, b"\x05", 0),
        (0x21, b"\x00", 0),
        (0x2A, b"\x00\x00\x01\x3F", 0),
        (0x2B, b"\x00\x00\x00\xEF", 0),
        (0xB2, b"\x0C\x0C\x00\x33\x33", 0),
        (0xB7, b"\x35", 0),
        (0xBB, b"\x1F", 0),
        (0xC0, b"\x2C", 0),
        (0xC2, b"\x01", 0),
        (0xC3, b"\x12", 0),
        (0xC4, b"\x20", 0),
        (0xC6, b"\x0F", 0),
        (0xD0, b"\xA4\xA1", 0),
        (0xE0, b"\xD0\x08\x11\x08\x0C\x15\x39\x33\x50\x36\x13\x14\x29\x2D", 0),
        (0xE1, b"\xD0\x08\x10\x08\x06\x06\x39\x44\x51\x0B\x16\x14\x2F\x31", 0),
        (0x21, b"\x00", 0),
        (0x11, b"\x00", 0),
        (0x29, b"\x00", 120),
]

at the module level. The reason why is resident memory allocation. That one variable takes up 3,424 bytes of RAM and because it is not something that is allocated on the stack it stays resident in memory unless you specifically delete the display driver module from sys.modules and also any references to that module that might be in user code. 3,424 bytes is a fairly large amount of memory to get used up on something that in most situations only gets used a single time. This is the reason why I places all of the initialization commands inside of a function. This way when the function finishes and exits the stack memory can get garbage collected freeing it up for use elsewhere.

Another thing is using bytes. This is an expensive data type to use due to it being open ended in size. I would suggest using array.array if you are going to be storing the data in a sequence like that. array.array has a smaller memory footprint. Not by a huge amount but there is a difference.

Also passing bytes, str, bytearray to a function call makes a copy of the data that is being passed. This will eat up quite a bit of memory on the stack. While the stack use gets freed if there is any allocation on the heap that occurs in the function that was called that stack allocation has the potential to cause quite a but of memory fragmentation. When a memoryview gets passed a copy is not made. It is a pointer to where the data is located in memory. When you slice a memoryview a copy is not made instead what happens is it makes a new memoryview object that has a different start and stop point but it still points to the same data as the original memoryview object.

a single byte object stored as a variable takes up 96 bytes of memory. A bytes object that has a length of 14 takes up 128 bytes of memory. 32 additional bytes of RAM used for 13 more bytes of data stored in a bytes object. If we increase the length to 28 you end up with 160 bytes of RAM used. The allocations are done in chunks of 32 bytes because I am running the tests on a 64 bit machine. On a 32 bit machine the allocation blocks would be 16 bytes. so if you allocate one byte into that extra block the entire block gets allocated where as if you use an array each additional byte past the initial byte takes up 1 byte of memory.

volkerjaenisch commented 5 months ago

Dear @all!

Am 14.06.24 um 09:32 schrieb Kevin Schlosser:

Another thing is using |bytes|. This is an expensive data type to use due to it being open ended in size.

Here I like to disagree.

"bytes" are using 40 Bytes internal memory and then exactly one Byte per payload Byte.

from pympler.asizeof import asizeof

asizeof(bytes(b'v'*1000)) 1040

At least in C-Python. I will check in Micropython.

Same here.

gc.collect() mem1 = gc.mem_free() a=bytes(b'v'*50000) gc.collect() mem2 = gc.mem_free() print(mem2)

56688

The different overhead can be from the not really exact measurement via the GC.

Cheers, Volker

--

inqbus Scientific Computing GmbH Dr. Volker Jaenisch Hungerbichlweg 3 +49 (8860) 9222 7 92 86977 Burggenhttps://inqbus.de

kdschlosser commented 5 months ago

cpython is different than micropython. you need to use micropython.

As I have said Micropython allocated in blocks of 16 for a 32 bit MCU. Go ahead an create a byte string having one value then create another having 14 values. You will see the memory allocation jump a number that is not equal to 13.

kdschlosser commented 5 months ago

it even states it right here in the documentation..

https://docs.micropython.org/en/latest/develop/memorymgt.html#allocation-of-objects

kdschlosser commented 5 months ago

https://docs.micropython.org/en/latest/reference/speed_python.html#arrays

volkerjaenisch commented 5 months ago

That is correct. But the discussion started with Constants. If the allocation is only done once bytes are efficient. Arrays are as efficient and memory views a good tool.

kdschlosser commented 5 months ago

bytes have an over allocation of memory for the given array size. it is NOT a 1 to 1. there is going to be a large up front allocation and then there are going to be allocations that occur in blocks and those blocks are going to be 16 bytes in size. as an example if you allocate a bytes object of 1 the size might be 96 bytes and if you allocate a bytes object that has a length of 14 the amount of memory that gets allocated would be 128 bytes. IDK about your math but mine works out to an additional 32 bytes of allocation for 13 additional bytes stored in the bytes object. That's not a 1 to 1... This is running MicroPython on Unix but the same applies if it is running on an MCU and the allocation size will differ based on what the bitness the MCU has. 64 bit = 32 byte allocation blocks and 32 bit MCU = 16 byte allocation blocks.

It states right in the "Memory Management" section of the MicroPython documentation that array.array is a more memory efficient way of storing bytes than using a bytes object.

This is neither here nor there. The fact that the initialization sequence is being set as a variable at the module level means it stays resident in memory the entire time the application is running. Doing this is using memory to store data that gets used a single time and that memory never gets released so the application is able to use it. The data stays in RAM the entire time the program is running unless the user adds specific code to delete it.

There are some places where the initialization data is not stored as a variable at the module level. Instead it is done inside of a function so when it is no longer being used it is able to get garbage collected.

https://github.com/bdbarnett/mpdisplay/blob/82d65913f9e3ffbed6db7d7988382d403a6d00f2/drivers/display/st7796.py#L32

volkerjaenisch commented 5 months ago

@kdschlosser I got your point. Our misunderstanding comes from the different type of usecase we are addressing. You criticize the use of bytes on module level since this memory is no longer available for the GC. And so you are correct for the initialization "Bytes" for e.g. a LCD-driver.

The Code I am into is for an E-Ink-Display. There are lots of constant LUTs (Lookup-Tables) (each 160Bytes long) that MUST persist till the end of the program, since they are used frequently (in contrast to the initialization bytes which are used only once). There are some approaches to not using these static LUTs but to generate them from a parameterization e.g. in the Good-Display.com reference drivers, but there is a CPU penalty in generating the LUTs any time the display is refreshed.

I think for E-Ink-Displays a high level approach for memory management of the LUTs could be the best solution. This means that only the LUTs are kept in Memory which are actually used. There are e.g. different LUTs if you like to refresh the display fast or slow or if you refresh it in grayscale of BW mode. The user may select via driver parameters which display mode(s) he like to use and only these LUTs are loaded persistently.

Also since E-Ink-Displays are used to maintain there state for minutes it may be an approach to actively flush the LUTs out of RAM. But this functionality should be an optional feature since it depends strongly on the usecase if this is beneficial.

I am sure that there lies much performance testing ahead :-).

Sorry for the misunderstanding.

Cheers, Volker

bdbarnett commented 5 months ago

Hey Kevin,

I completely understand what you are explaining. You and I have had this discussion at least once before. I'm not going to do the math with you, but I get it. However, BusDisplay has the ability to be initialized 3 ways: with the init string as a list of tuples as above, with the init string as a single bytearray the way CircuitPython does it, or with a function such as the init() method you linked to in the st7796.py file I modified from you. That is to say, the plumbing is there to do it your way, my way, and Adafruit's way. Whomever creates the display driver, me in the case of the st7789wv.py that I made last night, has their choice on the way to implement it. Whomever uses the display driver has their choice as to use it as is, or rewrite it the way the want. If you don't like the way it is written, rewrite it and submit a pull request. The drivers are exchangeable pieces of the puzzle, not part of the infrastructure that can't be replaced, so right now my focus is NOT on init strings. Maybe later.

I'd like to ask 2 things of you if don't mind.

  1. If you are going to post to MPDisplay's issue's or discussion board, first please install it, use it and give feedback about IT, not just pick apart the way portions of IT are implemented. If you have something you'd like to contribute, fantastic! If you're not even going to download and test the project, don't chime in on how wrong it is. I'm asking for help with the project, but not that kind of help. You read through my st7789wv.py within hours of me posting it, but have you read my project's README.md since the big overhaul earlier this week? Have you used it?

  2. Please keep the discussion on topic with the thread, and open up a new thread if what you have to say is on a different topic. In this case, the discussion is about Volker implementing EPaperDisplay support, and I happened to have told him in my last post that I created an lcd driver for the board he has. The topic of discussion is EPaperDisplays, not init strings. Of the 19 posts here so far, 7 are yours, and none of yours even mention EPaperDisplays or E-ink. I believe your intentions are good, but it strikes me as rude. From here forward, when you give advice that is off topic, my reply will simply be "Pull requests are welcome."

I've told you before, our projects have different goals and use cases. You and I can work together, but you've got to come at it from the right perspective. I appreciate everything you are doing for the community and look forward to being able to point people toward your mp_lcd_bus bus drivers once you have them unbundled from lvgl_micropython.

Thank you sir.

bdbarnett commented 5 months ago

Hi Volker,

I am more concerned with getting the API nailed down early than I am with implementation specifics. If you tackle LUTs one way now but decide it's better to refactor that code sometime down the line, that is no problem at all.

As you know, API's are difficult to change without potentially causing lots of headaches. That's why I chose to use CircuitPython's API for buses and display drivers. It may not be exactly what I would have done myself, but it is both good and complete.

Don't worry too much about whether your implementation is the best it can be from day one. My guess is it will be, but if it's not, it can always be revised. Just start with the way that makes the most sense to you.

Thank you sir! Have a great weekend!

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

You are so right:

"""Premature optimization is the root of all evil""" Sir Tony Hoare

But after more than 40 years of programming the mind is always faster than the typing.

I will mind you and start right now with the implementation. Optimization may follow.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

First goal! My Driver can utilize your SPIBus Class, without changes on your code.

Going to implement the EPaperDisplay Class, now. This will take longer.

Cheers, Volker

bdbarnett commented 5 months ago

@volkerjaenisch Fantastic!

volkerjaenisch commented 5 months ago

@bdbarnett I am unsure how to proceed. I started copying BusDisplay to EPaperDiplay and modified it. But BusDisplay is not a true abstract BaseClass and therefore it contains a lot of code which I have had to comment out. This does not support readability.

A new abstract EPaperDiplay BaseClass from scratch may be a better approach. I would keep the API of BusDisplay __init__ (extended by some more parameters), and implement the member functions that are needed for compatibility from scratch.

What is your opinion?

Cheers, Volker

volkerjaenisch commented 5 months ago

@bdbarnett Weird problems on the SPIBus Class. I have two versions of my driver.

One (lets call in NEW) utilizing your SPIBus Class and one (lets call it OLD) using machine.SPI directly.

Both drivers work after a reset of the device and producing exactly the same output.

But restarting the NEW Driver does nothing. While restarting the OLD works.

And it becomes weirder. After restarting the OLD driver the NEW driver works for one time and is stuck again.

I traced the data that goes to the SPI and it is exactly the same on both drivers.

I also checked the parameters. Both drivers result to the same SPI setup.

Before I go into debugging the timings - it may be good to have a night of sleep.

Cheers, Volker

bdbarnett commented 5 months ago

Hi Volker,

In my testing, I always reset the microcontroller before restarting SPIBus with import machine; machine.reset(), which is a workaround to the problem. I just created .deinit() methods in _basebus.py and _spibus.py and committed them. Hopefully after pulling the changes you'll be able to call .deinit() and the SPIBus object between runs and have it work. Since you already have it working with your OLD drivers, we can implement that instead.

Thanks!

volkerjaenisch commented 5 months ago

@bdbarnett If it were that simple ... The OLD driver does nothing different than the NEW one. The only difference may be some timings.

Surely I can reset the machine while testing, but this does not solve the problem.

On a long running task the NEW driver will get stuck.

Cheers, Volker

bdbarnett commented 5 months ago

Hi Volker,

I'm sorry I don't have a suggestion, but I'd be happy to look at your code if you want. Maybe your implementation of SPIBus is better than mine!

On a different note, you don't have to worry with implementing everything in BusDisplay right away. Just implement .__init__(), .init(), .blit_rect(), .fill_rect() and .show(). .show() isn't used in BusDisplay because it isn't needed with LCD displays. FBDisplay has only those methods along with .refresh(), which is just an alias for .show(). .init() is called from .__init__() and then every time the rotation is changed. Until you implement rotations, .init() can just pass.

I hope you got some sleep!

volkerjaenisch commented 5 months ago

Dear Brad, my savior!

I just created .deinit() methods in _basebus.py and _spibus.py and committed them. Hopefully after pulling the changes you'll be able to call .deinit() and the SPIBus object between runs and have it work.

Yes, the problem was the dangling SPI-bus. I have no clue why this did not affect my old driver , also. The old driver has no notion of deinit the SPI. Probably a timing thing.

Anyway the new driver works reliable now with your SPIBus. Puh!

I will implement an abstract Baseclass EPaperDisplay and then a minimal WS2in9V2 (Waveshare 2.9inch V2 Display) driver for basic operation (According to your list above), from scratch.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett

I am a bit unhappy with the overall repository structure. IMHO a project of this size needs a good IDE like PyCharm to work effectively. But this requires some structure and code organization.

I will reorganize the repository in my fork and also make the code a package with Poetry.

There are a lot of pieces where I have no clue where they should go. E.g. there are RH-tool, utils, binfont.py, bmp565.py, which are all in a sense utils.

Area.py is a really important core data class but it lives not in the core. BTW there is no core, ups. I think there is room for improvement. :-)

If a project should thrive it is of utmost importance that it is easy to use, simple to maintain, and a joy to extend.

Imagine a world where the installation of mpdisplay for a certain board is as simple as

pip install mpdisplay mpdisplay build []

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

Why did you prefix classes and files with an "" ? To declare them protected? But then area.py and a lot of other code should be protected, too. I propose to make the code more readable by omitting leading "" and do not use CamelCase in function names.

Also there are hundreds of PEP8 violations. I propose to use coding guidelines to get a cleaner code base. No worry I like to fix PEP8 violations an volunteer to fix them.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

For the Busdisplay you are using a kind of packed initialization sequence. This I cannot use since the initialization for the e-ink display is not only TX but also waiting on the busy line. Also there are different kinds of initialization sequences for EInk.

I will put the EInk initialization in "init" and give "init" of EPaperDisplay an additional parameter which EInk-Init initialization kind should be used.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

Another question. The driver has to wait for the busy signal before it can send another command. This is IMHO a protocol thing (like an ACK on the RX line) and not a high level thing. We should discuss to push this down into the SPIBus driver. For now I put it in the EPaperDisplay Base Class.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

I had to put the sleep_ms code (mp/cp) from the module into the base class.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

My code is ready for testing. BUT.... I noticed that you have lots of dependencies that are mandatory e.g. the Broker, Shapes Classes.

IMHO not many micro controller people like so much "bloat" just to use a graphics driver. There should be a possibility to strip down the mpdisplay code to the required minimum.

The fundamental problem you are stuck in is that you use (multiple) object inheritance where more modern programing paradigms like components should be used. Object inheritance in the end always result in FAT base classes. But on micro controllers one needs lean bases.

Components decouple objects effectively and break up import dependencies.

I will give the ZCA a try under MicroPython - stay tuned. If this does not work I can implement a simple component architecture from scratch that may solve lots of problems.

You may be astonished HOW a component architecture does all these nice things. It is quite simple. One of the fundamental problems in coding is: Your code is somewhere deeply nested and likes to call the "logger".

In OOP-Code you have only two ways to implement this: 1) One of your ancestors knows the logger 2) The current class knows the logger Knowing the logger means there has been an import of the logger class. So in fact both ways lead to a FIXED import dependency.

In component architecture there is something like a central registry that knows all components. Every part of your code knows the registry, which is one simple dependency.

If you need to get the logger component at any place in your code, just ask the registry to provide it. There is a slight overhead since a call the the registry is more CPU intensive than a direct function call. But the easiest way to deal with that is to cache the component for the time you are using it.

The benefit is that your code becomes decoupled an lean. Hard "load time" dependencies become soft "run time dependencies". A serious change but one can adopt to it.

And my driver code already has dynamic dependencies since e.g. the setting of sleep_ms is no longer at module but on class level.

Cheers, Volker

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

The driver runs, but nothing happens. Will have to debug some time. But first sleep.

Cheers, Volker

bdbarnett commented 5 months ago

Hi @volkerjaenisch!

I appreciate all the feedback AND that you are willing to implement, or help me implement, the changes you recommend. My coding skills are limited, and I could use the help!

I'll let you know my background. I'm 51 and started coding in BASIC on my first computer at about age 10, the Commodore C16 (not a Vic20 or C64). I programmed in BASIC on Apple II's in school and then got my second computer in High School, an 8086 clone with no hard drive. I did lots of batch file scripting on that. I took only 2 programming classes in college in 1990: Prolog and C++ Programming Under DOS. My college degree is electronics, not computer science. I programmed under AT&T Unix in a proprietary language for electronic test equipment in the military. Since then I have been in IT and the only programming I have done for pay is scripting, whether login scripts for Novell Netware, bash and PHP scripts for maintenance when my company owned an Internet Service Provider, and LOTS of DOS batch file and PowerShell scripting through the years. I got into microcontrollers and (Micro)Python about the time the ESP8266 came out and have been trying to teach myself as much about Python since then.

I wanted to tell you that so you know I'm capable of learning and have decent experience, but I have very little formal training in programming. I suspect this project will outgrow my skillset, if it hasn't already, and I'll need to move it to it's own location on Github, similar to the MicroPython repo being under the MicroPython "company" and turn over day-to-day maintenance to other folks like you with better skills. I have no problem with that at all. I started this project because I saw a need that nothing else had filled. I just want it filled for my own projects and don't care whose code it is.

When you make changes to a file, be sure to put your name in the copyright line. I want you to receive credit for anything you do.

Now, to address the comments you've made since I posted last:

I am a bit unhappy with the overall repository structure. IMHO a project of this size needs a good IDE like PyCharm to work effectively. But this requires some structure and code organization.

I will reorganize the repository in my fork and also make the code a package with Poetry.

I very much welcome your reorganization. I use VSCode, not PyCharm, but the reorganization will be a benefit no matter which IDE is being used.

There are a lot of pieces where I have no clue where they should go. E.g. there are RH-tool, utils, binfont.py, bmp565.py, which are all in a sense utils.

Agreed. After I got the bare implementation working, I wanted to make it work with @russhughes's test code for his st7789mpy_py class, which meant implementing the drawing primitives from framebuf.FrameBuffer and a handful of others that he had created. I put those that he had created in RH-tools as kind of a "don't mess with these or you'll break the examples". For the code to have good organization, and as you mentioned to be a joy to extend, that functionality will likely have to be broken at some point.

I'm not sure anything in the utils folder needs to be part of MPDisplay. They are helper classes that can be used in certain circumstances.

Area.py is a really important core data class but it lives not in the core. BTW there is no core, ups.

I struggled with where to put Area and actually put it in 3 or 4 places before dropping it where it currently is. As with everything else, I'm open to changing that.

Why did you prefix classes and files with an _ ?

I did that on class names to indicate the class is not intended to used by the end-user code, but instead to be subclassed first. I also prefixed file names with _ to indicate the same for modules. Rather than from mpdisplay._busdisplay import BusDisplay, I thought it would be better to encourage the user to use from mpdisplay import BusDisplay, and let mpddisplay.__init__.py take care of importing the class and anything else that may need to go along with it. This is particularly useful for DesktopDisplay, which isn't a class itself, but either PGDisplay or SDL2Display can be imported as DesktopDisplay.

I will put the EInk initialization in "init" and give "init" of EPaperDisplay an additional parameter which EInk-Init initialization kind should be used.

Sounds good to me.

Another question. The driver has to wait for the busy signal before it can send another command. This is IMHO a protocol thing (like an ACK on the RX line) and not a high level thing. We should discuss to push this down into the SPIBus driver. For now I put it in the EPaperDisplay Base Class.

I ask you to not change this. Not because it is a bad idea -- it's not a bad idea at all. The reason is that I want MPDisplay to work "out of the box" with CircuitPython's bus classes and Kevin Schlosser's bus classes, neither of which provide for that signal. Since it isn't required on LCDs, maybe that's the way we justify it living in the EPaperDisplay class.

I had to put the sleep_ms code (mp/cp) from the module into the base class.

That works for me.

I noticed that you have lots of dependencies that are mandatory e.g. the Broker, Shapes Classes.

I won't quote the rest of that post here, but you are correct in both your observations and your recommendations. I think your recommendations are exactly what we need, but I just don't have experience with it yet. This will be a great learning opportunity for me.

The reason I inherit from Shapes is so that the display_drv object has all the drawing primitives of framebuf (and framebuf_plus) attached to it. I like this idea because it keeps the API for doing something with a framebuffer the same as doing it with a display object. I prefer to keep this functionality if at all possible, but am open to other ways to implement it. For instance, rather than writing display_drv.hline(...), we might do display_drv.buffer.hline(...). My only problem with that is on LCD displays we don't (by default) draw to a buffer, but instead draw directly to the display's GRAM.

I'm open to having a Teams or Zoom call if you want. 12pm Noon my time is 7pm your time. I'd be glad to spend my lunch hour talking about this project!

I can't thank you enough for the time and effort you are putting into this. It's probably a lot more involved than you initially thought. Again, make sure you put your name in the copyright on any file you change. Also, I suspect you are much more comfortable and experienced with Git than I am, so let's discuss moving MPDisplay away from my account to a Github company or team account. I'm open to a name change, too, to reflect that it isn't just for MicroPython anymore, and renaming it during the move may be the best time to do it.

volkerjaenisch commented 5 months ago

Dear @bdbarnett !

Lets exchange email addresses volker.jaenisch@inqbus.de is mine. Send me an email and I got yours.

Cheers, Volker

bdbarnett commented 2 months ago

May circle back to this in the future.