Skip to content

WIP - Fix up SD / FD media handling#27815

Merged
thinkyhead merged 3 commits intoMarlinFirmware:bugfix-2.1.xfrom
thinkyhead:bf2_basic_media_cleanup_PR
Apr 29, 2025
Merged

WIP - Fix up SD / FD media handling#27815
thinkyhead merged 3 commits intoMarlinFirmware:bugfix-2.1.xfrom
thinkyhead:bf2_basic_media_cleanup_PR

Conversation

@thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Apr 21, 2025

Update CardReader::manage_media to handle more than one device insertion / removal in a logical manner.

  • Track insert/remove states for distinct media drives.
  • Call release() only when the "selected" (and mounted) media is removed.
  • Only mount (select) media on insert when other media is not already mounted.

Improve UI for Multiple Volumes:

  • Clear up cosmetic glitches in file listings on TFT ColorUI (and possibly some other displays).
  • Provide a consistent and simplified menu to attach media, eject media, and select files.
  • Reduce the number of steps required to get to the file selector.
  • Clarify status messages that pertain to specific media devices.

Followup to #21344 by @rhapsodyv

@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch 7 times, most recently from e32d361 to 7307f02 Compare April 23, 2025 02:18
@thisiskeithb thisiskeithb linked an issue Apr 23, 2025 that may be closed by this pull request
1 task
@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch 5 times, most recently from a7f3797 to f1a95af Compare April 23, 2025 18:45
@DaLiV
Copy link

DaLiV commented Apr 23, 2025

tested directly from f1a95af

can note additional minor points.

if SD without USB at boot inserted

  • boot - menu-attach, release, , Insert USB - no USB function
  • boot - remove SD from reader or attach USB without/prior to entry into menu ... works all

otherwise

  • can selectively mount one of another (Attach SD , Attach USB ... one at time /ok/.)

  • some messages with unprecise incorrect meaning :
    ** USB actions = USB Inserted/Removed - Ok
    ** SD Inserted = USB Drive Init Failed
    ** SD Removed = nothing changed on display

  • in Menu for browsing SD - "Show files from USB"

@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch 6 times, most recently from 9680c8e to 4e46d01 Compare April 24, 2025 00:48
@thinkyhead
Copy link
Member Author

tested directly from

Yeah, that commit doesn't have all of the pieces we want yet, so the prior commit is probably more interesting. It mostly keeps the previous menu layout but adds the updated individual drives. The USB Flash drive doesn't return isInserted until it gets initialized, so I'll look at doing some more explicit initialization of the Flash Drive (as is done by one of the TFT displays) regardless of its priority.

I updated the strings to only show "USB" or "SD" on generic media messages when the type of drive is known. Serial messages that hosts might depend on can't be altered, but clarifying messages may be emitted.

For now I don't want to change too much about the configuration,, but I do want to fix a possible reboot issue when inserting a USB Stick, and I want to rework the menu to be more simple. The main menu should show "Print from SD >" and "Print from Flash Drive >" when both drives are present (whether mounted or not). They can just be mounted as-needed as they are with the current "Media Select" screen. "Remove Media" isn't necessarily needed to eject mounted media when we can detect removal. "Change Media" basically only exists for media with no SD Detect.

I'll continue to mess around to get down to as few logical options as possible and then we'll see how much we miss the older layout.

@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch 2 times, most recently from 3f26f4e to c2f4416 Compare April 24, 2025 16:37
@thinkyhead
Copy link
Member Author

CardReader::manage_media doesn't currently handle more than one device insertion / removal properly, so that is another core issue I will aim to fix here.

  • Track insert/remove states for distinct media drives.
  • Call 'release()' only when the "selected" (and mounted) media is removed.
  • Only mount (select) media on insert when other media is not already mounted.

@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch from c2f4416 to 421fa55 Compare April 24, 2025 21:37
@DaLiV
Copy link

DaLiV commented Apr 26, 2025

IMHO:

think "Active media device" must be always only one. at print time - must no any interference occurs. at host-transfers - also only one operation ... no parallel.
but all times on all channels must be only monitored "arrival/removal" of devices (over interrupts ?)
any activation of media may use the same single "Thread" with currently selected "Target". (finish previous, release, init current)

even when shared to host - i see not a case that must go in parallel with printing operation from another media . even if current cpu is enough for such - multitasking here is not a primary point, and multicard - more resource intensive (especially ram-requirements can grow drastically)
i like multitasking, but not at printing (only operative params, and surely not new flash transferring ) ...

preparing for new print from other card ? may be ... but still must make good considerations about that ... who wants jerky prints ... as some timeouts/locks on card processes possible in-between ...

P.S. currently Volume sharing to host is not working for me (somewhat was once long time ago, but was not possible to reproduce - currently always off as that hanging PC explorer ... and breaks com-over-usb)

P.P.S.
however possible forgot some point - logging/debugging ... that case may be requires 2 devices in parallel ...

@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch 9 times, most recently from 24f7948 to a58eddb Compare April 27, 2025 00:15
@thinkyhead
Copy link
Member Author

thinkyhead commented Apr 27, 2025

  • The current model of a single "mounted" drive is fine and we won't change that.
  • The "interrupt" model for UHS works by setting a pin LOW that we monitor from the main loop, so it doesn't interfere with motion timing.
  • The "selected" drive remains selected even after it is physically removed. A determined M22 cancels the media print job.
  • No parallelism is expected, but we respect commands sent by the host during a media print.

currently Volume sharing to host is not working for me

It's not working for me either, but I haven't looked at CDC + MSC yet in conjunction with these updates or tested to see if it works with some older version of Marlin. I will test this tonight to see if it works when MULTI_VOLUME is disabled, with other combinations of options, or with some older version of Marlin, and I will make sure not to merge these changes until volume sharing is functioning as well as it did before.

@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch from a58eddb to 7c2b79b Compare April 27, 2025 06:12
@DaLiV
Copy link

DaLiV commented Apr 27, 2025

volume sharing is interesting and woud nice to see,
BUT not so critical - so can threat (NO_SD_HOST_DRIVE) as another non-blocking bug for this PR.
as that may be even related to much more nuances per cpu/board implementations.

@thinkyhead
Copy link
Member Author

I re-tested a build using the "MSC" environment with DEFAULT_SHARED_VOLUME USB_FLASH_DRIVE and the USB Flash drive mounted on my computer when the card booted up. But then there are some issues.

  • The volume can be simultaneously mounted by Marlin and the PC — not necessarily safe?
  • When the volume is ejected from the PC I can no longer get it to re-mount.
  • When it is the shared volume, sometimes inserting the USB Stick causes the board to reboot — and reboot again…

So, I need to continue working on implementing the best behavior and make sure it complies with considerations we discussed long ago in #14325.

@DaLiV
Copy link

DaLiV commented Apr 27, 2025

has tried too on last commit here - result:
1 drive not mounted in windows, browser slow downs (almost hangs in "rotating icon" )
2 for serial comm i get then: Error 995: The I/O operation has been aborted because of either a thread exit or an application request" (putty)

when NO_SD_HOST_DRIVE is undefined + no matter then what is with MULTI_VOLUME or DEFAULT_SHARED_VOLUME
so if your works, but my not - then or too big diff in some config parts or some board-related things ... i more suppose 2-nd

@thinkyhead
Copy link
Member Author

  1. drive not mounted in windows, browser slow downs (almost hangs in "rotating icon" )
  2. for serial comm i get then: Error 995: The I/O operation has been aborted because of either a thread exit or an application request" (putty)

I also saw the slowdown which implies too much processing is bogging down the main thread idle() function. We do need to call the USB Drive driver to update isInserted(), but that polling should probably be throttled to <= 10Hz to save cycles (if the driver isn't doing this internally already). I will reference the driver documentation (RTFM?!) to see if it has info on how often to call driver->idle(), how to handle the USB Drive / USB various states, etc.

@thinkyhead thinkyhead force-pushed the bf2_basic_media_cleanup_PR branch from a86dad3 to 091de64 Compare April 29, 2025 02:37
@thinkyhead
Copy link
Member Author

thinkyhead commented Apr 29, 2025

I went back to test Marlin code from last week before any recent media-related changes were made and it manifested the same problems with media sharing causing sluggish menus. So there are no new problems introduced by this PR, which is now reduced down to a rework of mounted media states and menu appearance.

We can investigate a cleaner implementation of MSC volume sharing in a separate PR. First important steps include…

  • Detect when a volume is mounted on the PC and mark it as not-available in Marlin.
  • When a device is mounted in Marlin unregister it as an MSC shared device.

@thinkyhead thinkyhead merged commit 85f6090 into MarlinFirmware:bugfix-2.1.x Apr 29, 2025
66 checks passed
@thinkyhead thinkyhead deleted the bf2_basic_media_cleanup_PR branch April 29, 2025 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] USB flash drive support broken

2 participants