Skip to content

Remove 'Media Released' message#17272

Merged
thinkyhead merged 3 commits intoMarlinFirmware:bugfix-2.0.xfrom
mathiasvr:remove-media-released
Apr 20, 2020
Merged

Remove 'Media Released' message#17272
thinkyhead merged 3 commits intoMarlinFirmware:bugfix-2.0.xfrom
mathiasvr:remove-media-released

Conversation

@mathiasvr
Copy link
Contributor

Description

I propose to remove the Media Released message from the LCD menu, as I find it unnecessary.
The Attach Media menu should be sufficient to convey that the media is released.
Also, since it is represented as a menu item and not a status message, some users might find it confusing and think something should happen when clicking it (or that something happens in the background?).

Benefits

Declutters the LCD menu by removing an item that does nothing.

@thinkyhead
Copy link
Member

I agree that the item is kind of strange, but I think it's meant to be the analog for the ACTION_ITEM(MSG_NO_MEDIA, nullptr) that applies for auto-detected SD cards (which can't be "released" or "detached" by a menu item). I believe the idea is that if the menu is on-screen when the state of the card changes the screen won't scroll or alter the current item selection (and it allows the status change to be seen).

Try seeing how the menu looks both with and without SD_DETECT_PIN, and both with and without an onboard SD card, and see whether it makes sense to eliminate altogether, or just refine.

@mathiasvr
Copy link
Contributor Author

I think it might be fine to have the MSG_NO_MEDIA item for auto-detected SD cards as it is more clear. I would still prefer that the counterpart to that would simply be the Attach Media menu and not two items.

Now I noticed that auto-detected SD cards cannot be released but instead has the MSG_CHANGE_MEDIA message. Is that necessary? Is it not sufficient to use the refresh item in the Media Menu instead?

@thinkyhead
Copy link
Member

MSG_CHANGE_MEDIA

With an SD_DETECT_PIN the printer is expected to know that you swapped cards. If you want to force the printer to notice, click the M21 item labeled as MSG_CHANGE_MEDIA. If no card is present then it will reflect that after trying to init it.

@mathiasvr
Copy link
Contributor Author

I think I understand, but would it be a problem to force this check initially when clicking the Media Menu? This way we could remove MSG_CHANGE_MEDIA as well. I think this item could actually be even more confusing for some users. Anyway, I'm clearly not a fan of having extra menu items :)

@thinkyhead
Copy link
Member

thinkyhead commented Mar 24, 2020

If your SD detect pin is working correctly then the SD card in the slot should already be mounted at startup. If no SD detect pin exists, Marlin just tries to mount the card anyway.

@mathiasvr
Copy link
Contributor Author

Unless I'm missing something the MSG_CHANGE_MEDIA is still displayed if the detect pin is working and the card is mounted right? In that case, I don't see much difference. I might have to try it out though.

@thinkyhead
Copy link
Member

…noticed that auto-detected SD cards cannot be released…
Is it not sufficient to use the refresh item in the Media Menu instead?

  • If the media is acting up, going into the SD card listing might not work.
  • The "Refresh" item is only shown when you have no SD detect pin (-1).

Years of history behind decisions that are hard to understand on first encounter.

@mathiasvr
Copy link
Contributor Author

mathiasvr commented Mar 26, 2020

Sorry I have to ask, but I have personally moved this line up above the release/change media menu item, because I thought it was nicer:

SUBMENU(MSG_MEDIA_MENU, menu_media);

However, this is apparently already the case for condition !HAS_ENCODER_WHEEL here:

SUBMENU(MSG_MEDIA_MENU, menu_media);

What is the reason that they are handled differently?

@thinkyhead
Copy link
Member

With an encoder wheel the hardest-to-miss positions in the menu are the top and the bottom.

@thinkyhead thinkyhead merged commit 875cd4e into MarlinFirmware:bugfix-2.0.x Apr 20, 2020
@mathiasvr mathiasvr deleted the remove-media-released branch April 20, 2020 20:04
jmp0x0000 pushed a commit to jmp0x0000/Marlin that referenced this pull request Aug 7, 2020
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
njibhu pushed a commit to njibhu/Marlin that referenced this pull request Aug 24, 2020
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
HairingX pushed a commit to HairingX/Marlin that referenced this pull request Jun 16, 2021
Co-authored-by: Scott Lahteine <github@thinkyhead.com>
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.

2 participants

Comments