Marlin: [BUG] NO_SD_HOST_DRIVE on non _USB environments

[qwewer0]

Bug Description

https://github.com/MarlinFirmware/Marlin/pull/20151 PR needs NO_SD_HOST_DRIVE on a non _USB environments, that already cannot provide it.
(e.g. STM32F103RC_btt_512K, STM32F103RC_btt)

Configuration Files

Latest Bugfix-2.0.x
Configuration.zip

Steps to Reproduce

1. Compile firmware on STM32F103RC_btt_512K/STM32F103RC_btt with https://github.com/MarlinFirmware/Marlin/pull/20151 PR
2. Flash the new firmware on the board
3. Remove/insert SD card
4. See that it doesn't responds as it should

Expected behavior:

https://github.com/MarlinFirmware/Marlin/pull/20151 should work just as it does with NO_SD_HOST_DRIVE enabled.

Actual behavior:

Without NO_SD_HOST_DRIVE https://github.com/MarlinFirmware/Marlin/pull/20151 doesn't work on STM32F103RC_btt_512K/STM32F103RC_btt environments.

Additional Information

https://github.com/MarlinFirmware/Marlin/pull/20151#issuecomment-727735820

[sjasonsmith]

I asked @qwewer0 to file this. This is a usability issue that is confusing, even for people like me. You shouldn't have to disable SD_HOST_DRIVE if it would not have been enabled in the first place!

[sjasonsmith]

I'm labeling this as STM32, but it might impact other platforms as well. Basically, when HOST_DRIVE support is active, the SD_DETECT pins are disabled, so you don't get any automatic behavior on card insertion.

[sjasonsmith]

You don't have to use the PR referenced for testing either. Just bring up a serial console and watch for messages when inserting media.

[thinkyhead]

As I understand it, the NO_SD_HOST_DRIVE flag is a companion to SDCARD_CONNECTION ONBOARD. The effect of NO_SD_HOST_DRIVE is to cancel the default (USB-mountable) behavior of SDCARD_CONNECTION ONBOARD, apparently provided via MSC, DISKIO, …. This makes the SD_DETECT_PIN behave as it commonly does (and then also there is no need for a "release media" menu item).

[thinkyhead]

Should we be looking for the presence of USE_USB_COMPOSITE in relation to whether drives are USB-accessible? Any other flags we can discern as meaningful in the pile?

[sjasonsmith]

I'm not sure, I have previously found this inconvenient, but never spent the time to fully understand the conditions involved. After describing the behavior to someone yet again I asked qwewer0 to file the issue as a reminder to come back and improve upon it.

Perhaps we need each HAL to declare something like HAL_SUPPORTS_SD_HOST_DRIVE. That might be a simple 0/1 flag, or might be dependent on other compiler flags. Each HAL will be a little different.

[rhapsodyv]

Should we be looking for the presence of USE_USB_COMPOSITE in relation to whether drives are USB-accessible? Any other flags we can discern as meaningful in the pile?

Yes, but it will only fix STM32F1.

As I understand it, the NO_SD_HOST_DRIVE flag is a companion to SDCARD_CONNECTION ONBOARD. The effect of NO_SD_HOST_DRIVE is to cancel the default (USB-mountable) behavior of SDCARD_CONNECTION ONBOARD, apparently provided via MSC, DISKIO, …. This makes the SD_DETECT_PIN behave as it commonly does (and then also there is no need for a "release media" menu item).

Yes.

NO_SD_HOST_DRIVE just disable usb host with mass storage _on LPC_. Just that HAL. Even if some STM32F1 boards support that usb host with mass storage, they ignore that config....
It add another macro too, HAS_SHARED_MEDIA.
But it only make sense if you really have usb host with mass storage. That is the bug. If we add && USE_USB_COMPOSITE should fix it.
#if USE_USB_COMPOSITE && SD_CONNECTION_IS(ONBOARD) && DISABLED(NO_SD_HOST_DRIVE) && !defined(ARDUINO_GRAND_CENTRAL_M4)

But USE_USB_COMPOSITE is a maple macro. On STM32 we use USBCON and USBD_USE_CDC, and Marlin/src/inc/Conditionals_post.h runs for both.

Maybe we should just put a compatibility layer between those two hal, like:

#if USBD_USE_CDC
  #define USE_USB_COMPOSITE
#endif

Or make what @sjasonsmith said: add a neutral marlin macro name, that we use across marlin code and the hal define it based in its own macros.

[rhapsodyv]

Anyone can test it here? #20176

Thanks

[rhapsodyv]

The fix was merged. Closing this.