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

Refactor EInkDisplay #3299

Merged
merged 6 commits into from
Feb 28, 2024
Merged

Conversation

todd-herbert
Copy link
Contributor

@todd-herbert todd-herbert commented Feb 27, 2024

What

In consultation with @markbirss,

  • Merges some variant-specific code
  • Shifts macros to respective variant.h files
    Shifts macros to respective platformio.ini files
    Previously tested displays documented at top of EInkDisplay2.cpp
  • Change some global variables in EInkDisplay2.cpp to members of the EInkDisplay class
  • Purges all "dynamic refresh" code

Why

General tidiness, but also in preparation to re-implement "dynamic refresh" as a derived class of EInkDisplay (easier to maintain, easier to scrap).
More tidying could probably be done. The variant-specific code in connect() has been completely avoid, for fear of breaking something important in some horrible way.

Who this might affect

Heltec Wireless Paper V1.0 reverts to using the stock "full-refresh" E-Ink system of updates.

Minor refactoring affects a large amount of boards. In theory, no changes should be noticed. I'm reasonably sure that everything went into the right variant.h files, but it would be good to get a second pair of eyes on this.

@todd-herbert todd-herbert marked this pull request as draft February 27, 2024 04:30
@todd-herbert
Copy link
Contributor Author

Something spectacular going wrong with one #include. I'll get back to you.

@todd-herbert todd-herbert marked this pull request as ready for review February 27, 2024 06:28
@markbirss
Copy link
Contributor

markbirss commented Feb 27, 2024

Something spectacular going wrong with one #include. I'll get back to you.

@todd-herbert

My apologises, I see what is happening I think, in our discord conversation I should have said to include EINK_DISPLAY_MODEL, EPD_HEIGHT and EPD_WIDTH in each variants platformio.ini. (not variant.h)

Eg ESP32-S3-Pico variant

platformio.ini
[env:ESP32-S3-Pico]
-DEINK_DISPLAY_MODEL=GxEPD2_290_T94_V2
-DEPD_HEIGHT=128
-DEPD_WIDTH=296

variant.h
removed

@todd-herbert todd-herbert marked this pull request as draft February 27, 2024 12:40
@todd-herbert
Copy link
Contributor Author

Something spectacular going wrong with one #include. I'll get back to you.

@todd-herbert

My apologises, I see what is happening I think, in our discord conversation I should have said to include EINK_DISPLAY_MODEL, EPD_HEIGHT and EPD_WIDTH in each variants platformio.ini. (not variant.h)

Eg ESP32-S3-Pico variant

platformio.ini [env:ESP32-S3-Pico] -DEINK_DISPLAY_MODEL=GxEPD2_290_T94_V2 -DEPD_HEIGHT=128 -DEPD_WIDTH=296

variant.h removed

Oh right, I see what you meant. I'll make the changes tomorrow.
Do you strongly prefer EPD_HEIGHT over EINK_HEIGHT?

@markbirss
Copy link
Contributor

@todd-herbert

We should use consistent names eg
EINK_DISPLAY_MODEL
EINK_WIDTH
EINK_HEIGHT

todd-herbert added a commit to todd-herbert/meshtastic-firmware that referenced this pull request Feb 28, 2024
@todd-herbert
Copy link
Contributor Author

todd-herbert commented Feb 28, 2024

Getting a build error for meshtastic-diy-v1, but seems related to 4ffb906

@todd-herbert todd-herbert marked this pull request as ready for review February 28, 2024 03:46
Copy link
Contributor

@markbirss markbirss left a comment

Choose a reason for hiding this comment

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

changes look good, just update the variant.h files as we only need them defined in platformio.ini not both

variants/m5stack_coreink/variant.h Outdated Show resolved Hide resolved
variants/monteops_hw1/platformio.ini Outdated Show resolved Hide resolved
variants/rak10701/variant.h Outdated Show resolved Hide resolved
variants/rak4631/variant.h Show resolved Hide resolved
variants/rak4631_epaper/variant.h Outdated Show resolved Hide resolved
variants/rak4631_epaper_onrxtx/variant.h Outdated Show resolved Hide resolved
variants/t-echo/variant.h Outdated Show resolved Hide resolved
variants/Dongle_nRF52840-pca10059-v1/variant.h Outdated Show resolved Hide resolved
variants/m5stack_coreink/platformio.ini Outdated Show resolved Hide resolved
variants/heltec_wireless_paper_v1/variant.h Outdated Show resolved Hide resolved
@todd-herbert
Copy link
Contributor Author

changes look good, just update the variant.h files as we only need them defined in platformio.ini not both

Oh jeez.. I absolutely did do all that, but somehow I've made a horrible mess of the commit..
Very grateful that you spotted that..

@todd-herbert
Copy link
Contributor Author

Lesson learnt: double check, and then check again. Can't apologize enough for turning what should have been a simple task into a saga.

@markbirss
Copy link
Contributor

Lesson learnt: double check, and then check again. Can't apologize enough for turning what should have been a simple task into a saga.

yes, these things happen

@markbirss markbirss closed this Feb 28, 2024
@markbirss markbirss reopened this Feb 28, 2024
@markbirss
Copy link
Contributor

@todd-herbert t-echo trunk issue ?

@todd-herbert
Copy link
Contributor Author

todd-herbert commented Feb 28, 2024

@todd-herbert t-echo trunk issue ?

Huh.. weird. It has only popped up after this latest commit. I'm out of the house right now but I'll look into it in a little bit.

A lot of variant specific code is merged, with the macros pushed to the respective variant.h files.
"Dynamic Partial" code has been purged, pending a rewrite.
Usage was in a block of variant-specific code, which had been intentionally left untouched.
@thebentern thebentern merged commit 6acc637 into meshtastic:master Feb 28, 2024
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants