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

Go to File Browser on Media Insert (option) #20151

Merged
merged 7 commits into from
Nov 18, 2020

Conversation

niccoreyes
Copy link
Contributor

@niccoreyes niccoreyes commented Nov 15, 2020

Requirements

  • Filling out this template is required. Pull Requests without a clear description may be closed at the maintainers' discretion.

Description

Added SD_AUTOOPEN_MENU to the Configuration_adv.h when plugged so you don't have to scroll all the way down for those using the old classic lcd screens when you plug an SD card and open Print from media. This has only been tested on a REPRAP_DISCOUNT_SMART_CONTROLLER type of lcd.

video here
https://youtu.be/HLcYCQ0gGkA

Benefits

Better Quality of Life with SD card insert and ejection

Configurations

Configuration.h
REPRAP_DISCOUNT_SMART_CONTROLLER

Configuration_adv.h
Enable SDSUPPORT
and uncomment
SD_AUTOOPEN_MENU

Related Issues

This was a feature originally part of the prusa firmware and I just added a few lines to do the same function. I'm not sure if it might conflict with other features / lcds yet

EDIT: I don't think this should work with the SD card password feature. I should make a sanity check soon. Done.

Added SD_AUTOOPEN_MENU to the Configuration_adv.h when plugged so you don't have to scroll all the way down for those using the old classic lcd screens when you plug an SD card and open Print from media.

This was a feature originally part of the prusa firmware and I just added a few lines to do the same function. I'm not sure if it might conflict with other features yet.
added sanitycheck if PASSWORD_FEATURE is enabled.
It's untested but I have a gut feeling problems might pop up with the PASSWORD_FEATURE
Copy link
Contributor

@qwewer0 qwewer0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't BOTH be used instead of ENABLED && ENABLED?

Marlin/src/inc/SanityCheck.h Outdated Show resolved Hide resolved
@qwewer0
Copy link
Contributor

qwewer0 commented Nov 15, 2020

It doesn't seem to work on Ender 3, SKR Mini E3 v1.2, CR10_STOCKDISPLAY.

Configuration.zip

@niccoreyes
Copy link
Contributor Author

niccoreyes commented Nov 16, 2020

It doesn't seem to work on Ender 3, SKR Mini E3 v1.2, CR10_STOCKDISPLAY.

Configuration.zip

Hmm, I'm not sure how to test/investigate without the hardware on hand, I don't know how the CR10_STOCKDISPLAY handles sd insert or eject events but I will compiling a config on a friend's CR10S - though which still uses REPRAP_DISCOUNT_SMART_CONTROLLER, this MIGHT be a feature working only for the REPRAP_DISCOUNT_SMART_CONTROLLER or similar at the moment

video for CR10S that is using REPRAP_DISCOUNT_SMART_CONTROLLER
https://youtu.be/J0ptqL2qqOY

Configuration.zip

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 16, 2020

In the videos the SD cards are inserted in to the display directly?

@niccoreyes
Copy link
Contributor Author

In the videos the SD cards are inserted in to the display directly?

yeah

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 16, 2020

yeah

I used the boards SD slot, so that might be why it doesn't worked for me.

@sjasonsmith
Copy link
Contributor

@qwewer0, make sure you have set both of the following. The DETECT pin gets disabled if the second is not set.

#define SDCARD_CONNECTION ONBOARD
#define NO_SD_HOST_DRIVE

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 16, 2020

@qwewer0, make sure you have set both of the following. The DETECT pin gets disabled if the second is not set.

#define SDCARD_CONNECTION ONBOARD
#define NO_SD_HOST_DRIVE

Already had SDCARD_CONNECTION ONBOARD, but not NO_SD_HOST_DRIVE. After enabling NO_SD_HOST_DRIVE, SD_AUTOOPEN_MENU now works!

@sjasonsmith
Copy link
Contributor

@qwewer0 were you building with one of the _USB environments? If not, perhaps you should report an issue about this. You shouldn't have to declare NO_SD_HOST_DRIVE if you are building in a way which already cannot do it!

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 16, 2020

@qwewer0 were you building with one of the _USB environments? If not, perhaps you should report an issue about this. You shouldn't have to declare NO_SD_HOST_DRIVE if you are building in a way which already cannot do it!

I used STM32F103RC_btt_512K.
So it should have worked without NO_SD_HOST_DRIVE on a non _USB environment?

@sjasonsmith
Copy link
Contributor

I used STM32F103RC_btt_512K.
So it should have worked without NO_SD_HOST_DRIVE on a non _USB environment?

As it is today, no... but that feels wrong. It seems odd to have to specify NO_SD_HOST_DRIVE on a platform that already cannot provide it.

@sjasonsmith
Copy link
Contributor

It is a usability issue that everybody is getting wrong, including myself. From that perspective I consider it to be a bug.

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 16, 2020

Hope I gave enough information on the issue to the problem between this PR and STM32F103RC_btt_512K/STM32F103RC_btt environments.

@rhapsodyv
Copy link
Member

I think we need sanity check for that, to use only with LCD Controllers what support marlin ui.

And I two question: anyone tested inside a deep menu screen, like some edit screen? And using Color UI?

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 17, 2020

If I'm in an edit menu (e.g. probe offset):

  • SD card is inserted: The print menu is opened, then if Main (back) menu item is selected, it goes back to the outside of the original edit screen where the SD card was inserted.
  • SD card is removed: It goes back out to the status screen.

When SD card is inserted, is there a way to have the "back" menu item in the printing menu to say where it would lead and not just the default Main?

Tested on: Ender 3, SKR Mini E3, CR10_STOCKDISPLAY

@niccoreyes
Copy link
Contributor Author

If I'm in an edit menu (e.g. probe offset):

  • SD card is inserted: The print menu is opened, then if Main (back) menu item is selected, it goes back to the outside of the original edit screen where the SD card was inserted.
  • SD card is removed: It goes back out to the status screen.

When SD card is inserted, is there a way to have the "back" menu item in the printing menu to say where it would lead and not just the default Main?

Tested on: Ender 3, SKR Mini E3, CR10_STOCKDISPLAY

Thanks for testing that! hmm, will have to study the edit menu function structures and will check if I can pass arguments to go back to the "last state" the edit menu was on

@rhapsodyv
Copy link
Member

@niccoreyes I don't think we really need a "go back" feature. When I asked for testing on deep menus, it's just to check if it would not put marlin in some wrong state. If it's working, I'm ok. We don't need make it harder and complex with a "go back" feature.

I just like to check if it will work with Color UI too. Because Color UI is exactly the same as marlin menus, but for touch.

@thinkyhead
Copy link
Member

As a quick patch, code can be added which checks whether the last item in the nav history is the main menu screen, and if it isn't, show "Back" instead of "Main".

@thinkyhead thinkyhead changed the title Automatically open sd card menu when inserted like Prusa Firmware Go to File Browser on Media Insert (option) Nov 18, 2020
@thinkyhead thinkyhead merged commit ecd8227 into MarlinFirmware:bugfix-2.0.x Nov 18, 2020
@qwewer0
Copy link
Contributor

qwewer0 commented Nov 18, 2020

  1. When SD card is inserted at Status or Main screen, it shows back , and with other menus it shows main.
  2. When SD card is inserted the LCD on the Status screen doesn't show Media inserted, only shows Media removed if the SD card is removed.
  3. While it is easier to have the back menu item to take us one folder up from where the media menu is called, I think it is still would be better to go back to the previous menu where the media menu is called. If the previous menu was an edit menu, maybe the best would be to go back outside of that, rather than back to the edit screen.

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 18, 2020

One more thing that I think would be good idea to add, is to not go to the status screen when SD card is removed, only go back one up or go to status screen when inside the media menu.

@qwewer0
Copy link
Contributor

qwewer0 commented Nov 18, 2020

Changing

BACK_ITEM_P(TERN1(BROWSE_MEDIA_ON_INSERT, screen_history_depth) ? GET_TEXT(MSG_MAIN) : GET_TEXT(MSG_BACK));

to

BACK_ITEM_P(TERN0(BROWSE_MEDIA_ON_INSERT, screen_history_depth) ? GET_TEXT(MSG_BACK) : GET_TEXT(MSG_MAIN));

makes that on the status screen and main menu the back menu shows main, and back for others menus.
But it sill not right to have main as back menu when it would go back to the status screen, so maybe is should just be back for everything.

Not sure if this would work for that:

BACK_ITEM_P(TERN(BROWSE_MEDIA_ON_INSERT, GET_TEXT(MSG_BACK), GET_TEXT(MSG_MAIN)));
```</s>

FhlostonParadise pushed a commit to FhlostonParadise/Marlin that referenced this pull request Nov 21, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Kannix2005 pushed a commit to Kannix2005/Marlin-1 that referenced this pull request Dec 7, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request Dec 9, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
@J-X-C
Copy link

J-X-C commented Dec 11, 2020

I have an Ender 3 with v4.2.7 board and the CR10 display with the wheel. A nice tweak would be when the media is inserted after being removed to update the main screen to remove the media removed message! I am looking a bit, should I submit this in some officially formatted way?

@niccoreyes
Copy link
Contributor Author

I have an Ender 3 with v4.2.7 board and the CR10 display with the wheel. A nice tweak would be when the media is inserted after being removed to update the main screen to remove the media removed message! I am looking a bit, should I submit this in some officially formatted way?

it is possible, but I haven't gotten back to coding yet, you could submit a feature request if someone could pick up on it
I can already think of which sections to add a few lines of code to but I'm currently busy working on a different project at the moment...

tharts pushed a commit to tharts/Marlin that referenced this pull request Jan 6, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
dpreed pushed a commit to dpreed/Marlin_2.0.x that referenced this pull request Feb 5, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 28, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit that referenced this pull request Apr 30, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.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.

6 participants