Skip to content
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] NO_SD_HOST_DRIVE on non _USB environments #20157

Closed
qwewer0 opened this issue Nov 16, 2020 · 10 comments · Fixed by #20176
Closed

[BUG] NO_SD_HOST_DRIVE on non _USB environments #20157

qwewer0 opened this issue Nov 16, 2020 · 10 comments · Fixed by #20176

Comments

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 16, 2020

Bug Description

#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 Go to File Browser on Media Insert (option) #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:

#20151 should work just as it does with NO_SD_HOST_DRIVE enabled.

Actual behavior:

Without NO_SD_HOST_DRIVE #20151 doesn't work on STM32F103RC_btt_512K/STM32F103RC_btt environments.

Additional Information

#20151 (comment)

@sjasonsmith
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

thinkyhead commented Nov 16, 2020

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

rhapsodyv commented Nov 17, 2020

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
Copy link
Member

Anyone can test it here? #20176

Thanks

@rhapsodyv
Copy link
Member

The fix was merged. Closing this.

@github-actions
Copy link

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.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants