[BUG] LCD_PINS_D7 never defined for RADDS

[vicinzu]

Did you test the latest bugfix-2.0.x code?

Yes, and the problem still exists.

Bug Description

With PR #19890 a lot of files have been modified and the changes have not been accurately tested.

When combining BOARD_RADDS with REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER the build fails with following error:

Marlin/src/lcd/../inc/../pins/sam/pins_RADDS.h:289:38: error: 'LCD_PINS_D7' was not declared in this scope

The cause is following addition to pins_RADDS.h:

#if ENABLED(REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER)
  #define BTN_ENC_EN               LCD_PINS_D7  // Detect the presence of the encoder
#endif

But LCD_PINS_D7 is never defined (e.g. in Conditionals_LCD.h).

Bug Timeline

PR19890

Expected behavior

Build succeeds

Actual behavior

Build fails

Steps to Reproduce

1. Set #define MOTHERBOARD to BOARD_RADDS
2. Uncomment #define REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER
3. Build for the Arduino DUE

Version of Marlin Firmware

2.0.9.3

Printer model

Custom Prusa i3

Electronics

Arduino Due + RADDS

Add-ons

RepRapDiscount Full Graphic Smart Controller

Bed Leveling

No response

Your Slicer

No response

Host Software

No response

Additional information & file uploads

No response

[thinkyhead]

In most pins files there seems to be a definition of that pin for the relevant case. I cannot vouch for the accuracy of this encoder-enabling pin in all the pins files where it is included, but that is what we hope to learn. So, does this work…?

 #define BTN_ENC_EN                        47

… or do you think this pin should only apply to the case of RADDS_DISPLAY ?

CC: @sjasonsmith

[sjasonsmith]

I think it should just be deleted from that file. That pin is only used if you have a smart display which can be switched back and forth between RRDFG and TFT modes, and you’ve enabled the feature to test for an active encoder.

This board uses a custom-built adapter to attach a RRDFG display, so we don’t know that pin (if even exposed) without digging into that.

It seems best to leave that exercise to somebody actually using that board with a switchable display to sort out.

[thinkyhead]

It fell into a lot of files in PR #19890 — Do you think most of them are improper? — There is no checking in the code itself for a switchable display, instead the can_encode method is always present for MarlinUI. So… since #19890 the pin is actually required for all MarlinUI displays (regardless of HAS_ENCODER_ACTION).

[sjasonsmith]

I’ll take a look at the code in a little bit. It sounds like we might be unsafely using that pin for displays that don’t necessarily implement it. It might be fine most of the time of the board/MCU pulls the pin in the right direction, but that’s not a very safe guarantee.

[thinkyhead]

Ah, I see it does this… so it should be covered when defined to a non-existent symbol….

#if BUTTON_EXISTS(ENC_EN)
  #define _BUTTON_PRESSED_ENC_EN _BUTTON_PRESSED(ENC_EN)
#else
  #define _BUTTON_PRESSED_ENC_EN false
#endif

. . .

inline bool can_encode() { return !BUTTON_PRESSED(ENC_EN); }

[sjasonsmith]

OK, looking back at the original PR to add the feature (#19796) I see that this was committed without any configuration options. It has been this way for over a year...which makes me think it is ok and boards must all be pulling that input appropriately?

Based on this I think the only action needed for this bug is to delete the pin definition from RADDS, since there isn't actual proper connectors to be sure which pin it would be on.

[thinkyhead]

It could exist for RADDS_DISPLAY only, which does define the _D7 pin.

[thinkyhead]

…Similar situation with EINSY_RETRO…

[sjasonsmith]

The whole purpose of the feature was so that BTT's TFT displays wouldn't create encoder noise while in TFT (non-RRDSG) mode. That is why it was added inside "if RRDSG" blocks. Defining it for other displays not known to have similar issues won't provide any value.

[sjasonsmith]

With that display users typically configure their firmware for REPRAPDiscount Full Graphics, then can switch on the fly between RepRapDiscount mode or serial-connected TFT mode. When in TFT mode the encoder lines were undriven, which was causing weird behavior on the printer.

[sjasonsmith]

Or maybe it is that the encoder was still connected to the encoder pins, so that even though the display was in TFT mode, the user would be accidentally controlling their printer over encoder pins even though they were using the knob to navigate their TFT-mode menus.

[descipher]

Seems like the real solution should have been to define an "REPRAP EMULATOR DISPLAY" Option

[descipher]

I'm thinking this is would resolve multiple issues.

Configuration.h
//
// RepRapDiscount FULL GRAPHIC Smart Controller
// https://reprap.org/wiki/RepRapDiscount_Full_Graphic_Smart_Controller
//
//#define REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER
//#define FULL_SMART_CONTROLLER_EMULATION   //Use for emulation switching screens


Conditionals_LCD.h
 
#elif ANY(REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER, BQ_LCD_SMART_CONTROLLER, FULL_SMART_CONTROLLER_EMULATION)

pins_xxx.h

#if ENABLED(FULL_SMART_CONTROLLER_EMULATION)
  #define BTN_ENC_EN       LCD_PINS_D7  // Detect the presence of the encoder for emulation switching displays 
#endif

[thisiskeithb]

BTT’s standalone/dual-mode TFTs can also emulate a text / 20x4 LCD, so if you are going to add an emulated 12864 option, an emulated 2004 option should be added as well.

[sjasonsmith]

I do agree that making this feature opt-in is preferable to the current assumption that it is harmless for all RRDFGSC users. It is apparently not causing problems for most users, but it is quite possible that some boards/people don't pull that line the way assumed, and checking that pin might cause a dead encoder on non-emulated displays which may leave it unconnected.

To @thisiskeithb's point above, the original PR (#19796) didn't expose it as new display types, but as an option for the encoder configuration. That option was removed prior to merge in favor of looking for pin definitions instead.

[descipher]

I draw I line in the sand .. lol

[sjasonsmith]

Although I will also stand by my previous assertion that it's been this way for a year, so it might not be worth the effort. If this is the only complaint then delete it from the RADDS file and be done with it. If we've been seeing lots of reports of flaky encoders then it probably needs more attention.

[thinkyhead]

FULL_SMART_CONTROLLER_EMULATION

Can the displays that support the emulated DOGLCD actually be made to not present the option to switch to the emulated DOGLCD? Would that be done by locking the BTN_ENC_EN to OUTPUT + HIGH or something like that? If the displays are always going to show that UI, then there is no point in adding an option to make it optional. Meanwhile, only the TFT displays which support the emulated DOGLCD should have BTN_ENC_EN defined. So, that pin could be #undefined in pins_postprocess.h or wrapped in an additional condition in the pins files.

So, instead of a user-configurable option, a conditional…

#if ANY(BQ_LCD_SMART_CONTROLLER, FULL_SMART_CONTROLLER_EMULATION)
  #define TFT_EMULATED_DOGLCD 1
#endif

[thisiskeithb]

Can the displays that support the emulated DOGLCD actually be made to not present the option to switch to the emulated DOGLCD?

Not without cutting up TFT code. Holding the encoder down for 1.5 seconds will always present the mode switch screen (even when marlin_type is set to 1 / 2004 LCD).

[thinkyhead]

Right, I assumed they were hard-coded that way, so I'm really suggesting the things that follow.

[vicinzu]

Sorry for my late reply and thanks for the interest in this issue. It took a year to notice it, because the combination of RADDS and the FGSC is very rare. Most people don't use it, because you need an adapter, In my case i wanted to reuse the screen that i used with RAMPS before.

@thinkyhead

So, does this work…?
#define BTN_ENC_EN 47
… or do you think this pin should only apply to the case of RADDS_DISPLAY ?

It works. Build successful and screen running.

[thinkyhead]

It works. Build successful and screen running.

Cool! But now there is some extra logic in the code that isn't needed, checking that pin before processing the encoder. It should only be doing that for displays that have built-in encoder magic.

[github-actions]

This issue has had no activity in the last 60 days. Please add a reply if you want to keep this issue active, otherwise it will be automatically closed within 10 days.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.