ADAFRUIT_ST7565 #2105

eboston · 2015-05-17T19:59:31Z

Added support for new display type.

Added support for new display type

AnHardt · 2015-05-17T20:26:34Z

@eboston
To make travis and me happy:

don't #define ADAFRUIT_ST7565 by default
use our coding standard: http://www.marlinfirmware.org/index.php/Coding_Standards (i suppose you used tabs)

else this looks good.

eboston · 2015-05-17T20:30:11Z

This is my first ever pull request/contribution to an open source so I'm still learning. I thought I had commented out the define but guess that didn't get saved. As for the tabs, I put in spaces but think the editor replaced them. Will fix it up.

Fixes for some code formatting changes and commenting out the ADAFRUIT_ST7565 define that was left defined.

AnHardt · 2015-05-17T20:43:54Z

@eboston
Are you using a a premade board with a name we can use, or just a display and an encoder?
In the later case separate the encoder from the display.
EDIT:
Ah, i see. A not finished-published board.
Could be a good idea to use:

in Configuration.h #define MYBOARD and add a hint on u8glib, If possible add a link to the board.
in Conditional.h #ifdef MYBOARD #define ADAFRUIT_ST7565
in pins_RAMPS_13.h #ifdef MYBOARD #define yourpins

eboston · 2015-05-17T20:47:36Z

It is a board that I am designing. It uses the Adafruit ST7565 display and a modified version of the RepRapDiscount Smart Graphics board.

eboston · 2015-05-17T21:16:31Z

I have no problems with making those changes, but don't follow what it will really accomplish. It looks basically like renaming the ADAFRUIT_ST7565 to MYBOARD. Are you thinking the ADAFRUIT_ST7565 name is too close to the display used? I was planning on calling the board the Adafruit ST7565 Graphic Display.

AnHardt · 2015-05-17T22:07:17Z

@eboston
Yes. Basically I think ADAFRUIT_ST7565 is the name of a display what can find its way to other boards. If you have a relation to Adafruit this may be a good name for the board - otherwise i'd think about a more personal name.

eboston · 2015-05-17T22:11:05Z

The display does come from Adafruit and that is the product name on the website (http://www.adafruit.com/products/250). I understand what you are saying so will change to a more generic name for the define.

Rename of define to avoid confusion between the controller and the display which had similar names.

AnHardt · 2015-05-17T23:30:22Z

@eboston
Sorry for editing my second comment, so you may have missed that.
dogm_lcd_implementation.hand ultralcd.cpp deal with the DISPLAY and its property 'contrast'. Here a dependency on ADAFRUIT_ST7565 is a good thing to make this reusable.
In pins_RAMPS_13.h you deal with property s of the BOARD, like sd-reader, encode, ..., what is of limited value for others.
Conditionals.h is the right place to make the DISPLAY a property of the BOARD. (near the DEFAULT_LCD_CONTRAST)

@thinkyhead
Would like to sugest the addition in some more Configuration.h's. But lost the overview what makes sense for a (until now) RAMPS only board .

eboston · 2015-05-18T00:05:34Z

I think I understand what you are saying. Let me repeat back what I think you want.

Configuration.h is fine with ELB_FULL_GRAPHIC_CONTROLLER
Conditionals.h is fine using ELB_FULL_GRAPHIC_CONTROLLER to define DEFAULT_LCD_CONTRAST. This matches the format of the other definitions of DEFAULT_LCD_CONTRAST.
pins_RAMPS_13.h should make a define for ADAFRUIT_ST7565
In dogm_lcd_implementation.h, instead of ELB_FULL_GRAPHIC_CONTROLLER to define the u8g class to use, use new ADAFRUIT_ST7565 defined in pins_RAMPS_13.h.
Do the same as 4 in ultralcd.cpp for setting the display format.

Does that sound about right?

AnHardt · 2015-05-18T00:31:29Z

No.
1.) ok. And add a hint on u8glib.
2.) Add #define ADAFRUIT_ST7565 beneath your #define DEFAULT_LCD_CONTRAST
3.) leave as it is. To be exact #define SDCARDDETECTINVERTED and #define SDSLOW are not really pins, so they are better placed in Conditionals.h (also beneath #define DEFAULT_LCD_CONTRAST)
4.) yes and no. The display driver is dependent on the display. How it is connected is dependent on the board.
5.) Yes. Here you are dealing with the contrast and its display-format, what is a property of the display (or its driver) - not the board.

Please wait with a new commit for a comment of @thinkyhead .

eboston · 2015-05-18T00:46:12Z

What do you mean by "And add a hint on u8glib"?

The other things are just really confusing me.

