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

[1.1.x] Support some more LCD types #7824

Closed
wants to merge 32 commits into from
Closed

[1.1.x] Support some more LCD types #7824

wants to merge 32 commits into from

Conversation

jmdearras
Copy link
Contributor

Added 4 displays

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 2, 2017

All of the example/configuration.h files need to be changed also.

@@ -108,7 +108,26 @@
#define ULTIPANEL
#define NEWPANEL

#endif
#elif ENABLED(CR10_stockdisplay)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are just delays specific to this board when using that display, shouldn't these be (and already are in) pins_MELZI_CREALITY.h ?

https://github.com/MarlinFirmware/Marlin/blob/1.1.x/Marlin/pins_MELZI_CREALITY.h#L60-L63

Copy link
Contributor

@fiveangle fiveangle Oct 4, 2017

Choose a reason for hiding this comment

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

I was looking at something else and saw this, so I think your REVERSE_MENU_DIRECTION might over-rided by Configuration.h without including any ifdef/ifenabled checks (although it mentions only pins.h and Configuration_adv.h):

/**
 * Conditionals_LCD.h
 * Conditionals that need to be set before Configuration_adv.h or pins.h
 */

#ifndef CONDITIONALS_LCD_H // Get the LCD defines which are needed first

To be 100% sure, just put your changes into Conditionals_LCD.h, then compile / load on your printer with and without REVERSE_MENU_DIRECTION set in Configuration.h and ensure it changes. If it does, you should be good so no changes would be needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment on next code block re: define precedence, which may make this okay to keep as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the menu direction

#define U8GLIB_SH1106
#define REPRAP_DISCOUNT_SMART_CONTROLLER
#define NEWPANEL
#define REVERSE_MENU_DIRECTION
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this override in all circumstances for this display ? Doesn't this need to honor REVERSE_MENU_DIRECTION if set in Configuration.h ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at something else and saw this, so I think your REVERSE_MENU_DIRECTION might over-rided by Configuration.h without including any ifdef/ifenabled checks (although it mentions only pins.h and Configuration_adv.h):

/**
 * Conditionals_LCD.h
 * Conditionals that need to be set before Configuration_adv.h or pins.h
 */

#ifndef CONDITIONALS_LCD_H // Get the LCD defines which are needed first

To be 100% sure, just put your changes into Conditionals_LCD.h, then compile / load on your printer with and without REVERSE_MENU_DIRECTION set in Configuration.h and ensure it changes. If it does, you should be good so no changes would be needed.

Copy link
Contributor

@fiveangle fiveangle left a comment

Choose a reason for hiding this comment

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

just a few things i was curious about

// Generic support for SH1106 OLED I2C LCDs
//U8GLIB_SH1106_128X64 u8g(U8G_I2C_OPT_NONE | U8G_I2C_OPT_FAST); // 8 stripes
U8GLIB_SH1106_128X64_2X u8g(U8G_I2C_OPT_NONE | U8G_I2C_OPT_FAST); // 4 stripes
U8GLIB_SH1106_128X64_2X u8g(U8G_I2C_OPT_NONE | U8G_I2C_OPT_FAST); // 4 stripes //bunnyhack
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "bunnyhack" ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

place keeper, sorry, removed!

// Makeboard 3D Printer Parts 3D Printer Mini Display 1602 Mini Controller

//#define MAKEBOARD_MINI_2_LINE_DISPLAY_1602

//
Copy link
Contributor

@fiveangle fiveangle Oct 3, 2017

Choose a reason for hiding this comment

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

Normally new panels are added at the end of the list so that people who are intimately familiar with the Config format can easily find the section start when scanning through it. Regardless, the entry should also match the comment style:

//
// panel name
// link to external info on the panel (ideally reprap.org wiki page)
//
// any notes (optional)
//
//#define PANEL_DEFINE_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 3, 2017 via email

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 3, 2017 via email

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 3, 2017 via email

@fiveangle
Copy link
Contributor

fiveangle commented Oct 3, 2017

There is not necessarily a "right" or "wrong" way, but I did pose some comments and questions in your code. For REVERSE_MENU_DIRECTION, as long as the value in Configuration*.h over-rides the value you added, then it should be fine. If not, it probably should be left out and the user make the change in Config. If you think what you have is correct, Scott can review when he merges.

The other things Roxy and I left you didn't respond to (bunnyhack ? moving new panel to end of list, fixing comment format, updating the rest of the configs, etc). The protocol is to post your PR like you have, and then respond to comments made (you need to go to the website and respond to the review as you cannot easily do it via email).

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 4, 2017 via email

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 4, 2017 via email

