Move away from defines

[mcer12]

In particular board revision, display type and display resolution.

I think using defines / esp-idf settings wasn't the best way to make the library portable. In arduino IDE it's a huge headache to make it work. In platformio it's fine but still little overly complicated for a layman. What I see as a better approach would be selecting the display type and board version in epd_init. This would work on all platforms, there would be no need to go in menuconfig or add custom boards in arduino ide. Similar to how most other libraries work like GxEPD or u8g2

Proposed solution would look like this:
epd_init(EpdLutOptions lutOptions, EpdDisplayOptions displayOptions, EpdBoardOptions boardOptions);

So something like this:
epd_init(EPD_LUT_64K, ED097OC4, EPDIY_V5);

I am also thinking about adding an override for display resolution so if you have an unusual display, you can use this:
epd_init(EpdLutOptions lutOptions, EpdDisplayOptions displayOptions, int displayWidth, int displayHeight);
instead of digging in the library.

What do you think @vroland @martinberlin

[martinberlin]

Hi @mcer12, in my humble opinion if this could be an alternative I think it could work fine, not sure if it's possible to do it alternatively while maintening the existing way. Below my thought's about it.
The problem I see if the epd_init is updated so it works only in this new way is that it breaks backwards compatibility (BC) with all what is already built on top of this library, including existing examples.
The idf.py menuconfig has also the benefit that some options can turn different menu's on and off, like currently happens with V6 board (VCOM option appears only when this board is selected)
And also, I think, Kconfig offers an easy way to see what defines are being set. Since I see all people that implemented this component using Platformio correctly set the Board and Display options.
@vroland It would be possible to mantain the current options to avoid breaking BC with all the rest, while offering alternatively an option to do an custom epd_init ?

That way maybe it could be easier in the cases mcer mentions while not breaking existing working Firmware's that are build with EPDiy. I've been always a bit scared of making updates if they break BC because it can potentially break existing Firmware (Thinking for ex. about Platformio dependencies, where EPDiy is "picked up" and installed in the lib_deps folder automatically)
Post note: Also width / height defines are used on many different parts of the codebase, and imho, they should be defines that are set "per display" since every display has an unique width/height. In case there is a display variation it should be simply defined as a new display with it's new width / height. If this moves away from defines actually a big part of the component has to be rewritten. And tested in each board and for each display to confirm it works as before.

[mcer12]

Yes, this is a breaking change but changing the sketch takes below a minute no matter how complicated, it's a minor annoyance really. We can always add a custom compiler error with a notice that there's been a breaking change with a link to wiki. I think in the end, it would simplify library usage for the user. Epdiy is just difficult to use at this point, especially with arduino ide.

I think you don't see it from the view of users that don't use or are not familiar with the esp-idf. Me personally, I hate it and I don't want to mess with it unless I really need to. For those users the library usage is overly complicated and it doesn't have to be. The proposed changes wouldn't make it any more difficult for esp-idf users but it would make it much more useful for others.

As for your VCOM note, that can be easily added as another (optional) parameter in init.

[mcer12]

@martinberlin As for the width / height defines, they definitely shouldn't be defines in my opinion because there are many display models that aren't officially supported but can work easily with preexisting profiles, all that needs to change is the resolution. You can see this in this issue:
#70
Currently, if you're using unsupported display, you have to dig in the library and don't forget that each time there might be an update of EPDIY, you will have to edit again. How annoying :D IMO this is a nobrainer.

And don't get me wrong, I understand this requires a significant changes to the library. But again, IMO, this is long overdue change that simplifies the library for everyone and makes it nice and portable.

I definitely want to make this change for myself so the question is, do we collaborate on the best way to do this or do I make a fork and this library remains esp-idf exclusive?

[martinberlin]

I just give my opinion but the one that has to ultimately decide is @vroland . I will adapt to the decision and go forward, and of couse help testing and implementing it if possible.
I use both IDF / Platformio with arduino-esp32 firmware is not that I'm exclusively using one Framework. Of course I've would be happy if any update is making this to be easier to use, but just pointing out my honest feedback about what it will take a change like this.

update: FreeRTOS has been ported to many different MCUs and different systems, maybe it would be nice to check their porting guide
https://www.freertos.org/FreeRTOS-porting-guide.html

In my opinion also GxEPD is not an example of nice configuration. It’s quite messy and examples usually have many different configuration files that you don’t really know how to use.

[mickeprag]

I have been thinking of this for a while and more hardware abstractions is needed imho. As of now it is not easy creating a new board if the signals are not routed on the same pins or the same port expander is used. A new board must patch the epdiy library which is not so portable.

I think this is doable without breaking too much existing code if done correctly. Unfortunately I have not had the time to prototype something yet but hopefully will find some time soon.

Extending epd_init() is not the best option in my view. Just as @martinberlin says it will break too much existing code. Keeping a stable API must be a high priority.

My 2 cents is instead:
Keep the current defines with menuconfig. If one likes to use these with the defines then thats fine. But add another option to the EPD_DRIVER_BOARD_REVISION option: EPD_BOARD_CUSTOM. Chose this option if you are not using any of the upstream supplied boards. Not defining a board (as if the library is used in Arduino or PlatformIO) this option should be the default value.

Add new function:
void epd_set_board(...) that should be called after epd_init() to tell epdiy which board to use.
In epd_init() the function could end with something like this:

void epd_init(...) {
   // epdiy init code

#if defined(EPD_BOARD_REVISION_V6)
  epd_set_board(EPD_BOARD_V6);
#elif defined(EPD_BOARD_REVISION_V5)
  epd_set_board(EPD_BOARD_V5):
#else
  // No board defined, do not setup the board. The application must explicitly call epd_set_board() itself
#endif
}

This way all old code should keep working since the board will be set using defines, just as today. But new projects will have the possibility to set this in code instead.

And the argument for epd_set_board() is a discussion for later. I have some ideas there as well... ;)

[mickeprag]

The idf.py menuconfig has also the benefit that some options can turn different menu's on and off, like currently happens with V6 board (VCOM option appears only when this board is selected)

Regarding this option. This is only good if your firmware targets one single board. If you want to make a generic firmware targeting multiple boards this value should be read from flash and not built during compile time. In my view this option is just plain wrong to have in menuconfig.

[mcer12]

@mickeprag Sorry but I don't see how this would work to be honest :) To the contrary, this seems like another reason why to move away from defines. The GPIOs are now set via defines as well, if we define them as global variables instead, we can very easily add a function for custom pin mapping, you will even be able to remap the pins on the fly!

[mcer12]

@mickeprag VCOM can't be read from flash at this point, if you want to discuss this, let's create a separate issue.

[mickeprag]

@mcer12 My long term plan is to move away from gpio pin mapping using defines as well. This was just the first step... ;)
I am not proposing pin mapping as global variables...

The defines in my example was only to not break existing code. If this was a new project, it should not be there. And can be removed when the library goes to version 2.0 and breaking changes are acceptable.

@mickeprag VCOM can't be read from flash at this point, if you want to discuss this, let's create a separate issue.

Exactly my point! I don't need to discuss this one here. I was just giving my opinion, just as yours, that defines should go away...

[mcer12]

Extending epd_init() is not the best option in my view. Just as @martinberlin says it will break too much existing code. Keeping a stable API must be a high priority.

Well, on the backend, it for sure will be a significant rewamp of the code but in the user sketch, it's just a matter of adding board version and display model in the init function. No biggie. But there's other ways of setting it up of course, let's wait for @vroland see what he thinks.

[martinberlin]

I'm also curious to know what will be the best resolution for this. Ultimately this pins down on how developers use the component, and what are the needs in your particular configuration. What is good for some might be not ideal for the others. That's why I would vote for a way to keep what is already done and have an alternative way to configure it. Ideally EPDiy should be the working core and configuration should come from the outside. But this is a task hard to achieve when you have different boards in a component that is evolving constantly.
As I pointed up there, since I didn't want to make this Issue larger, maybe looking at FreeRTOS portability would be a nice example.
FreeRTOS has been ported to many different MCUs and different systems, maybe it would be nice to check their porting guide
https://www.freertos.org/FreeRTOS-porting-guide.html
For me the essential part of making something portable is as first step, that is good documented, and then you also need a "porting guide" in case someone wants to make his own board.