So looking at the REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER define in Conditionals.h. It has a define for U8GLIB_ST7920. So under the ELB_FULL_GRAPHIC_CONTROLLER, would it make more sense to make a define like U8GLIB_LM6059 so it is using the same convention?

The defines for SDSLOW and SDCARDDETECTINVERTED would make more sense under the ELB_FULL_GRAPHIC_CONTROLLER in Conditional.h?

#elif defined(ELB_FULL_GRAPHIC_CONTROLLER)
  #define DEFAULT_LCD_CONTRAST 110
#endif

becomes

#elif defined(ELB_FULL_GRAPHIC_CONTROLLER)
  #define DEFAULT_LCD_CONTRAST 110
  #define SDSLOW
  #define SDCARDDETECTINVERTED
  #define U8GLIB_ST7920
#endif

and those two defines are removed from pins_RAMPS_13.h?

This would essentially remove the define ADAFRUIT_ST7565 and add U8GLIB_LM6059.

Am I getting close?

AnHardt · 2015-05-18T01:36:08Z

What do you mean by "And add a hint on u8glib"?

// ==> REMEMBER TO INSTALL U8glib to your ARDUINO library folder: http://code.google.com/p/u8glib/wiki/u8glib

The other things are just really confusing me.

So looking at the REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER define in Conditionals.h. It has a define for U8GLIB_ST7920. So under the ELB_FULL_GRAPHIC_CONTROLLER, would it make more sense to make a define like U8GLIB_LM6059 so it is using the same convention?

Locking at the others - there is no real convention - but room for improvement. What ever you want, as long we can distinguish between board and display. U8GLIB_LM6059 is also fine for me.

The defines for SDSLOW and SDCARDDETECTINVERTED would make more sense under the ELB_FULL_GRAPHIC_CONTROLLER in Conditional.h?

#elif defined(ELB_FULL_GRAPHIC_CONTROLLER)
  #define DEFAULT_LCD_CONTRAST 110
#1539

becomes

#elif defined(ELB_FULL_GRAPHIC_CONTROLLER)
  #define DEFAULT_LCD_CONTRAST 110
  #define SDSLOW
  #define SDCARDDETECTINVERTED
  #define ~~U8GLIB_ST7920~~ U8GLIB_LM6059
#endif

and those two defines are removed from pins_RAMPS_13.h?

Yes.

This would essentially remove the define ADAFRUIT_ST7565 and add U8GLIB_LM6059.

Where you want to address the board use ELB_FULL_GRAPHIC_CONTROLLER. Where you want to address the display use one of the above - but decide for one.

Am I getting close?

Yes.

eboston · 2015-05-18T01:59:17Z

#define ~~U8GLIB_ST7920~~ U8GLIB_LM6059

Oops. Oh the joys of cut and paste. :)

But in regards to the define U8GLIB_LM6059, I have to use a different name since it would conflict with the class definition. Would U8GLIB_LM6059_AF as a define be clear enough or do you have a better suggestion?

thinkyhead · 2015-05-18T02:31:57Z

Would like to suggest the addition in some more Configuration.h's…

Yes, we can discuss that. Such changes will bump the version up to 1.1, you know. 😁 If you look at the configurator, it breaks up options into sections, which could roughly correspond to configurations. So it's possible to end up with a bunch in the end. We should use shorter names and follow a new convention, so it can perhaps remain compatible with the old style.

Conf_controller.h
Conf_machine.h
Conf_motion.h
Conf_temperature.h
Conf_geometry.h
…

thinkyhead · 2015-05-18T02:32:30Z

Please wait with a new commit for a comment of @thinkyhead

I'll look it over shortly and get back to you…

thinkyhead · 2015-05-18T02:34:39Z

Marlin/ultralcd.cpp

+#ifdef ELB_FULL_GRAPHIC_CONTROLLER
+      lcd_contrast += encoderPosition;
+      lcd_contrast &= 0xFF;
+#else


Minor syntactical thing. Compiler directives should be part of the cascade too. This is so that naïve text editors can do code folding properly.

So what is it you are suggesting that should be changed here?

thinkyhead · 2015-05-18T02:35:34Z

It looks simple and stable to me. I'm actually surprised it doesn't require even more code.

eboston · 2015-05-18T02:44:45Z

@thinkyhead When I was starting to add support, I realized it was very similar to the VIKI2 display so I just followed the settings for it. About the only difference was the pins used and the u8g class.

AnHardt · 2015-05-18T02:49:45Z

@thinkyhead
No. I don't not want to have more configuration files. I want to integrate this particular feature into more of the existing configurations. But since here are only pins defined for RAMPS it makes no sense to put it into all existing configurations.
I'll go to bed for now, since it seems i'm talking french tonight.

thinkyhead · 2015-05-18T02:50:42Z

No. I don't not want to have more configuration files.

@AnHardt Oh good, that's a can of worms.

Moved SDCARDDETECTINVERTED and SDSLOW to Conditionals.h. Added U8GLIB_LM6059_AF to define display specific actions. Added reminder to compile in u8glib

AnHardt · 2015-05-18T19:41:59Z

@eboston
You are close. Very close!
Travis fails because you have again uncommented your board in Configuration.h
Additionally
https://github.com/MarlinFirmware/Marlin/pull/2105/files#diff-34f7cdd618621ae7d8c113cf0b86a2f4R1113
and
https://github.com/MarlinFirmware/Marlin/pull/2105/files#diff-34f7cdd618621ae7d8c113cf0b86a2f4R1125
sh(o)uld not be: #ifdef U8GLIB_ST7920
but your display: #ifdef U8GLIB_LM6059_AF

Good job!

Fixed a couple defines that were not changed or commented out.

thinkyhead · 2015-05-19T04:47:04Z

Marlin/Configuration.h

@@ -660,6 +660,12 @@ const bool Z_PROBE_ENDSTOP_INVERTING = false; // set to true to invert the logic
 //#define VIKI2
 //#define miniVIKI

+// This is a new controller currently under development.  A link to more information will be provided as it
+// becomes available.


When will it be available?

How about right now? I just updated Configuration.h with the link.

Added the link to the github information about the ELB_FULL_GRAPHIC_CONTROLLER

AnHardt · 2015-05-19T12:51:57Z

I'm happy with the link.
Now make your changes to Configurations.h to the other 11 Configurations.h. You'll find them in the tree below configurator and example_configurations.

Updated all the example Configuration.h files for the new display type.

thinkyhead · 2015-05-20T01:29:36Z

Marlin/ultralcd.cpp

-      lcd_contrast -= encoderPosition;
-      lcd_contrast &= 0x3F;
+      #ifdef U8GLIB_LM6059_AF
+        lcd_contrast += encoderPosition;


This PR is looking very complete. I have one question about the encoder direction. Does the contrast increase when the knob is turned clockwise? And, do the menus go in the down direction when you turn the knob clockwise? The reason I ask is that we want to add an option to reverse the direction that the knob moves through menus (for a knob to the right of the display). That change will only reverse menu movement. So I want to make sure that when we do that, reversing the knob direction, it works the same way, with clockwise moving in the up direction in the menus.

When I turn the knob to the right (clockwise), the menu selection moves down and the contrast value moves up. When the contrast value goes up, the screen gets darker.

Great, perfect!

I may kick myself for asking this, but since you already merged in the change, too late! :) I got a RepRapDiscount Full Graphic Controller and noticed the default behaviour of the knob is opposite of what I have. Turn it counter-clockwise and the menu selection moves down. Is the standard going to be the way I have it on mine? Personally, turning it counter-clockwise seems backwards to me no matter where the knob is located.

Well, it's one of the requests out there, at least to have an option to reverse just the menu direction. So, I just need to see if all knobs move the menu highlight in the same way, so that option will tend to behave the same on all printers.

Roxy, I did not mean at all to sound mean or angry at any of you! I meant I
am so glad the environment is no longer toxic here! I appreciate all your
hard work and look forward to continue collaborating with the team!

On Wed, 1 Jun 2016 at 20:14, Roxy-3DPrintBoard [email protected]
wrote:

In Marlin/ultralcd.cpp
#2105 (comment):

@@ -1110,13 +1110,24 @@ static void lcd_control_volumetric_menu() {
#ifdef HAS_LCD_CONTRAST
static void lcd_set_contrast() {
if (encoderPosition != 0) {

lcd_contrast -= encoderPosition;

lcd_contrast &= 0x3F;

#ifdef U8GLIB_LM6059_AF

lcd_contrast += encoderPosition;

@emartinez167 https://github.com/emartinez167 I fully understand your
statements. We would like you to give us a second chance. If you even think
you were treated less politely than you should have been, please send an @
message to ThinkyHead or me. My guess is we won't be getting any messages
from you going forward. We are very serious about keeping this a healthy
environment for both users and developers.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/MarlinFirmware/Marlin/pull/2105/files/082ed3beedfaffaa7fb8b8e2ae4fa2f0f9f8aca4#r65348309,
or mute the thread
https://github.com/notifications/unsubscribe/AGcPb2IDnwCS4FHmvu3x1s4yCihqoqkvks5qHXejgaJpZM4Ed7jG
.

@thinkyhead Reading through this, it seems most of the issues being discussed are addressed in that patch I submitted. In regards to action of turning the encoder in relationship to contrast, my thoughts are regardless of how it works with the menus, turning clockwise should increase contrast, counter clockwise decrease. That is how most controls like this work. Clockwise to turn up the volume. Increase the temperature on a stove. (at least in the US).

…issues being discussed are addressed in that patch…

@eboston Yes, your patch is a great improvement.

regardless of how it works with the menus
turning clockwise should increase contrast, counter clockwise decrease

The LCD contrast will change according to the same rotation direction as editing values. So clockwise increases contrast, counterclockwise decreases it. Users can reverse the direction of that using REVERSE_ENCODER_DIRECTION, if they have some reason to do so.

(Menu navigation direction is reversed with REVERSE_MENU_DIRECTION.)

@eboston #3944 is based on the original PR. Does it look ok to you?

Based on a quick look, it looks good.

Add ELB_FULL_GRAPHIC_CONTROLLER with ADAFRUIT_ST7565 display.

AnHardt · 2015-05-21T11:37:08Z

@eboston
It's never to late for an other change. Just make an other PR.

When in edit mode, turning clockwise is ought to increase the numbers (test with a temperature). If not swap BTN_EN1 and BTN_EN2.
Even in LCD contrast the numbers are ought to increase when turning clockwise. Whether you adjust the brightness or the darkness is up to you. (max_value - display_value to reverse)

The direction of movement in the menu may be configurable later.

eboston · 2015-05-21T12:49:57Z

Since the RRD Full Graphic controller doesn't support menu based contrast, I don't have anything to compare against. But the code would make sense, using -= for contrast value, if turning the knob worked like it does on the RRD, turning counter-clockwise moves the menu selection down.

So with the code the way it is, if someone changed the encoder pins to switch the menu movement, it would also switch how the contrast changes. Is this the reason for the request to add the option to switch the menu direction? If so, I might just look at that if no one else is.

thinkyhead · 2015-05-21T22:04:38Z

Is this the reason for the request to add the option to switch the menu direction?

Some users would prefer counterclockwise ⟲ movement of a knob on the right side of the display to move the menu ⬇︎ instead of ⬆︎. But edited values, contrast, etc., should always increase clockwise ⟳ and decrease counterclockwise ⟲.

ADAFRUIT_ST7565

41f8cdb

Added support for new display type

AnHardt added PR Needs update C: Boards/Pins labels May 17, 2015

Formatting Changes

1999ac4

Fixes for some code formatting changes and commenting out the ADAFRUIT_ST7565 define that was left defined.

Changed ADAFRUIT_ST7565 to ELB_FULL_GRAPHIC_CONTROLLER

047e688

Rename of define to avoid confusion between the controller and the display which had similar names.

thinkyhead reviewed May 18, 2015
View reviewed changes

More functional seperation

a83bf18

Moved SDCARDDETECTINVERTED and SDSLOW to Conditionals.h. Added U8GLIB_LM6059_AF to define display specific actions. Added reminder to compile in u8glib

Define fixes

ca8d175

Fixed a couple defines that were not changed or commented out.

thinkyhead reviewed May 19, 2015
View reviewed changes

Added link to controller github

152b681

Added the link to the github information about the ELB_FULL_GRAPHIC_CONTROLLER

Propagate Configuration.h changes

082ed3b

Updated all the example Configuration.h files for the new display type.

thinkyhead reviewed May 20, 2015
View reviewed changes

AnHardt added a commit that referenced this pull request May 20, 2015

Merge pull request #2105 from eboston/Development

0651356

Add ELB_FULL_GRAPHIC_CONTROLLER with ADAFRUIT_ST7565 display.

AnHardt merged commit 0651356 into MarlinFirmware:Development May 20, 2015

thinkyhead removed the PR Needs update label May 21, 2015

ADAFRUIT_ST7565 #2105

ADAFRUIT_ST7565 #2105

Conversation

eboston commented May 17, 2015

AnHardt commented May 17, 2015

eboston commented May 17, 2015

AnHardt commented May 17, 2015

eboston commented May 17, 2015

eboston commented May 17, 2015

AnHardt commented May 17, 2015

eboston commented May 17, 2015

AnHardt commented May 17, 2015

eboston commented May 18, 2015

AnHardt commented May 18, 2015

eboston commented May 18, 2015

AnHardt commented May 18, 2015

eboston commented May 18, 2015

thinkyhead commented May 18, 2015

thinkyhead commented May 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkyhead commented May 18, 2015

eboston commented May 18, 2015

AnHardt commented May 18, 2015

thinkyhead commented May 18, 2015

AnHardt commented May 18, 2015 • edited by Roxy-3D Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnHardt commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnHardt commented May 21, 2015

eboston commented May 21, 2015

thinkyhead commented May 21, 2015

AnHardt commented May 18, 2015 •

edited by Roxy-3D

Loading