@Tannoo
Copy link
Contributor

Tannoo commented Oct 4, 2017

Once you have the config files the way you want... you do have to update all of the example config files to match.

@fiveangle
Copy link
Contributor

fiveangle commented Oct 4, 2017

I suggest leave this open and just fix it.

I was looking at something else and noticed that Conditionals_LCD.h might load before Configuration.h so you may fine there (I commented on that code directly, so take a look).

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 4, 2017 via email

@fiveangle
Copy link
Contributor

fiveangle commented Oct 4, 2017

Fix in your local repo then commit to your "Marlin-wip" branch which you have requested the merge for here. Also, you really should start interacting on the website, as I mentioned... as the email interface is too limiting. Click the "view on github" link, then re-read the info we've put here as a start.

@thinkyhead
Copy link
Member

thinkyhead commented Oct 4, 2017

I hate to sound stupid, but how do I fix it if it's open?

Any commits you add (and push) to your branch will show up here.

@thinkyhead
Copy link
Member

@jmdearras I have no problem using the CR-10 example configurations included in the current "nightly" build of Marlin. Are you having issues with them?

@@ -1146,7 +1146,7 @@ static_assert(1 >= 0
static_assert(1 >= 0
#if ENABLED(ULTIMAKERCONTROLLER) \
&& DISABLED(SAV_3DGLCD) && DISABLED(miniVIKI) && DISABLED(VIKI2) \
&& DISABLED(ELB_FULL_GRAPHIC_CONTROLLER) && DISABLED(PANEL_ONE)
&& DISABLED(ELB_FULL_GRAPHIC_CONTROLLER) && DISABLED(PANEL_ONE) && DISABLED(MKS_OLED13_128x64_FULL_GRAPHICS_CONTROLLER)
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed, unless enabling MKS_OLED13_128x64_FULL_GRAPHICS_CONTROLLER were to define ULTIMAKERCONTROLLER which I don't see that it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got sanity check failure, two displays defined without it.

Copy link
Contributor Author

@jmdearras jmdearras Oct 5, 2017

Choose a reason for hiding this comment

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

#if defined (MKS_OLED13_128x64_FULL_GRAPHICS_CONTROLLER)
 #define DOGLCD
 #define U8GLIB_SH1106
 #define REPRAP_DISCOUNT_SMART_CONTROLLER
 #define NEWPANEL
#endif

and

#if ENABLED(PANEL_ONE) || ENABLED(U8GLIB_SH1106)
  #define ULTIMAKERCONTROLLER
#endif

it is defined

@@ -1146,7 +1146,7 @@ static_assert(1 >= 0
static_assert(1 >= 0
#if ENABLED(ULTIMAKERCONTROLLER) \
&& DISABLED(SAV_3DGLCD) && DISABLED(miniVIKI) && DISABLED(VIKI2) \
&& DISABLED(ELB_FULL_GRAPHIC_CONTROLLER) && DISABLED(PANEL_ONE)
&& DISABLED(ELB_FULL_GRAPHIC_CONTROLLER) && DISABLED(PANEL_ONE) && DISABLED(MKS_OLED13_128x64_FULL_GRAPHICS_CONTROLLER)
+ 1
#endif
#if ENABLED(REPRAP_DISCOUNT_SMART_CONTROLLER) && DISABLED(REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER)
Copy link
Member

@thinkyhead thinkyhead Oct 4, 2017

Choose a reason for hiding this comment

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

Instead, since enabling MKS_OLED13_128x64_FULL_GRAPHICS_CONTROLLER enables REPRAP_DISCOUNT_SMART_CONTROLLER, this is where you would add the clause.

Then, below you would also add:

   #if ENABLED(MKS_OLED13_128x64_FULL_GRAPHICS_CONTROLLER)
     + 1
   #endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I see. Done

#endif
#define LCD_PINS_RST 27
#define LCD_PINS_DC 25
#endif
Copy link
Member

Choose a reason for hiding this comment

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

These pins haven't been defined yet, so there's no need to #undef them. Just put your custom values in the LCD block below.

@@ -129,6 +129,21 @@
#define MAX6675_SS 66 // Do not use pin 49 as this is tied to the switch inside the SD card socket to detect if there is an SD card present
#endif

/*---------------MKS OLED patch_4-----------------------*/
#if defined (MKS_OLED13_128x64_FULL_GRAPHICS_CONTROLLER)
Copy link
Member

@thinkyhead thinkyhead Oct 4, 2017

Choose a reason for hiding this comment

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

#if ENABLED(...) is required.

#define BTN_EN1 31
#define BTN_EN2 33
#define BTN_ENC 35
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Indentation counts. Please strive to emulate the indentation style used in the rest of the codebase.

@@ -404,4 +461,4 @@
#endif
#endif // NEWPANEL

#endif // ULTRA_LCD
#endif // ULTRA_LCD
Copy link
Member

@thinkyhead thinkyhead Oct 4, 2017

Choose a reason for hiding this comment

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

Remove this space.

}

#if defined (MKS_12864OLED)
SET_OUTPUT(LCD_PINS_DC);
Copy link
Member

Choose a reason for hiding this comment

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

Also, this is the only place this pin is used. Does this SET_OUTPUT do something special that the display needs, even though the pin is never used after this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was in the patch file for the display. I can remove it and see if it still works.

Copy link
Member

Choose a reason for hiding this comment

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

This pin coincides with DOGLCD_A0 — they are both assigned to pin 25. During init I guess this pin takes on a different role…?

@@ -183,9 +183,13 @@
//U8GLIB_SSD1306_128X64 u8g(U8G_I2C_OPT_NONE | U8G_I2C_OPT_FAST); // 8 stripes
U8GLIB_SSD1306_128X64_2X u8g(U8G_I2C_OPT_NONE | U8G_I2C_OPT_FAST); // 4 stripes
#elif ENABLED(U8GLIB_SH1106)
#if ENABLED(MKS_12864OLED)
U8GLIB_SH1106_128X64 u8g(23, 17, 16, 25); // SW SPI Com: SCK = 23, MOSI = 17, CS = 16, A0 = 25
Copy link
Member

@thinkyhead thinkyhead Oct 6, 2017

Choose a reason for hiding this comment

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

What's with the hard-coded pin numbers? According to these additions to the pins_RAMPS.h file, we end up with these pins:

    #if ENABLED(MKS_12864OLED)
      #define LCD_PINS_DC  25
      #define LCD_PINS_RST 27
    #else
      #define LCD_PINS_D5  25
      #define LCD_PINS_D6  27
    #endif

    #define LCD_PINS_RS    16
    #define LCD_PINS_ENABLE 17
    #define LCD_PINS_D4    23
    #define LCD_PINS_D7    29

So this line might be written instead as:

U8GLIB_SH1106_128X64 u8g(LCD_PINS_D4, LCD_PINS_ENABLE, LCD_PINS_RS, LCD_PINS_DC);

Was that your intention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not familiar with your functions and macros. That was the OEM recommendation. If you show me the final code, I'll try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i understand, now. I'll try it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that worked

Works  Not sure about the reset I removed in marlin_main, but the display seems to work fine without it.
@jmdearras
Copy link
Contributor Author

hold off on commit, issue with MKS_MINI_12864 after changing the physical order of things,

@jmdearras
Copy link
Contributor Author

quitting for the night, I am suddenly getting errors on ENCODER_PULSES_PER_STEP and PER_MENU_ITEM, no clue how I broke them.

@jmdearras jmdearras closed this Oct 7, 2017
@jmdearras
Copy link
Contributor Author

I closed it by mistake

@jmdearras jmdearras reopened this Oct 7, 2017
@MarlinFirmware MarlinFirmware deleted a comment from jmdearras Oct 7, 2017
@@ -183,9 +183,14 @@
//U8GLIB_SSD1306_128X64 u8g(U8G_I2C_OPT_NONE | U8G_I2C_OPT_FAST); // 8 stripes
U8GLIB_SSD1306_128X64_2X u8g(U8G_I2C_OPT_NONE | U8G_I2C_OPT_FAST); // 4 stripes
#elif ENABLED(U8GLIB_SH1106)
#if ENABLED(MKS_12864OLED)
// U8GLIB_SH1106_128X64 u8g(23, 17, 16, 25); // SW SPI Com: SCK = 23, MOSI = 17, CS = 16, A0 = 25
U8GLIB_SH1106_128X64 u8g(LCD_PINS_D4, LCD_PINS_ENABLE, LCD_PINS_RS, LCD_PINS_DC);
Copy link
Member

Choose a reason for hiding this comment

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

Changed to…

  // MKS 128x64 (SH1106) OLED I2C LCD
  U8GLIB_SH1106_128X64 u8g(DOGLCD_SCK, DOGLCD_MOSI, DOGLCD_CS, DOGLCD_A0);

@thinkyhead
Copy link
Member

thinkyhead commented Oct 9, 2017

#7867 should now have the right NEWPANEL pins — I had forgotten to set REPRAP_DISCOUNT_SMART_CONTROLLER. If that passes, it might get into 1.1.6. Just putting release notes together now….

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 9, 2017 via email

@ScruffR
Copy link

ScruffR commented Oct 17, 2017

Sorry for commenting here but I just couldn't make sense of what I found on the net so far and this discussion did lighten things up a bit, but it also added some confusion 😳

I'm trying to use the stock CR-10 display with a GT2560 board to add dual extrusion to my CR-10, so I tried 1.1.6 with

#define REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER
#define U8GLIB_SH1106
#define CR10_STOCKDISPLAY

but I only get short blink and a chirp from the buzzer when switching on the board.
Do I need to rotate the 10pin header so that top left pin ends up bottom right?
Or do I need to flip the pins so that top left ends up top right?
Or both so that top left ends up bottom left?

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 17, 2017 via email

@ScruffR
Copy link

ScruffR commented Oct 18, 2017

Thanks!
I'll comment REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER and try rotating the connector (after shaving off the polarity protection) then.

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 18, 2017 via email

@AnHardt
Copy link
Member

AnHardt commented Oct 18, 2017

I do know.
It's this ingenious pdf where you cant say what's on the right and what's on the left.
http://reprap.org/mediawiki/images/7/79/LCD_connect_SCHDOC.pdf

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 18, 2017 via email

@jmdearras
Copy link
Contributor Author

jmdearras commented Oct 18, 2017 via email

@ScruffR
Copy link

ScruffR commented Oct 18, 2017

I've now tried with the connector rotated and now the display lights up and the buzzer chirps when keeping the encoder button pressed, but the display stays blank

Tried all combinations of

//#define REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER
#define U8GLIB_SH1106
#define CR10_STOCKDISPLAY

and

#define REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER
#define U8GLIB_SH1106
#define CR10_STOCKDISPLAY

and

BOARD_GT2560_REV_A 

and

BOARD_GT2560_REV_A_PLUS

(it's a board marked as A+)

The display is connected to the 10 pin header marked LCD (I guess this is EXT2 - not AUX)

BTW, gcode M300 does play a sound as intended.

These are my configs for 1.1.6

Configuration.zip

@ScruffR
Copy link

ScruffR commented Oct 19, 2017

I have now flashed some test sketch onty my GT2560 A+ board and realised that the pinout on the header must be completely off.

With a u8glib test sketch I figured that the CR10_STOCKDISPLAY actually uses only three pins (SPI mode).
On the GT2560 A+ the respective pins are

// CR10_STOCKDISPLAY on GT2560 | name on DISPLAY | pin# on GT2560 | function
#define E  21                 // E               | 21             | SCLK  (SPI Clock) 
#define RW  6                 // RW              |  6             | MOSI  (Master Out Slave In)
#define RS  5                 // RS              |  5             | CS/SS (Chip/Slave Select)

(this is with the 10pin header rotated)

@ScruffR
Copy link

ScruffR commented Oct 19, 2017

I haven't come round yet to investigate the SD pinout but this is how the display and rotary encoder need to be set up for CR10_STOCKDISPLAY on GT2560 A/A+

pins_GT2560_REV_A.h

#if ENABLED(ULTRA_LCD)

  #define BEEPER_PIN       18
  #if ENABLED(NEWPANEL)
    // ************* ScruffR ************* 
    #if ENABLED(CR10_STOCKDISPLAY) 
      #define LCD_PINS_RS      5
      #define LCD_PINS_ENABLE  6
      #define LCD_PINS_D4     21
      #define LCD_PINS_D5     -1
      #define LCD_PINS_D6     -1
      #define LCD_PINS_D7     -1

      // Buttons are directly attached
      #define BTN_EN1         17
      #define BTN_EN2         16
      #define BTN_ENC         19
    // ************* ScruffR ************* 
    #else
      #define LCD_PINS_RS     20
      #define LCD_PINS_ENABLE 17
      #define LCD_PINS_D4     16
      #define LCD_PINS_D5     21
      #define LCD_PINS_D6      5
      #define LCD_PINS_D7      6

      // Buttons are directly attached
      #define BTN_EN1         42
      #define BTN_EN2         40
      #define BTN_ENC         19
    #endif

    #define SD_DETECT_PIN     38

  #else // !NEWPANEL

    #define SHIFT_CLK      38
    #define SHIFT_LD       42
    #define SHIFT_OUT      40
    #define SHIFT_EN       17

    #define LCD_PINS_RS      5
    #define LCD_PINS_ENABLE  6
    #define LCD_PINS_D4     21
    #define LCD_PINS_D5      0
    #define LCD_PINS_D6      0
    #define LCD_PINS_D7      0

    #define SD_DETECT_PIN  -1

  #endif // !NEWPANEL

#endif // ULTRA_LCD

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.

7 participants