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

Add MKS LCD12864 A/B Controller Support #18120

Merged

Conversation

thisiskeithb
Copy link
Member

@thisiskeithb thisiskeithb commented May 26, 2020

Description

@makerbase-mks introduced the LCD12864 A/B, an updated REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER-sized LCD controller, with a MKS_MINI_12864 pinout & digitally controllable contrast. More info can be found in this AliExpress listing.

Since these share the same pinout as the MKS_MINI_12864, I updated the various pins files so they can be used on those boards as well.

Benefits

Adds native MKS LCD12864 A/B support so users don't have to to define MKS_MINI_12864, which I found unintuitive.

Related Issues

Initial contrast support for these LCD controllers were added in #16163.

@thisiskeithb thisiskeithb changed the title Add MKS LCD12864B Add MKS LCD12864 A/B Controller Support May 26, 2020
@thisiskeithb thisiskeithb force-pushed the pr/add_mks_12864b branch 2 times, most recently from f2ed9ad to 4cfffa3 Compare June 4, 2020 14:35
@thinkyhead
Copy link
Member

Apart from skipping all the initialization stuff, what other firmware differences between the MKS_MINI_12864 and MKS_LCD12864 are there? Because if there are no other real differences, then it a bit cleaner to keep MKS_LCD12864 as a sub-type of MKS_MINI_12864.

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Jun 5, 2020

Apart from skipping all the initialization stuff, what other firmware differences between the MKS_MINI_12864 and MKS_LCD12864 are there? Because if there are no other real differences, then it a bit cleaner to keep MKS_LCD12864 as a sub-type of MKS_MINI_12864.

None that I'm aware of & testing one with MKS_MINI_12864 defined worked fine. MKS's instructions include defining MKS_MINI_12864 for these, so if there's an easier way to do it, then there shouldn't be an issue.

I just wasn't sure how far I needed to take it or if I could do something like (which would have greatly simplified this PR):

#ifdef MKS_LCD12864
  #define MKS_MINI_12864
#endif

@thinkyhead
Copy link
Member

Typically a sanity-check would be added to alert users that they need to change the LCD type. However, that is tricky here. Three boards insisted that whenever MKS_MINI_12864 is used, the init is mostly skipped. Meaning, for these boards you couldn't use a proper MKS_MINI_12864, only these special models, unless you uncommented the lines in the pins files.

So, should those pins files throw an alert for this LCD type and tell users to change it…?

@thisiskeithb
Copy link
Member Author

So, should those pins files throw an alert for this LCD type and tell users to change it…?

I’ll have to check and get back to you on that.

@thisiskeithb thisiskeithb marked this pull request as draft June 5, 2020 04:27
@makerbase-mks
Copy link
Contributor

Normally, MKS LCD12864A/B screen type can be directly defined as MKS_MINI_12864 for use, but the driver chip is still different.

  1. MKS_MINI_12864 uses ST7565R, but MKS LCD12864A/B uses ST7567.
  2. The appearance of MKS LCD12864A/B and MKS_MINI_12864 are very different
  3. In order to facilitate maintenance in the future, i think, the two are still distinguished according to the @thisiskeithb opinion. So, never affect MKS_MINI_12864 itself's use

@thisiskeithb thisiskeithb marked this pull request as ready for review June 13, 2020 02:24
@thinkyhead
Copy link
Member

If users have one of the boards that previously converted MKS_MINI_12864 into LCD12864A/B automatically, those users might still have MKS_MINI_12864 as their enabled LCD. Since this will no longer work for them, perhaps we should throw a #warning or #error in the affected pins files to remind them to check whether they have a real MKS_MINI_12864 or something else.

Current example configs that use MKS_MINI_12864 are:

  • CR-20
  • Fysetc F6 v1.3
  • Tevo Michaelangelo
  • Tevo Tarantula Pro

@thisiskeithb
Copy link
Member Author

If users have one of the boards that previously converted MKS_MINI_12864 into LCD12864A/B automatically, those users might still have MKS_MINI_12864 as their enabled LCD.

Did we do that when when separated ENDER2_STOCKDISPLAY from MKS_MINI_12864?

@thinkyhead
Copy link
Member

Did we do that when when separated ENDER2_STOCKDISPLAY from MKS_MINI_12864?

Since the MKS_MINI_12864 was not being checked for and treated special by a certain grouping of pins files before it was split off into a separate option, it didn't come up as a question. And only pins_BTT_SKR_V1_3.h ends up doing anything special with ENDER2_STOCKDISPLAY.

@thisiskeithb
Copy link
Member Author

Current example configs that use MKS_MINI_12864 are:

CR-20
Fysetc F6 v1.3
Tevo Michaelangelo
Tevo Tarantula Pro

None of those example configs/printers came with an MKS LCD12864 and use a real MKS_MINI_12864, so I think it'd be safe to leave those as-is without any warnings or errors.

@thinkyhead thinkyhead merged commit 2603a23 into MarlinFirmware:bugfix-2.0.x Jun 16, 2020
@thisiskeithb thisiskeithb deleted the pr/add_mks_12864b branch June 16, 2020 08:34
@anomalchik
Copy link

Tevo Tarantula Pro with MKS_MINI_12864 have build error:
avr-gcc: error: CreateProcess: No such file or directory
On the stage "Linking everything together..."
If i disable MKS_MINI_12864 in configuration then build succeeds

@sjasonsmith
Copy link
Contributor

That is usually a path-length issue when using the Arduino IDE on Windows. This is a frequent topic that comes up in the Marlin Discord, and is extremely unlikely to be directly related to this change.

For best results getting help with configuration and troubleshooting, please use the following resources:

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Jul 5, 2020

@anomalchik: Your file path is likely too long. Try moving Marlin to the root of your hard drive. I just built the latest bugfix-2.0.x with the default Tevo Tarantula Pro config from the Marlin Configurations repo and it built fine:

Linking .pio/build/mega2560/firmware.elf
Building .pio/build/mega2560/firmware.hex
Checking size .pio/build/mega2560/firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [======    ]  58.7% (used 4807 bytes from 8192 bytes)
Flash: [=====     ]  51.4% (used 130612 bytes from 253952 bytes)
============================ [SUCCESS] Took 35.51 seconds ============================

Environment               Status    Duration
------------------------  --------  ------------
mega2560                  SUCCESS   00:00:35.512
mega1280                  IGNORED
MightyBoard1280           IGNORED
MightyBoard2560           IGNORED
rambo                     IGNORED
[snip]

@anomalchik
Copy link

anomalchik commented Jul 5, 2020

I have marlin-bugfix-2.0.x in D:/ drive root
And i have stable marlin-2.0.x also at the root D:/
Bugfix branch doesn't build, stable build succeful.
Unfortunately, I have not watched the changes in the bugfix branch for a long time, but early bugfix branch builded from this directory.
Why this happens when enabled MKS_MINI_12864? Actually it made me look for the latest edits regarding this display

Maybe it's time to get away from Arduino IDE

jmp0x0000 pushed a commit to jmp0x0000/Marlin that referenced this pull request Aug 7, 2020
HairingX pushed a commit to HairingX/Marlin that referenced this pull request Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants