Fix SD pins for SKR Pro and GTR #19047

sjasonsmith · 2020-08-16T23:45:19Z

Description

Define SD_DETECT differently depending on the value of SDCARD_CONNECTION.

For the GTR, I modified it to use the same layout as the SKR Pro. It was previously incorrect regarding which SPI is enabled in the variant file, and was not properly switching pins around to allow both LCD and ONBOARD LCD to work.

GTR also had an issue where inserting a card into the the ONBOARD slot could freeze the LCD display, even though they were physically disconnected SPI buses.

Benefits

On both boards, you can now select either ONBOARD or LCD in the Configuration_adv.h.
Allows SD detection when using either mode.

I also moved where the pull on the SD_DETECT pin is configured. This was previously initialized in the LCD code instead of the SD code, so it did not work properly if you had an external "LCD" SD Reader, without actually configuring a display.

Related Issues

#17906 - [BUG] BTT SKR PRO onboard SD Card - detect pin still used from LCD (bugfix-2.0)
#18671 - The SD card on the LCD FYSETC_MINI_12864 screen cannot be read on the SKR Pro V1.1 motherboard (and GTR)

sjasonsmith · 2020-08-17T05:43:29Z

I added similar modifications to the GTR board. There was already some attempt to do this in the GTR file, but it did not work at all for me.

The behavior on the GTR prior to my change was that inserting a card into the ONBOARD SD would freeze the LCD, and generally things with mixed up between the two. Previously I has probably never tried to use ONBOARD SD with these.

sjasonsmith · 2020-08-17T05:49:57Z

If anybody is using a GTR, it would be great if you could test with this to ensure it doesn't break functionality with your screen.

GhostlyCrowd · 2020-08-17T14:03:13Z

Hoping to test this once your other PR is merged.

sjasonsmith · 2020-08-17T14:11:32Z

There were some comments on Discord I need to look back at prior to this merging.
https://discord.com/channels/461605380783472640/491161257303474176/744825858740584448

giryan · 2020-08-18T06:26:19Z

I have a BTT GTR, and prior to cherry picking my config changes onto the PR branch I could print from SD, afterwards Marlin doesn't think an SD card is inserted('Attach Media' message on LCD/'No SD card' message over serial)

Configuration and Configuration_adv:
GTR_Config_Marlin.zip

giryan · 2020-08-18T06:43:14Z

I have a BTT GTR, and prior to cherry picking my config changes onto the PR branch I could print from SD, afterwards Marlin doesn't think an SD card is inserted('Attach Media' message on LCD/'No SD card' message over serial)

Configuration and Configuration_adv:
GTR_Config_Marlin.zip

I have spotted my issue, there is no default SDCARD_CONNECTION define in pins_BTT_GTR_V1_0.h, so by default SD won't work.
I guess there needs to be a sanity check for that?
#if !SDCARD_CONNECTION && SDSUPPORT #error you need to say where the card is? ;)
This Configuration_adv.h adds #define SDCARD_CONNECTION ONBOARD and the SD card is now accessible in Pronterface
Configuration_adv.h.txt

thisiskeithb · 2020-08-18T07:09:00Z

Here's how it's handled in the SKR 1.3:

Marlin/Marlin/src/pins/lpc1768/pins_BTT_SKR_V1_3.h

Lines 359 to 361 in c55f477

    
           #ifndef SDCARD_CONNECTION 
        
             #define SDCARD_CONNECTION                  LCD 
        
           #endif

sjasonsmith · 2020-08-18T07:22:32Z

Here's how it's handled in the SKR 1.3:

@thisiskeithb do you know why that is? Does everybody have SDs on their LCDs? It seems strange that is the default, even if there is no LCD!

It seems like the sensible default would be the on-board port, since it is always there?

thisiskeithb · 2020-08-18T09:11:55Z

It seems like the sensible default would be the on-board port, since it is always there?

I believe that's how it was initially, but was changed at some point.

SDCARD_CONNECTION LCD is the default on several boards now.

sjasonsmith · 2020-08-18T14:33:05Z

I think I prefer the recommendation to just fail the build with an error if they enable SD without defining this.

thisiskeithb · 2020-08-18T15:13:41Z

I think I prefer the recommendation to just fail the build with an error if they enable SD without defining this.

Do you want to change the other pins files as well for consistency?

thisiskeithb · 2020-08-18T22:42:24Z

Testing an SKR Pro 1.1 with REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER, changing the SDCARD_CONNECTION setting appears to work correctly, but I noticed that the pins file already has a SD_CONNECTION_IS(LCD) block, it's just buried:

Marlin/Marlin/src/pins/stm32f4/pins_BTT_SKR_PRO_common.h

Lines 307 to 310 in 47d7258

    
           #if SD_CONNECTION_IS(LCD) 
        
             #define SD_DETECT_PIN                 PF12 
        
             #define LCD_SDSS                      PB12 
        
           #endif

This was easier to notice when I defined MKS_MINI_12864 because scrolling with the encoder didn't work, SD detection didn't work, etc., so there are some other outstanding issues there that I'm still working out. Link to pinout for reference.

I'll keep testing...

GhostlyCrowd · 2020-08-19T23:48:04Z

This is working fine for me on the SKR Pro 1.1

thisiskeithb · 2020-08-20T00:06:55Z

This is working fine for me on the SKR Pro 1.1

Which LCDs did you try?

GhostlyCrowd · 2020-08-20T00:41:03Z

This is working fine for me on the SKR Pro 1.1

Which LCDs did you try?

I'm using a BTT TFT35 V3, which emulates a Reprap Discount smart display in marlin mode and is what i have it configured as in marlin.

Choosing Onboard, the SKR Pro's SD card is now usable and the lcd is working in a normal manner, and Choosing LCD, the SD card on the BTT TFT35 V3's SD slot works in marlin mode.

Ran a print from it and it appears to be fine.

GhostlyCrowd · 2020-08-20T03:14:16Z

Just noticed that marlin does not detect the card being inserted or removed showing " Media connected " or "media removed " like it does with the LCD sd card, but the Onboard is in fact working now at least.

So sd detection is not in fact working on the SKR Pro but the onboard sd now works which didnt before.

thisiskeithb · 2020-08-20T03:22:13Z

I had to add #define SD_DETECT_PIN PF12 to the #if SD_CONNECTION_IS(LCD) block.

GhostlyCrowd · 2020-08-20T03:41:52Z

I had to add #define SD_DETECT_PIN PF12 to the #if SD_CONNECTION_IS(LCD) block.

I didnt have to do this to get the SD on the lcd working however as stated above the detection or onboard is not working but printing from sd does

sjasonsmith · 2020-08-20T03:57:32Z

I believe I had the detect pin working for both, but ~~mate. That~~ maybe that was just the GTR? I know I had to uncomment the line below you specify ONBOARD. Something like NO_HOST_SD. If that is not “enabled” the detect pins gets undefined in on of the conditionals files.

GhostlyCrowd · 2020-08-20T04:11:03Z

I believe I had the detect pin working for both, but mate. That was just the GTR? I know I had to uncomment the line below you specify ONBOARD. Something like NO_HOST_SD. If that is not “enabled” the detect pins gets undefined in on of the conditionals files.

This makes sense and in fact fixed the non "media detected / removed"

That might throw some people off. any way to warn them so they dont file an erroneous bug report?

GhostlyCrowd · 2020-08-20T05:48:43Z

@thisiskeithb can you try flashing firmware.bin on the GTR from onboard sd with it enabled in marlin via the M997 command see if it fails for you as well? M997 does not work for me if i have #define SDCARD_CONNECTION ONBOARD but the sd works for printing and what not. The board ends up in a frozen blank screen state with M997 it never flashes the bin.

thinkyhead · 2020-08-21T22:12:07Z

Is this considered good enough and at least fixing the issue now? Or, does it need more review and testing.

thisiskeithb · 2020-08-21T22:14:14Z

This still needs more work.

sjasonsmith · 2020-08-22T18:35:24Z

These are the combinations I'm going to test and verify work as expected:
(I will be updating this based on my own testing and development, NOT the current state of this branch. The table below does not reflect the current state of the work until I have pushed new changes after it.

Board	Display	ONBOARD	LCD
GTR	External SD, no display configured	✔️	✔️
GTR	RepRapDiscount FullGraphics	✔️	✔️
GTR	FYSETC Mini	✔️	✔️
GTR	MKS Mini	✔️	✔️
GTR	CR10	✔️	N/A
SKR Pro	External SD, no display configured	✔️	✔️
SKR Pro	RepRapDiscount FullGraphics	✔️	✔️
SKR Pro	FYSETC Mini	✔️	✔️
SKR Pro	MKS Mini	✔️	✔️
SKR Pro	CR10	✔️	N/A

Board	SDCARD_CONNECTION when Undefined
GTR	ONBOARD (Matches previous behavior)
SKR Pro	LCD (Matches previous behavior)

…ass, to enable SD without display

sjasonsmith · 2020-08-23T06:19:35Z

I completely replaced the earlier solution, and methodically went through one display at a time making sure they were working.

I tried to make the GTR and SKR Pro configurations as similar as possible. There were some oddities where they didn't behave the same, and I ended up forcing SOFTWARE_SPI for the RepRapDiscount display on the SKR Pro, but not on the GTR.

In the end, everything seems to be working on these three displays, with both LCD and ONBOARD SD selected.

Maybe @thisiskeithb and @GhostlyCrowd can sanity check with the configurations they had problems with before.

You still need to uncomment NO_SD_HOST_DRIVE if you are using the ONBOARD SD if you want the detect pin to work. I decided not to change anything about that in this PR.

sjasonsmith · 2020-08-23T06:22:25Z

This detail in the description is significant, so I'll repeat it here:

I also moved where the pull on the SD_DETECT pin is configured. This was previously initialized in the LCD code instead of the SD code, so it did not work properly if you had an external "LCD" SD Reader, without actually enabling a display.

sjasonsmith · 2020-08-23T06:25:18Z

@thinkyhead I just marked this ready for review. It looks like CI builds are completely broken, but since this primarily impacts 2 pins file I'm not expecting build failures.

GhostlyCrowd · 2020-08-23T13:33:12Z

Onboard SD and LCD SD working here with proper detection for SKR pro and BBT TFT35 V3 (which emulates RepRapDiscount FullGraphics Smart Display)

Only Caveat is that M997 rebooting for flashing from ONBOARD AND the reset button are broken on the SKR Pro when using ONBOARD SD and their is a SD in the slot, the system doesn't reboot properly and gets stuck in a weird state with a blank screen, nor will it mount the onboard SD and flash firmware.bin. It must hang trying to mount the SD to check if firmware.bin is present and never finishes this routine. Power cycle solves this.

sjasonsmith · 2020-08-23T16:41:55Z

@GhostlyCrowd, did you earlIer say that the M997 issue also existed before my changes?
If so, then I don’t think it should block this PR.

GhostlyCrowd · 2020-08-23T17:02:39Z

@GhostlyCrowd, did you earlIer say that the M997 issue also existed before my changes?
If so, then I don’t think it should block this PR.

I don't know if it existed before this PR As onboard SD never worked for me previously, i can Try fresh bug fix and see if the onboard SD works and if the issue is there.

GhostlyCrowd · 2020-08-23T17:21:44Z

So current bugfix Onboard SD still points to LCD SD for me, M997 seems to work but thats because onboard SD is never used. SO thats what i've found, I don't think M997 not working on the SKR pro should hold this back as it has never worked properly given its usefulness is only if you can access on board sd send the firmware and flash it via Gcode

sjasonsmith · 2020-08-23T19:28:42Z

It bothered me that overriding pins to use hardware SPI worked on the GTR but not the SKR Pro. Looking at the experimental Multi-SPI PR (#16260) I was able to find the reason, and added a comment to the pins file documenting it.

I confirmed that if I modify the HAL to use SPI Mode 3 then it can work, but decided it is best to wait for other multi-device SPI improvements rather than adding yet another hack for this one board.

rhapsodyv · 2020-08-25T22:40:39Z

It bothered me that overriding pins to use hardware SPI worked on the GTR but not the SKR Pro. Looking at the experimental Multi-SPI PR (#16260) I was able to find the reason, and added a comment to the pins file documenting it.

I confirmed that if I modify the HAL to use SPI Mode 3 then it can work, but decided it is best to wait for other multi-device SPI improvements rather than adding yet another hack for this one board.

@sjasonsmith I just pushed the multi data mode support for hardware SPI in LPC. We can try test it.

sjasonsmith · 2020-08-25T23:15:46Z

@rhapsodyv these boards both use HAL/STM32.

rhapsodyv · 2020-08-25T23:27:28Z

🤦🏻‍♂️sorry,I confused it with "skr turbo", which is lpc! Haha

…K8800-2.0.x * tag '2.0.6.1' of https://github.com/MarlinFirmware/Marlin: (195 commits) Version 2.0.6.1 [cron] Bump distribution date (2020-08-28) Add set_all_homed Mark axes not-homed with HOME_AFTER_DEACTIVATE (MarlinFirmware#18907) set_axis_not_trusted => set_axis_never_homed Independent Neopixel option (MarlinFirmware#19115) Fix Creality V4 probe pin Fix small typø Allocate sufficient MSG_MOVE_Z_DIST buffer One MARLIN_DEV_MODE warning per rebuild (MarlinFirmware#19163) FYSETC S6 2.0 (MarlinFirmware#19140) [cron] Bump distribution date (2020-08-27) Fix SINGLENOZZLE fan speed bug (MarlinFirmware#19152) Fix NEOPIXEL_STARTUP_TEST last delay (MarlinFirmware#19156) TFT (plus Hardware SPI) for LPC (MarlinFirmware#19139) Prusa => Průša Direct Stepping update (MarlinFirmware#19135) Fixes to FTDI Touch UI (MarlinFirmware#19134) Add Einsy Rambo Filament Runout Pin (MarlinFirmware#19136) Fix SD pins for SKR Pro and GTR (MarlinFirmware#19047) ...

robertluo168 · 2020-09-07T09:41:50Z

unsubscribe

>>>> Original Message <<<<<

From: MarlinFirmware/Marlin To: MarlinFirmware/Marlin Cc: Subscribed Sent Date: Mon,17 Aug 2020 22:03:44 Asia/Taipei Subject: Re: [MarlinFirmware/Marlin] Fix SD pins for SKR Pro and GTR (#19047) Hoping to test this once your other PR is merged. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

sjasonsmith requested a review from thisiskeithb August 16, 2020 23:59

sjasonsmith mentioned this pull request Aug 17, 2020

[BUG] The SD card on the LCD FYSETC_MINI_12864 screen cannot be read on the SKR Pro V1.1 motherboard #18671

Closed

sjasonsmith changed the title ~~Fix SD_DETECT for onboard SPI on SKR Pro~~ Fix SD_DETECT pins for SKR Pro and GTR Aug 17, 2020

sjasonsmith changed the title ~~Fix SD_DETECT pins for SKR Pro and GTR~~ Fix SD pins for SKR Pro and GTR Aug 17, 2020

sjasonsmith added the Needs: Testing Testing is needed for this change label Aug 17, 2020

sjasonsmith marked this pull request as draft August 18, 2020 06:28

thisiskeithb removed their request for review August 18, 2020 22:47

sjasonsmith removed the Needs: Testing Testing is needed for this change label Aug 22, 2020

sjasonsmith added 2 commits August 22, 2020 23:11

Initialize SD_DETECT Pullup in CardReader class instead of display cl…

5876ffa

…ass, to enable SD without display

Improve SD configuration for SKR Pro and GTR

7c65de4

sjasonsmith force-pushed the PR/17906_SKR_PRO_SD_DETECT branch from 47d7258 to 7c65de4 Compare August 23, 2020 06:15

sjasonsmith removed the Needs: Work More work is needed label Aug 23, 2020

sjasonsmith marked this pull request as ready for review August 23, 2020 06:25

Add detail to SKR Pro Software SPI comments.

1a7da6d

rhapsodyv mentioned this pull request Aug 24, 2020

Marlin TFT port to LPC + Bonus: Full Hardware SPI Support #19139

Merged

6 tasks

thisiskeithb added the C: Boards/Pins label Aug 25, 2020

Merge remote-tracking branch 'upstream/bugfix-2.0.x' into pr/19047

2e22c62

thinkyhead merged commit 646d90f into MarlinFirmware:bugfix-2.0.x Aug 26, 2020

albertogg pushed a commit to albertogg/Marlin that referenced this pull request Aug 31, 2020

Fix SD pins for SKR Pro and GTR (MarlinFirmware#19047)

26e0d42

thinkyhead pushed a commit to thinkyhead/Marlin that referenced this pull request Sep 2, 2020

Fix SD pins for SKR Pro and GTR (MarlinFirmware#19047)

67d3100

sjasonsmith deleted the PR/17906_SKR_PRO_SD_DETECT branch November 23, 2020 09:21

vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request Dec 9, 2020

Fix SD pins for SKR Pro and GTR (MarlinFirmware#19047)

33e5922

kageurufu pushed a commit to CR30-Users/Marlin-CR30 that referenced this pull request Apr 30, 2021

Fix SD pins for SKR Pro and GTR (MarlinFirmware#19047)

6043f5d

Fix SD pins for SKR Pro and GTR #19047

Fix SD pins for SKR Pro and GTR #19047

Conversation

sjasonsmith commented Aug 16, 2020 • edited Loading

Description

Benefits

Related Issues

sjasonsmith commented Aug 17, 2020

sjasonsmith commented Aug 17, 2020

GhostlyCrowd commented Aug 17, 2020

sjasonsmith commented Aug 17, 2020

giryan commented Aug 18, 2020

giryan commented Aug 18, 2020

thisiskeithb commented Aug 18, 2020

sjasonsmith commented Aug 18, 2020

thisiskeithb commented Aug 18, 2020

sjasonsmith commented Aug 18, 2020

thisiskeithb commented Aug 18, 2020

thisiskeithb commented Aug 18, 2020 • edited Loading

GhostlyCrowd commented Aug 19, 2020

thisiskeithb commented Aug 20, 2020

GhostlyCrowd commented Aug 20, 2020

GhostlyCrowd commented Aug 20, 2020

thisiskeithb commented Aug 20, 2020

GhostlyCrowd commented Aug 20, 2020

sjasonsmith commented Aug 20, 2020 • edited Loading

GhostlyCrowd commented Aug 20, 2020

GhostlyCrowd commented Aug 20, 2020

thinkyhead commented Aug 21, 2020

thisiskeithb commented Aug 21, 2020

sjasonsmith commented Aug 22, 2020 • edited Loading

sjasonsmith commented Aug 23, 2020

sjasonsmith commented Aug 23, 2020

sjasonsmith commented Aug 23, 2020

GhostlyCrowd commented Aug 23, 2020

sjasonsmith commented Aug 23, 2020

GhostlyCrowd commented Aug 23, 2020

GhostlyCrowd commented Aug 23, 2020

sjasonsmith commented Aug 23, 2020 • edited Loading

rhapsodyv commented Aug 25, 2020

sjasonsmith commented Aug 25, 2020

rhapsodyv commented Aug 25, 2020

robertluo168 commented Sep 7, 2020 via email

sjasonsmith commented Aug 16, 2020 •

edited

Loading

thisiskeithb commented Aug 18, 2020 •

edited

Loading

sjasonsmith commented Aug 20, 2020 •

edited

Loading

sjasonsmith commented Aug 22, 2020 •

edited

Loading

sjasonsmith commented Aug 23, 2020 •

edited

Loading