Disable option for SDSUPPORT

[thinkyhead]

In response to #3484…

The old behavior always enabled SDSUPPORT for those controllers that have it, and you couldn't disable it. Then we made SDSUPPORT an explicit option. This PR demonstrates the alternative – to keep SDSUPPORT on unless specifically requested to be off. The SDSUPPORT option can also be added to force it to be on, even if the active controller doesn't support it.

[jbrazio]

@thinkyhead please check the followup on #3484 (comments).

[Grogyan]

This makes things more confusing than it needs to be, in my opinion.

[thinkyhead]

I do like the idea of making SDSUPPORT more prominent in the LCD Controller configuration section, since it is a core option for most controllers.

However, I can also understand the user frustration of needing to enable SDSUPPORT explicitly now, where older Marlin simply enabled it for you. At that time you couldn't easily turn it off, so that was a minor annoyance for a few.

When we made SDSUPPORT an explicit option we did confuse some users who expected their displays to work "out of the box" (according to some website's instructions from 2011).

The other thing we obscured was the association between SDSUPPORT and ULTIPANEL – which always enabled SDSUPPORT along with it. Users with ULTIPANEL certainly would prefer SDSUPPORT to be enabled by default. (But not in such a way that it can't be disabled.) Maybe ULTIPANEL isn't the best clue about SDSUPPORT – after all we can see whether the user's selected display has an SDDETECT pin.

So there's this lingering question of how to make sure that components like LCD controllers with SD slots work as users expect, or to find some way to warn them in advance of flashing the firmware.

Perhaps with ULTIPANEL we would instead issue a warning:

#if DISABLED(SDSUPPORT)
  #error Your controller has an SD Card slot! Enable SDSUPPORT to use it, or comment out this line to continue without it.
#endif

[jbrazio]

Perhaps with ULTIPANEL we would instead issue a warning

@thinkyhead Can't we just use Conditionals.h to enable SDSUPPORT if ULTIPANEL is selected ?

[thinkyhead]

Can't we just use Conditionals.h to enable SDSUPPORT if ULTIPANEL is selected?

@jbrazio That would override any attempt by the user to disable SDSUPPORT elsewhere, and that is exaclty why the code to enable SDSUPPORT with ULTIPANEL was removed in the first place.

[CONSULitAS]

@thinkyhead @jbrazio

@jbrazio That would override any attempt by the user to disable SDSUPPORT elsewhere, and that is exaclty why the code to enable SDSUPPORT with ULTIPANEL was removed in the first place.

Perhaps just having SDSUPPORT is not an appropriate model for the reality of the hardware. I think we have the following cases:

1. printer without SD
2. printer with onboard SD module
3. printer with separate SD module
4. printer with LCD with SD module (like the Ultipanel)

So lets think in two steps:

• Hardware that is there should be enabled by default. Everything else would surprise the common plug and play user.
• For some reasons users want to disable hardware. Perhaps for testing or switching off a defective unit or ...

So we need two properties to define whats going on:

• HAS_SDREADER for defining the found hardware
• DISABLE_SDREADER for overriding the default behavior

So we will get a logic like:

// Ultipanel has a build in SD card reader
#if ENABLED(ULTIPANEL) && !ENABLED(HAS_SDREADER)
  #define HAS_SDREADER
#endif

... // more LCDs

// Example board has a build in SD card reader
#if (ENABLED(BOARD_WITH_SDREADER)) && (!ENABLED(HAS_SDREADER))
  #define HAS_SDREADER
#endif

... // more boards 

// set SD support according to found hardware and user choice
#if (ENABLED(HAS_SDREADER)) && (!ENABLED(DISABLE_SDREADER))
  #define SDSUPPORT
#endif

Consequences:

• solution should be backwards compatible to older configs (which complain the incompatibility, like Not sure what is being accomplished with #define SDSUPPORT #3484)
• SDSUPPORT would vanish from Conditionals.h
• HAS_SDREADER and DISABLE_SDREADER as options in Configuration.h
• build in hardware is configured automagically
• you can switch it off - override
• you can configure "SD only" readers like http://www.ebay.de/itm/TF-SD-Card-Ramps-DIP-RAMPS-1-4-Module-for-3D-Printer-Reprap-/252245909026 with `HAS_SDREADER

[jbrazio]

@CONSULitAS your concept is perfect I will not disagree with it not even I am opposing for its implementation, but I will question if this really needs so much effort ? The flag will have [soon #3496] more visibility. We let the user to decide what he wants to do.. and as with most Marlin features we keep them disabled unless the user enables them.

[CONSULitAS]

@jbrazio
Thanks. 💐

• I really love, if something is perfectly designed and polished.
• For software i have the experience, that every software is used by more people than you think and is used longer than you even imagine. 😄
• So make software a bit better all the time and you get paid back really soon.

With the experience of the issues in the past, i am asking me, if the effort is not less in total (1000 users thinking only 3 minutes over it...) to use the nearly ready solution.

See perhaps http://mashable.com/2011/12/18/steve-jobs-20-life-lessons/#04.59UyNhiqV (the part with "Be Persuasive").

But just my 2 💵 or ¢.

[jbrazio]

I leave the burden of taking the decision up to @thinkyhead. ;-P

[thinkyhead]

@CONSULitAS You're essentially voting to reopen this PR. 😁

I agree that SDSUPPORT should be enabled when the hardware exists. But we can't check this automatically based on the MOTHERBOARD setting alone. The pins_MYBOARD.h files only name the pins that are recommended for various uses. They don't state outright that a piece of hardware like an SD reader exists.

So, historically, if you had ULTIPANEL then you had SDSUPPORT and that was the only condition required. To disable it you just had to uncomment the right #define SDSUPPORT line, but it wasn't exposed as "here's something you can disable."

Given these limitations, this PR reverts to exactly the behavior you prescribe.