[vroland]

In general, I agree that it would benefit portability and ease of use if we'd use less compile-time defines. So far, I see three
points where that would need to be looked into:

• Backwards compatibility: As you mentioned, we could either break backwards compatibility or require API changes. I think neither would be a big problem. In the case of API changes, we'd bump the library version to 2.0 which should avoid people automatically pulling the new version for existing projects. At least if semantic versioning is done right in platformio.
• To handle pin confingurations, etc. dynamically, we'd need some global config structure that is accessible or passed everywhere. This would require some refactoring.
• It should be investigated whether using global variables for thinks like resolution hurts performance. Right now, the copiler may optimize some waveform calculations based on the resolution being divisible by 4. Not sure if it's a problem, but should be looked into.
• Finally, we need a clean way to replace board-specific #ifdefs, e.g. for the config registers. Right now, only the appropriate header is compiled. This would have to change such that the appropriate methods can be used at runtime.

So, there is quite some refactoring to do. As of now, I'm really busy with my thesis so I'm not going to have time to work on this
or do major reviews. If it can wait a while, we can first discuss the above points in this issue. I also have no problem with someone else working on this, but we should still discuss design descisions together.

[martinberlin]

Thanks Valentin,
regarding the backward compatibility point I just made some tests:

In the case of API changes, we'd bump the library version to 2.0 which should avoid people automatically pulling the new version for existing projects

I'm afraid that many developers link the component this way using platformio:

lib_deps =
    https://github.com/vroland/epdiy.git

And then if I come and download the project, which in turn pulls last epdiy that is v2, on a project that was done for version 1 of course will break BC.
As you can see commented here, the way to do it will be to tag for example this current release as v1.
And then you will add it in your library dependencies like this:

lib_deps =
    https://github.com/vroland/epdiy.git#v1

Then independently of what major changes are done in v2, you will still use v1 and it won't break your existing build. But well that should not be an impediment for progress, you have to change a line in your platformio.ini configuration and it will be solved.
If someone proves me wrong, please enlighten me since I didn't found a way yet to make Platformio smarter with library revisions.

I'm also at the moment moving and in a hard moment to test major updates. But I like the ideas and I would say @mickeprag if you have also interesting takes/ideas in hardware abstraction or how to configure this better, please make a Fork and show us how you would do it.
@mcer12 I will help you also working on this updates provided I find some time. I find very important @vroland point that "we should still discuss design decisions together" and have an overall agreement on what is the road to take.

Started making some drawings about this. If anyone wants access we could shape it together in draw.io

Questions & Analysis
What needs to change to inject display model and board dynamically?

render.c - epd_init(Lut, Board, Display)
display_ops.* - Display options
epd_internals.h - There width / height resolution is defined
config_reg_v(2,4,6).h - Config registers where board IOs are defined

[martinberlin]

@mcer12 I will be referencing some commits from my fork just to test ideas and to make a list of what C & header files should be refactored in order to "inject" the initial parameters instead of using defines.
But for the moment I will leave Board defines and IO configs and focus in the Display configuration that looks like already a bit complicated. Then I will think how to make a Board struct once I get more clear all the things that need to be changed.
This will not be a pull request proposal, but a preliminary analysis of what it will take, to make this change. Feel free to comment / discuss my strategy and ideas.

[martinberlin]

There is something that I could not resolve properly and it's that I wanted to create some default definitions for the displays (So Waveform, resolution Width and Height can be set in one line)
@mickeprag told me that one idea is to do it like Espressif Macros also is possible to do it differently.
Nevertheless as waveforms are defined now, I cannot put them in a global definition like this:'

EpdDisplay ED047TC1 = {
   // defined as const EpdWaveform epdiy_ED047TC1 (epd_internals.h)
    .waveform = epdiy_ED047TC1,
    .width = 960,
    .height = 540
    };

The compiler will complain with .waveform: initializer element is not constant.

C 2011 6.7.9 4: All the expressions in an initializer for an object that has static or thread storage duration shall be constant expressions or string literals.

At the moment is possible also to define a custom display like this, which I think is also a good option, since it's easy and you don't need any predefined default settings:

void app_main() {
  EpdDisplay epd_display;
  epd_display.name = "Lilygo EPD47"; // optional
  epd_display.waveform = epdiy_ED047TC1;
  epd_display.width = 960;
  epd_display.height = 540;
   epd_init(EPD_OPTIONS_DEFAULT, epd_display);
  hl = epd_hl_init(&epd_display.waveform);
}

I will try to merge what I did so far with Micke's take on the boards section. Maybe he will find a better way to refactor it. Good thing is that now you can set what display you are using without menuconfig, and it's just 3 additional lines of code. Just drop ideas if you know how to do it better. Next week I will be focused in some other issues that are more important at the moment than moving electronic ink particles up and down ;)

[mickeprag]

Dear all,

I have now worked a couple of nights and made a proposal how different boards can be implemented. I think I have addressed your concerns above, Valentin. If not, please let me know.

It should not break backwards compatibility but still offer an application to supply it's own board configuration without the need to modify any code in epdiy.

Currently I only have access to v5 boards so this is the only board I have tested it with. More tests on new boards is needed. If anyone have other versions please help me test so I didn't break it during the refactoring...

[Yardie-]

This is going in such a cool direction.
Am I right in presuming
support for the high res waveshare e-papers and the m5 would just be a case of adding an new custom board section then defining at the start of the code.

I absolutely love the font flexibility this system allows and I want to use it on all my e-papers.

Even 1bpp e-papers could be used with just a couple of modifications to the fonts.c and the fontconvert.py to allow this.

This could be so cool.

[martinberlin]

Am I right in presuming
support for the high res waveshare e-papers and the m5 would just be a case of adding an new custom board section then defining at the start of the code.

No, you are wrong. There was never a signal in this issue, that this things are going to be added. About M5 I already answered to you in previews issue that is not supported. About Waveshare raw epapers, that have 16 lines, there where never supported.
And for the fonts thema this was never part of the discussion in this Issue.

[Yardie-]

Sorry if I have offended you @martinberlin.

[martinberlin]

Not at all. But I feel that sometimes you just get excited and comment or open an Issue just because.
And I simply reply honestly. Because this is an open source project, if you feel like collaborating, instead of opening Issues with your questions you could simply engage testing pull requests or directly asking questions on the Slack where I invited you.
But opening issues just because something does not work for you, it does not help, it just creates more open things that IMHO do not help a project to go forward. That is just my opinion, not meant to be offensive or anything, I hope you get the idea.

[Yardie-]

Thanks I thought the idea was actually legitimate. No other system offers the flexibility of this one. I have contributed and gone way out of my comfort zone to do so.
But I will keep my ideas on slack.

[martinberlin]

@mickeprag do you think we can push this forward once you have your new test boards ready?

Would be nice if it does not gets stale since it's a very interesting contribution.

[mickeprag]

Yes. I really hope so.

[mickeprag]

My PR has been updated and has been tested with a "v6 clone".

[martinberlin]

Reopening this issue since @mickeprag in his good right, didn't like that the merge squashed the commits in one.

Keep in mind please that this is the current default way to merge offered by Github (Before was not) so if you want your PR to be merged differently say it in advance.

Hard resetting this to previous commit:

git reset --hard 3ffb83c33cf424494c07fcd2715ba9123ffc64d3
HEAD is now at 3ffb83c Add epd_powerdown for lilygo (#179)

Micke please make your PR again and this time we merge it as you like.

[martinberlin]

This was merged! Great work Micke, thanks a lot!
Only thing we need to check is why this is coming in Lilygo Board, I have some time this weekend, so I will take a look. Is coming just as an error at the end but it renders correctly.

I (7442) gpio: GPIO[5]| InputEn: 0| OutputEn: 0| OpenDrain: 0| Pullup: 1| Pulldown: 0| Intr:0 
E (7442) RTCIO: rtc_gpio_isolate(171): RTCIO number error

[Yardie-]

Well done Micke @mickeprag.

[martinberlin]

Great this is done, merged, and I think we can close here. I'm not sure if my display part is going to be ever merged and I think needs some rounds of though more specially after this more important part is done.
Let's discuss it on the epdiy Slack channel.
Thanks a lot Micke, that was a very important step in getting this right!