Marlin
Marlin copied to clipboard
♻️ Separate LCD pins from board pins
Gradual migration of LCDs to separate pins file(s) using defined connectors / interfaces.
I'm not using anymore AGCM4 + RAMPS_144 but it seems that on AUX1 you have:
VCC GND D1 D0
VCC GND A3 A4
but VCC may be 5V/3V depending on J4 jumper (this goes also to IO pin)
AUX3 is (please note that SPI pins on AGCM4 are not like DUE but standard D50-52 may be redirected via solering pads):
RESET GND SCK MISO VCC
NC 5V D53 MOSI D49
AUX4 is: VCC and not 5V
RAMPS_1.4.4_C6.pdf LCD144-A_Rev.A.pdf
As reference I add schematic
N.B. Ramps144 has been discontinued, because of developper health issues, then AGCM4 has de facto no shields available
If this isn't breaking anything I may go ahead and merge this incomplete LCD refactor to get the ball rolling. This set of changes will be very important to the continued relevance and viability of Marlin Firmware going forward.
please no!
128x64 lcds and 20x4 displays are already broken, with spacing issues
https://github.com/MarlinFirmware/Marlin/issues/26424
marlin controlled touch screen are all now broken after latest touch refactors
https://github.com/MarlinFirmware/Marlin/pull/26445
SPI TFT displays have been broken for a long time on many platforms.
https://github.com/MarlinFirmware/Marlin/pull/26052 https://github.com/MarlinFirmware/Marlin/pull/26176
Please don't break any more displays any further than they are already.
128x64 lcds and 20x4 displays are already broken, with spacing issues marlin controlled touch screen are all now broken after latest touch refactors SPI TFT displays have been broken for a long time on many platforms
That is all related to code logic, not to pins. If this PR is not changing the pins that are assigned for LCDs (or otherwise making those issues worse) then these other issues have no bearing on this PR. We can speculate about whether this PR would affect those issues, but without any data that is just speculation.
Thanks for bringing those issues up, as it is a major priority with this next release to fix up all remaining and known bugs. So let's keep plugging away at fixing these things and see if we can locate more nerds who like to dive in and solve these issues so they don't linger for such a long time.
Scott I guess the rationale for this change is to remove duplication with the same displays having to be defined in the pins file of every mainboard they are compatible with. Is that correct?
Looking at the code diffs, the logic of the change seems simple enough but the scale is immense. With that much copying I think the likelihood of an error is high. On the other hand, I prefer smaller merges more often to larger merges more seldom so if the only choice is between merging now or doing even more changes and merging later then now is better. Even better would be if this could be broken down into smaller components.
I am mindful of @ellensp's concerns as well. Better to get displays working generally before refactoring.
How much testing has this PR had? Does it even compile for every potential combination?
When complete this is going to be a good change. Maintaining every screen in every pins file is never-ending. It's also difficult to understand if they are correct when pins are added.
I don't think this is ready to go in. I see lots of details moved to pins/lcd files, but I don't see anything actually including those files yet. I also don't think there would be any practical way for me to review the changes and actually confirm they are correct.
The following is what I think could be a safe way to approach this with minimal disruption for anyone:
-
Stage and commit all of the non-functional changes. This touches a lot of comments which add noise and make it harder to spot which changes in the PR deserve attention. The smaller the scope of each PR, the easier it is for someone to volunteer to review and test those changes, and actually understand what they're looking at.
-
Write a script which will set a board (one per pins file) and use the preprocessor to dump all the symbols it generates. This should be repeated for each screen which is being extracted into a new file. We have done something similar to this in the past when we were playing with adding embedded configurations, so I don't think this should be too hard.
-
Add the new LCD pins files, but ONLY include them if the board pins file has "opted-in" by declaring something like USE_COMMON_LCD_PINS. This flag could be removed once the transition is complete.
-
As pins files opt-in, re-run the script from step 2 and compare the output before/after output to check for unexpected differences. Fixing them at this stage will be a lot easier than figuring out what's wrong after people are using the change.
I'm sure there will still be some mistakes made...especially in the "weird' boards with non-standard connectors, but this should enable the majority to be transitioned without requiring any specific hardware testing.
I'm not really going to be available today, but I'm willing to contribute to creating that script I mentioned in step 2.
I support everything @sjasonsmith says.
I don't see anything actually including those files yet.
You probably already figured this out, but the appropriate LCD defines are included automagically by pins_lcd.h
which is included at the end of pins.h
…
//
// LCD / Controller Pins based on board expansion headers with adapters
//
#include "pins_lcd.h"
Hello
in #26719 you swapped the BNT buttons in the ReARM board for the CR10_STOCK_DISPLAY
can ask why?
greetings
in #26719 you swapped the BNT buttons in the ReARM board for the
CR10_STOCK_DISPLAY
can ask why?
To eliminate this pointless requirement only mentioned in the pins file:
// Requires REVERSE_ENCODER_DIRECTION in Configuration.h
oh. i forgot that
thanks
The following is what I think could be a safe way to approach this with minimal disruption for anyone:
Stage and commit all of the non-functional changes. This touches a lot of comments which add noise and make it harder to spot which changes in the PR deserve attention. The smaller the scope of each PR, the easier it is for someone to volunteer to review and test those changes, and actually understand what they're looking at.
Write a script which will set a board (one per pins file) and use the preprocessor to dump all the symbols it generates. This should be repeated for each screen which is being extracted into a new file. We have done something similar to this in the past when we were playing with adding embedded configurations, so I don't think this should be too hard.
Add the new LCD pins files, but ONLY include them if the board pins file has "opted-in" by declaring something like USE_COMMON_LCD_PINS. This flag could be removed once the transition is complete.
As pins files opt-in, re-run the script from step 2 and compare the output before/after output to check for unexpected differences. Fixing them at this stage will be a lot easier than figuring out what's wrong after people are using the change.
I'm sure there will still be some mistakes made...especially in the "weird' boards with non-standard connectors, but this should enable the majority to be transitioned without requiring any specific hardware testing.
I agree with all of this. We should also hold off on such a huge refactor until after 2.1.3
.