-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
[BUG] LCD_PINS_D7 never defined for RADDS #23437
Comments
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 CC: @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. |
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 |
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. |
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); } |
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. |
It could exist for |
…Similar situation with |
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. |
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. |
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. |
Seems like the real solution should have been to define an "REPRAP EMULATOR DISPLAY" Option |
I'm thinking this is would resolve multiple issues.
|
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. |
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. |
I draw I line in the sand .. lol |
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. |
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 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 |
Not without cutting up TFT code. Holding the encoder down for 1.5 seconds will always present the mode switch screen (even when |
Right, I assumed they were hard-coded that way, so I'm really suggesting the things that follow. |
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.
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. |
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. |
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. |
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:
The cause is following addition to
pins_RADDS.h
:But
LCD_PINS_D7
is never defined (e.g. inConditionals_LCD.h
).Bug Timeline
PR19890
Expected behavior
Build succeeds
Actual behavior
Build fails
Steps to Reproduce
#define MOTHERBOARD
toBOARD_RADDS
#define REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER
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
The text was updated successfully, but these errors were encountered: