Update SD card connection / sharing options#14325
Update SD card connection / sharing options#14325thinkyhead merged 10 commits intoMarlinFirmware:bugfix-2.0.xfrom
Conversation
|
that's a bum change. I need the SD over USB first before I go to print something. having commented out SD exclusivity until printing it doesn't affect me much yet but I'm basically going to have to reverse this in my builds. I think a better change would be config option to mount SD at startup which then turns off mounting SD over USB at startup. |
|
@forkoz Well that is a bigger issue and will need input from @thinkyhead this change is simply to make things work as well as they can with how things are at the moment. I do agree that having a simple configuration option to decide if the card should be mounted at boot time makes sense. That may also help folks that do not have a display. This is especially true if it turns out that Macs don't like SD cards coming and going (which looks like it may be the case). |
|
The recent change I made was to help out in the case where you have enabled
So, is it the case for all such boards that mounting the SD card associated with the Instead of this change, we should just check the new SD flags that we set in the pins files and if there is also an on-board SD card then I will skip the new mounting behavior. That will be better for almost all 32-bit boards. |
|
So… which options should we check before deciding to mount the SD card reader accessory at startup? Here are the candidates… So maybe Marlin should only try to mount the SD card when… NONE(SHARED_SD_CARD, LPC_SD_ONBOARD) \
&& (ENABLED(USB_SD_DISABLED) || !PIN_EXISTS(ONBOARD_SD_CS)) |
|
@thinkyhead Yes having both the USB code and Marlin access the SD card at the same time is not safe, so if Marlin mounts the card at boot time then the USB code should not, I think that will be true for all cards that can share the card via USB. In theory it should be fine to have Marlin mount the card at boot and have the USB code present an "empty" sd card reader to the host, then switch things later if required. However although this works fine on WIndows/LInux it looks like a Mac may not like it. Also I think allowing the user to choose if the card should be available to the host or to Marlin at boot would be nice and I think this change allows that? A couple of points on the changes...
Final thought, do you think we should drop the LPC_ prefix for these settings? I suspect that we will need similar settings for other boards at some point. |
No. How about mounting as if the write protect on the sd-card is set until some wants to write? How about disabling write access (writing and deleting) for Marlin completely, if the card can be accessed via USB (and EEPROM simulation and Power Loss Recovery are solved otherwise). Can we protect the blocks used by the files we have to write to by file system flags (readonly, system, hidden, ...)? The files, we have to write to, are more or less fix in size. Can we effectively protect the file Marlin reads from by setting 'readonly' while printing? Is there a mechanism we can use to tell the USB side something has changed - so caches are invalid? Simulated 'card detect'? |
Well this is pretty much what we have had until now and so far we have had very few complaints. Anything other than pure read only for both is probably heading for problems (and certainly complexity) at some point. Pure read only for both does not seem very useful. You are very limited in terms of communicating things to the host system, basically you can say the disk has been removed or inserted. But that tends to trigger a lot of activity from most host systems often the reading of many thousands of blocks so it can be slow. Also the way that different host systems react to this sort of thing seems to vary. In the case of the Mac it seems it does not like change at all! Honestly if you really want a shared file system then it would be better to use something like MTP (rather than MSC) as the sharing mechanism, that works at a file level and is designed for this sort of thing. But when I last looked there did not seem to be any open source MTP implementation available and it needs more resources to manage the file/handle mapping. |
|
@thinkyhead I've just pushed some changes to the shared card logic and also allowed for the user to force mounting of the shared SD card by Marlin. Please review. I've tested most of the configurations and with this logic they seem to do what I would expect. The SD card will be mounted by Marlin on boot in all cases except when the card is shared. In that case the card will be made available to the host. In the case of both card readers being enabled Marlin will mount the LCD card and the host will mount the onboard card. |
That (especially in Marlin) is not an indicator for a initially good concept for what to do - just for a simple / fast / easy / more or less working implementation. It just was done by someone, Everyone was glad it worked somehow. But nobody else thought about that in dept. At least that's the usual way here. |
|
I've had few issues sharing the SD between marlin and the host since the crashes have been fixed. I only make marlin de-host the SD when printing. Between prints I regularly delete files / add new ones then hit refresh. Only instances of legit corruption were when the printer crashed. Scandisk fixed it. Occasionally the scrolling long file names feature will meld names together after a bunch of deletes but I'm not sure if that is a bug in that feature or the file system. The way it is stock causes extra work since the whole arm is cut off when marlin mounts the card. Only benefit is possible saving of files I will use only once, maybe twice at best. In a multi setting/material world the gcode isn't getting re-used much at least in my opinion. I might sing a different tune if I still used eeprom.dat, except those settings are being purged now whenever I flash new fw anyway. Both readers used is an interesting concept. One would be for settings/fw and the other for gcode? If so, marlin would have to swap around cards with the host and itself in any case since you want to be able to add fw/gcode to the appropriate place but also read settings. Or possibly you only share the fw card and manually load the gcode SD, in which case andy's solution works well. On a personal level I haven't removed the SD in months and do everything over the cable, others may be doing the same via octoprint/esp, etc. |
gloomyandy
left a comment
gloomyandy
left a comment
There was a problem hiding this comment.
This will not work correctly in the case of a user specifying USB_SD_ONBOARD and LPC_SD_LCD. In that case the USB option is active and INIT_SDCARD_ON_BOOT will be true (as it is in effect just an ordinary SD card case), but we will need to make the onboard SD card available via USB.
|
So just to confirm I have now tested the current state of the PR and it does not work correctly with the default settings for an SKR V1.3 board (which is to use the LCD SD card for Marlin and to share the onboard SD card with host via USB). Inserting/removing the SD card from the LCD reader is not detected (instead the menu has the Init SD item which it should not have), also when you Init SD the shared onboard SD card is no longer available to a host system, again this is not correct as in this configuration we are not sharing the any SD card between Marlin and the host. Reverting the last commits to Conditionals_post.cpp and to main.cpp so that they now read... #if ALL(USB_SD_ONBOARD, SDSUPPORT, LPC_SD_ONBOARD)
#undef SD_DETECT_PIN
#define SHARED_SD_CARD
#endif
#if ENABLED(SDSUPPORT) && DISABLED(SHARED_SD_CARD)
#define INIT_SDCARD_ON_BOOT
#endifand #if DISABLED(USB_SD_DISABLED) && !BOTH(SHARED_SD_CARD, INIT_SDCARD_ON_BOOT)
MSC_SD_Init(0); // Enable USB SD card access
#endifrestores the correct operation. Do you want me to update the PR? |
Sure! And then I'll merge it to at least deal with the main issue. |
#if ALL(USB_SD_ONBOARD, SDSUPPORT, LPC_SD_ONBOARD)Again, note that |
@thinkyhead actually USB_SD_ONBOARD can be set without the others, in fact this is the default for some boards! Here is the code that does exactly that: #if !ANY(LPC_SD_LCD, LPC_SD_ONBOARD, LPC_SD_CUSTOM_CABLE)
#undef USB_SD_DISABLED
#define USB_SD_ONBOARD
#define LPC_SD_LCD
#endifThe default configuration (from Configuration_Adv.h is to have none of "LPC_SD_LCD, LPC_SD_ONBOARD, LPC_SD_CUSTOM_CABLE" defined. As you can see in this case LPC_SD_ONBOARD will not be set, but USB_SD_ONBOARD is set! Also some folks explicitly set USB_SD_ONBOARD but do not set SDSUPPORT, in this mode Marlin does not use an SD card, but the setting of USB_SD_ONBOARD means that the onboard SD card is still shared over USB and can be used to update the firmware. This is actually a useful mode it you don't use the SD card for Marlin, but want to be able to easily perform firmware updates. This option is not in the configuration files, but some users definitely use it (I've had conversations on Facebook and elsewhere about this setup). In both of these cases USB_SD_ONBOARD will be set but we will not be sharing the SD card between Marlin and a host. The only time we actually share access to the card is when all three are defined. Anyway I will update the PR... |
|
All the pins files that have these options do this: So there is an explicit linkage between
|
|
You are missing the first part of the if statement in the code that you quoted. Again from the SKR V1.3 pins file.... #if !ANY(LPC_SD_LCD, LPC_SD_ONBOARD, LPC_SD_CUSTOM_CABLE)
#undef USB_SD_DISABLED
#define USB_SD_ONBOARD
#define LPC_SD_LCD
#endif
#if ENABLED(LPC_SD_LCD)
#define SCK_PIN P0_15
#define MISO_PIN P0_17
#define MOSI_PIN P0_18
#define SS_PIN P0_16 // Chip select for SD card used by Marlin
#define ONBOARD_SD_CS P0_06 // Chip select for "System" SD card
#elif ENABLED(LPC_SD_ONBOARD)
#if ENABLED(USB_SD_ONBOARD)
// When sharing the SD card with a PC we want the menu options to
// mount/unmount the card and refresh it. So we disable card detect.
#define SHARED_SD_CARD
#undef SD_DETECT_PIN
//#define SD_DETECT_PIN P0_27 // (57) open-drain
#endifSo in the default case (the first if statement) we will have LPC_SD_LCD defined and USB_SD_ONBOARD defined but we will not have LPC_SD_ONBOARD defined. Note that in this case SHARED_SD_CARD is NOT set (as we take the first part of the following if statement), which is correct for this configuration. So in that case there is no linkage between USB_SD_ONBOARD and LPC_SD_ONBOARD. Basically SHARED_SD_CARD should not be set unless USB_SD_ONBOARD, LPC_SD_ONBOARD and SDSUPPORT are all defined. This is the only situation in which both Marlin and the host both have access to the SD card. In all other cases the card is not shared. |
Right. And I think that is an error because it then has no effect. |
|
I see that the LPC_SPI / SD options were added to the configuration files. While this then allows a user to set |
|
Why do you think that is an error? I don't see any problem with it at all. It is saying I want to use the SD card on the LCD for Marlin (LPC_SD_LCD), but I want to share the SD card on the board over USB (USB_SD_ONBOARD) this allows for conventional Marlin use of the LCD SD card, but provides the on board SD card to a PC for easy firmware updates. What is wrong with that?
in the following default case... #if !ANY(LPC_SD_LCD, LPC_SD_ONBOARD, LPC_SD_CUSTOM_CABLE)
#undef USB_SD_DISABLED
#define USB_SD_ONBOARD
#define LPC_SD_LCD
#endifIt explicitly sets both USB_SD_ONBOARD and LPC_SD_LCD which is a valid combination as described above. So I don't think there is an actual problem here as such. |
|
I would say though that this... #if ENABLED(LPC_SD_ONBOARD)
//#define USB_SD_ONBOARD // Provide the onboard SD card to the host as a USB mass storage device.
#endifIn the configuration files is probably confusing. It is certainly valid to set USB_SD_ONBOARD in other circumstances. Like the above example of using the LCD SD card reader. Perhaps the if ENABLED(LPC_SD_ONBOARD)Should be removed? |
|
First thing is first. I want to preserve the existing logic for these options before modifying any other logic or behavior. That is why I keep asserting these points.
Go back to the current code search for where Let us start from this point, where the existing logic (unchanged!) has simply been migrated over to the |
|
I'm not sure that all of the original logic was actually maintained when it was migrated from the pins file. At least some of the clarity was lost. I think to fully understand this you need to step back to how it was before then... The configuration scheme as used in the pins files came from @p3p and was based on my original PR to his Marlin branch. At the time it only applied to the MKS SBase and RAMPS_RE_ARM. Here is my understanding of how it works. For reference this is how the configuration options looked before they were moved to the config files... /**
* The SBase can share the on-board SD card with a PC via USB the following
* definitions control this feature:
*/
//#define USB_SD_DISABLED
#define USB_SD_ONBOARD // Provide the onboard SD card to the host as a USB mass storage device
/**
* There are a number of configurations available for the SBase SD card reader.
* - A custom cable can be used to allow access to the LCD based SD card.
* - A standard cable can be used for access to the LCD SD card (but no SD detect).
* - The onboard SD card can be used and optionally shared with a PC via USB.
*/
//#define LPC_SD_CUSTOM_CABLE // Use a custom cable to access the SD
//#define LPC_SD_LCD // Marlin uses the SD drive attached to the LCD
#define LPC_SD_ONBOARD // Marlin uses the SD drive attached to the control boardSee: Marlin/Marlin/src/pins/pins_MKS_SBASE.h Line 161 in 9b578ca First we have the defines that control making an SD card available over USB. USB_SD_DISABLED and USB_SD_ONBOARD. These two settings are mutually exclusive, only one from this set should ever be defined. All other settings are invalid. Note that in effect this means we have two bits of data defining only two states which is overkill but I think Chris probably had in mind that there is a possible further option USB_SD_LCD (make the LCD SD card reader available via USB) that is not currently available. but which would make sense to have. We then have the settings that define which SD card is used by Marlin, in this case we have three again mutually exclusive settings LPC_SD_CUSTOM_CABLE, LPC_SD_LCD and LPC_SD_ONBOARD. Note again only a single one of these should ever be defined other combinations are invalid. If in either case none of the available options has been selected then a default case should apply. As you can see above the default was explicit when the defines were in the pins file. I think this is currently true for the LPC_ settings, but I'm not sure that it is always being correctly set in the USB_ case. That is pretty much it. There is no limitation on how the above two sets can be combined. So there are six possible configurations, all of which are valid (though the custom cable options may only apply to some boards). From this you can see that LPC_SD_LCD and USB_SD_ONBOARD is a valid combination. Yes it is true that USB_SD_ONBOARD is not used directly in code with this setup, but the fact that USB_SD_DISABLED is not set is the important part. So having USB_SD_ONBOARD defined when LPC_SD_LCD is also defined is not strictly required, however I think it should be as it ...... When all of this logic was just in the pins file I think it was a little clearer. In the process of moving the configuration options I think the notion that the two sets have mutually exclusive values was lost (I think it was more obvious, though not explicitly stated or enforced when the settings were in the pins file). Also there was no direct linkage between the setting of LPC_SD_ONBAORD and USB_SD_ONBOARD. That #if was added when the settings were moved from the pins file into the configuration files. I think it should be removed. We also probably need to add some tests to ensure the mutual exclusive rules are followed. I think some of the confusion here is because we are using simple enabled/disabled defines to describe what is in effect a multiple value variable. You may find it useful to look at the original pr as submitted to @p3p when I did the initial work on this feature. This did not use the current system for configuration and was I think clearer in its intention. @p3p came up with the current way of doing things before he submitted a PR to Marlin (and I can understand why as it reduced the number of defines a little). You can see the original configuration options here: So that's my best effort at describing how this all works. @p3p may have other comments. EDITED: To fix typos and mistakes |
|
No idea why this failed in the Travis tests....
Some sort of problem with Travis? |
|
Thanks for going over those points. Is this now fully corrected and in line with your description? I'll see if the Travis test needs a kick. It often fails due to non-code errors, and this seems more common since we switched to a different Linux distro for the build. |
|
At the moment the actual code/logic is correct. But I have not made any changes to the configuration files or to the sanity checks (to enforce choosing one option only from each set etc.). I will have a go at doing that later if you want? |
|
The HALs are intended to abstract processors or if possible processor families. |
|
@GMagician sorry mixed folks up. But doesn't each shield need to have a pins file anyway? In which case is it that much trouble to have the onboard defines in there? There are two reason I don't like having this override in the HAL...
If this definition is common to a set of boards then can't the pins file have a base pins definition with other shield specific stuff in the shield pins files? Haven't we already had this situation with the RAMPS/ReArm combination? How many of these are we talking about? |
|
@AnHardt & @gloomyandy I understand that HAL should address processor specific parts but also I'm thinking at my case (and maybe this may be exctended). I would like to note that I have not approached SD implementation till now so maybe my info are wrong. |
|
@GMagician The problem is if you add this board specific code into your HAL then that means if there is another board with the same processor we will have to have another HAL to support it. Given that HALs are much bigger than the pins files I think it would be better to have additional pins files rather then additional HALs. But that is just my view. As to the hidden information. What I mean is that as a user and a developer I would expect to find how all of the pins are being used in the pins file. I don't really want the HAL to go and reassign things as I wouldn't be expecting that. |
|
I've tested pretty much everything I can, most things are fine. However there a few final issues....
|
|
I agree that |
Make sure the stuff in #if ENABLED(SDSUPPORT)
#if SD_CONNECTION_IS(ONBOARD) && ENABLED(USB_SD_ONBOARD)
#undef SD_DETECT_PIN
#define SHARED_SD_CARD
#endif
#if DISABLED(SHARED_SD_CARD)
#define INIT_SDCARD_ON_BOOT
#endif
#endifIt looks like this will undo the
The only design restriction I know about is that they cannot both be enabled at the same time. Which option is it that causes at least one of these two options to be required? Would it be for every LPC1768 board? If it is the case that at least one of these is required then they can be reduced to a single option. If it's enabled, you get one behavior. And if not, then you get the other. |
I'm not sure, but I believe that if we follow the thinking that if neither option is enabled in the configs, then the board default should apply… And, if we want I say, let's pick one and we can always switch it if we get complaints. |
|
@thinkyhead thanks for the feedback on this. I have now updated the PR to include the above points... By default the SD card will be shared by USB. I think most users will expect this to enable easy firmware updates. This feature is now controlled by a single define USB_SD_DISABLED which will disable the sharing if set. I don't really like using a negative configuration variable but in this case I think it makes sense. I have moved the setting of this define out of the test against SDSUPPORT and cleaned up the configuration and pins files.
Yes you are correct these settings are only intended for use when the SD card is not being shared over USB as well and the setting will be undone in the case of the card being shared. |
Recent changes to Marlin mean that when using a shared SD card Marlin will mount it at boot time. This fix changes the HAL startup code so that it no longer initially makes the SD card available over USB. Without this a host system will see the card, start reading it and then find it is no longer available (which causes errors). The card can be made availabe via USB to a host system by releasing the card either via the LCD or via gcode M22
Only set shared state when the card is actually being shared, allow use of LCD SD card for MArlin, while still sharing onboard SD card over USB. Base automatic setting of INIT_SDCARD_ON_BOOT on sharing state. Allow for INIT_SDCARD_ON_BOOT to be forced by user.
thinkyhead
force-pushed
the
pr_noinitusbsdcard
branch
from
77a096b to
1d02d36
Compare
thinkyhead
force-pushed
the
pr_noinitusbsdcard
branch
from
928364d to
a087f49
Compare
|
Will this cover STM32F boards like the BigTreeTech SKR Mini 1.1 and the million other STM32F boards they keep creating? |
|
So far this only pertains to LPC1768. We don't have SD card sharing control for other platforms at this time. |
thinkyhead
changed the title
|
I'm revisiting this PR in relation to #27815 where I'm working on completing the handling of multi-volume insert/remove. I want to make sure I'm handling MSC properly and not making a mess of it. So if anyone would like to help review that PR and provide insight to help us clear up glitches and put the final touches on this core part of the firmware, we'd appreciate your input! |
| // | ||
| #if ENABLED(SDSUPPORT) | ||
| #if SD_CONNECTION_IS(ONBOARD) && DISABLED(NO_SD_HOST_DRIVE) | ||
| #undef SD_DETECT_PIN |
There was a problem hiding this comment.
Specifically, for the benefit of multi-volume I hope to keep the SD_DETECT_PIN and show the "Refresh" item only in the file listings of the shared volume, preferably only when it is currently mounted on the PC. This pin is useful and we can hide/show menu items by better criteria.
I'm not sure where we stand on volumes being shared with PC while mounted by the firmware (and so on) so could use guidance on what is safe. I am seeing some slowness in sharing the USB Flash Drive over MSC and contention between Marlin and the PC when it is later inserted — but otherwise it all works great! Our current work is in conjunction with multi-volume, looking for the best approach to elegantly claim / release media and track media sharing status.
There was a problem hiding this comment.
The main thing to remember is that MSC is a block level protocol, you cannot have block level access to a memory device from 2 hosts as they can both change low level data, MTP was supposed to be a solution to this as it is a "file" level protocol and doesn't handle the filesystem, though it has other problems. so a block device can only be used by 1 host at a time, even if it has multiple partitions.
The fact that Windows (as of when I wrote the LPC usb stuff) ignores the MSC/SCSI commands to control access and report removal complicates it even more.
7 participants
Description
Recent changes to Marlin mean that when using a shared SD card Marlin will mount it at boot time. This fix changes the HAL startup code so that it no longer initially makes the SD card available over USB. Without this a host system will see the card, start reading it and then find it is no longer available (which causes errors). The card can be made availabe via USB to a host system by releasing the card either via the LCD or via gcode M22
Benefits
Stops hosts from getting errors while trying to mount the USB SD card.
Related Issues
#14318