Multiple, simultaneous, SPI buses handling and Smart SD mounting

[LinoBarreca]

Description

This is a work in progress. Do not merge yet.
This PR is meant to allow marlin to have a more modern approach to complex boards with several peripherals.

Work in progress? Why opening it now then?
Basically...It's a hell of work. I'm working on it since one month.
@boelle suggested me to not wait until this is feature complete but make "smaller PRs" so they are easier to review and merge. I am opening the pull request here to discuss it with the Marlin team to see what actually implement and what to delay to "future" and to let the contributors and HAL maintainers make the necessary changes to the other part of the software. Help from everyone is very appreciated.

Why was this even born?
To fix one simple problem and a open million of possibilities.
The problem: On board SD-Card on SKR-PRO 1.1 can't be used.
Why? Several reasons actually, all of them addressed with this huge PR.

1. Board design flaw: R5. The board has a wrong resistor in a place where it should not. What is R5, why is it wrong and why shouldn't be there? It's simple. BTT applied the basic principle that every digital line shouldn't be left floating....and that's correct. Problem is that R5 PULLS HIGH a clock line (which should be and IS managed by the controller) so when you deactivate the bus (stop talking to the card) there's half clock (and another half when you start talking to the card again) and
2. Wrong buses handling. The bus is configured in mode 0 (because it's the most common configuration) and other devices work in mode 0 and we couldn't handle several configurations (per bus). In theory each SPI device can have different settings about speed, clock polarity, clock phase, significant bits...and can be located on different buses. Since in old avr there was just one bus Marlin was born with that logic which still persists. Now we have a more modern approach to SPI devices: each one has its settings and its bus...which has its default parameters. This allows us to configure the bus with the wrong R5 to have a "resting" state high while leaving the other spi buses working at "resting state" low (technically this is called "CLOCK POLARITY").
3. Wrong bus clock frequency....Marlin is deeply tied to the old AVR world, some parts of it still think that the frequencies are those in the old AVR world...

How is this supposed to work?
Simple: instead of dealing with "pins" and levels for each device there's a more modern approach: device are accessed by id (somewhat your computer does..but simplified) The developers, when there's a new board, define how many spi buses the controller handles and on which pins (optionally they can give a default bus configuration), then they say which device is attached to which bus along with its type (sdcard, display, driver, eeprom, sensor) and properties and each device has its own identifier. When Marlin needs to access that device it just do a read/write to that device (by ID). the rest is done internally by the hal (setting bus/speed and so on). Another benefit is that looping on an array makes the code simpler and shorted (smaller program) and makes it ready for a variable, (big enough to be considered) infinite, number of axis, estrusors, sensors, and so on. Did someone think about CNC?

So you have a SKR-PRO. What will you gain with this PR:
This is still in an early stage with the other boards but for yours but it's already working and...

1. you can access the onboard sd card (it wasn't possible before)
2. you can use SPI3 port even if you are using drivers in SPI mode (it wasn't possible before)

What are the benefits for everyone else?
This is a structural change which creates support for simultaneous writes/reads to multiple devices. In the future you'll be able to use more than one card and more than one display working together.

Can you make an example? How does this work?
Sure. I used the SKR-PRO as base (which is the only board I modified, I need others' help to extend this logic to other boards)

• On-board SD is on bus ID 0 (hardware SPI1). There's a detect switch: card in? Marlin sees it. Card out? Marlin doesn't.
• Screen SD is on bus ID 1 (hardware SPI2). There's a detect switch: card in? Marlin sees it. Card out? Marlin doesn't. If the opposite happens flip the SD_DETECTED_INVERTED, in configuration.h
• External SD can be attached to bus ID 2 (hardware SPI3). There isn't a detect switch in that port: if Marlin searches the card in there it will always "find it" and try to initialize it. If you are sure you don't use it, remove the probing of SD in bus 2. How? #define SD_SEARCH_ORDER { 1, 0 } in configuration.h

I have a SKR-PRO. So it should be working. What will it happen when I flash it?
It depends on your current situation.

• If you have a display with a SD card slot and that slot contains a card you will not see any change. You will be able to access your files. Like you did without this PR. If you pull out the SD card from the display (unmount it from menu, before) and ask to "mount card" again you should see (magically) the files in the on-board SD.
• If your display doesn't have a card slot or the slot is empty you will see the files in your on board sd card.... and you will be able to save/restore EEPROM to/from it

Benefits

This fixes a few bugs on SKR-Pro regarding the not working sdcard and "missing eeprom".
If your display didn't have the sd-card slot you couldn't use the onboard sd neither to print nor for saving the eeprom contents (on SKR-Pro there isn't a real EEPROM).

Things already done since the PR was opened:

• Handle speed change by device (to be defined in pins file along with device properties) When this will be complete the definition of the device parameters (speed/msb/polarity etc) will be "per device" (you shouldn't care anyway...it will be internal in marlin).
• Unnecessary bus reconfig when talking to the same device.
• Actually SD-card calls are mixed. New ones are "per device" old ones are still "per bus". We gotta modify them.

Things which are known to be done before this can be merged. In order of priority:

• Other boards. This required a modification on the HAL interface with the core. This means that, to make this working, other hals must accept the calls and route them properly to their inner methods.
• Messages in Chittagonian. English only at the moment, sorry. We need to extract new strings into resource files and localize them.
• User stupidity checks. Actually if you fuck up some configuration on the new features you get no warning from SanityCheck. We will add them later... anyway if you are here you are an expert which reads the instructions and knows what he's doing...by the way DO NOT EDIT PINS file unless you are told so. 😆 There are other ways to do what you need in configuration/configuration_adv.h

Future development and possibilities that this PR opens:

1. Multiple axis and drivers per axis. We should separate the Hardware-layer from the "usage" logic. Actually, when marlin wants to move X axis, now it thinks about "CS pin". This can work only if the drivers are on the same SPI bus. Proper logic should be:

• define buses (this part is done)
• define devices attached to buses (storage, driver, sensor) (this part is done)
• map devices to usage: for example axis X is controlled by driver 0, axis Y is driver 1, axis Z is driver 2, extruder 0 is driver 3, extruder 1 is driver 4, axis Z2 is driver 5. This part is not implemented the proper way: actually the binding driver/axis/index is done in pins file in the SPI_Devices section. While it could work for "default" configuration X,Y,Z,E0,E1,E2 the user might want to use a driver to drive an axis i.e. X2=E2 and we shouldn't ask the user to edit pins file. So we should rather provide a way to map a function (i.e. axis X) to a driver (i.e. ID 5) something like this:

#define NumAxis 3 //my printer has 3 axis (optional, we could leave it dynamic)

const int AxisSteppers[NumAxis][] = {{1,2}, {4}, {5,7}} //axis 0 uses steppers 1 and 2, axis 1 uses stepper 4, axis 2 uses stepper 5 and 7. what does it mean? dual x, single y, dual z
const int ExtrudersSteppers[] = { 3, 6, 8} //three extruders, use steppers 3, 6 and 8

As you can easily imagine this would require a biiiig marlin logic rewrite that would allow marlin to run on any kind of machine (from printer to CNCs) and implement in the future a real non planar 3d printing (for example adding another axis which turns clockwise, anti-clockwise the extruder).
At the moment, however, since accessing axis and extruders by index needs a lot of work w need to find a way to make it work with the current logic (besides asking people to edit pins file..which is still possible, and requires no development at all)

2. Make Hardware CRC on SPI bus working for all the STM models which support it. Actually it's only working on F4 (This is a new feature: The F4 has CRC in hardware. with this PR we go and use that hardware to calculate crc on data. This makes the program smaller and talking with the card faster).. by the way...did you know that the on-board sd card on SKR-Pro is blazing fast? (double the speed of the LCD card) Since it's out of SD card specifications I actually lowered the speed to the one in the specs(look at the pins file, SPI_HALF_SPEED )...but we tested it very well and most modern cards work at full speed (42 Mhz) We might just ignore the compatibility with 10yo cards and merge this to work at the highest speed possible.
3. Multiple cards at once. Actually Marlin can only handle one card at once. Since we can now have N working all together, we need to modify some commands to accept a "card number". That's maybe why
4. ESP8266 can't upload files to the LCD card when an on-board one is already present and working.
5. SPI drivers when TMC_USE_SW_SPI is undefined (that's because tmcstepper library might need an update to support more than one bus, like we are doing here) if we address them by pins through marlin, however with "TMC_USE_SW_SPI", everything works fine. so, at the moment #define TMC_USE_SW_SPI. I gotta look at it later.
6. DMA transfers for STM32. They were never implemented....but since we are working on it...Not that we need them, since the STM32 MCUs are actually faster than needed by marlin....but it's an improvement and we might want to do it in the future.

Fixes:
#14684

[epsilon-0311]

@Keltere there were some major changes in Marlin/src/sd/Sd2Card.cpp
At least thats shown by the diff viewer in the PR.

I started in Marlin/src/sd/cardreader.cpp at void CardReader::mount() function. From there Sd2Card::init is called to initalize the SD-Card. Inside the init procedure the SD-Card shall be set to idle via cardCommand(CMD0, 0), but this does not return R1_IDLE_STATE. I already tried an increased timeout.

So I currently don't know if the issue is inside the cardCommand call or if the issue is already 5 lines prior inside the following loop:

for (uint8_t i = 0; i < 10; i++) spiBusSend(BUS_OF_DEV(dev_num), 0xFF); //Send to bus, not to device (to not alter CS)

[Keltere]

So I currently don't know if the issue is inside the cardCommand call or if the issue is already 5 lines prior inside the following loop:

for (uint8_t i = 0; i < 10; i++) spiBusSend(BUS_OF_DEV(dev_num), 0xFF); //Send to bus, not to device (to not alter CS)

Thanks, with some debug message, ill try to understand what is going on in that cycle.
Btw first time i saw a FOR without any parameters but with a GOTO inside.
Isn't that considered bad coding practice?

[epsilon-0311]

Yeah, goto's should be avoided, they make the code hard to read and to follow. (It also increases the cyclomatic complexity when doing static code analysis. )

The for is not that bad, but if I want an endless loop I typically use a while or a do-while.

[Keltere]

@LehrChristoph you where right, it always go on timeout and if i try to add more time in any way the system will always crash after returning the error.

[jeno007]

@LinoBarreca I'm just considering quick fixes for this problem focusing only on SKR Pro v1.1.
Based on your investigation, and accepting that current Marlin uses only one SPI bus, getting the onboard SD card to work would require only one change, to initialize the SPI bus in mode2 instead of mode0. Besides setting the PINs properly of course.
Am I right?

Other solution would be to mechanically remove the famous R5, but that might make the closed source bootloader unoperable, thus making the simple firmware update impossible.

[spattinson]

I used multi spi branch about a month ago my experience was the onboard reader worked fine though i only used the 125mb card that was supplied with the board plus one other. The card that came with board has died now and i am running bugfix branch. The problems i had with multi spi branch was with external reader on my lcd display out of about 7 cards at hand only one worked and they are all branded cards. Maybe long ribbon cables may be an issue, they all work on bugfix branch though. I will try the latest multi spi again and let you know if anything has changed.

[thinkyhead]

Sorry this got stale while making (many) other adjustments to the HALs and dealing with the usual blizzard of not really bug reports….

Is this still a viable concept given our ongoing reforms of the HAL? And… What about getting the HALs into a formal shape with a namespace or a HAL class? Is it too soon to make that leap…?

[GMagician]

@thinkyhead times ago I tried to add this to samd51 hal but I found some problem. Samd51 can handle spi only on some specific pins and has spi classes that don't want any pin definition. I was wondering if a different approach was possible, but I think it was not considered. My idea was to use only some spi class to fill spiBus and these may be some derived class with pins for frameworks that let this to be done...

see #16260 (comment)

[Bob-the-Kuhn]

I was concerned about how this approach affected daisy chained SPI devices. Below is my though process on how I concluded that the needed changes would be fairly easy.

---

If I'm reading the code correctly the top level application view is:

• creates/acquires/uses a SPI device
• passes a buffer to the SPI device
• The HAL sets CS active
• The HAL transmits/receives the buffer
• The HAL sets CS inactive

The hard vs. soft SPI implementation is hidden from the application.

I have mixed feelings on always having the HAL control the CS.

I guess my main concern are the daisy chained SPI devices. Currently the software passes to the lowest application level a command buffer and a stepper number. The lowest level sets CS active, writes to the SPI (adding null commands as needed) and then sets CS inactive.

To use this PR's approach, the lowest application level would need to be changed to write to a buffer rather than the SPI and then pass the buffer. As long as the buffer contains one complete transaction then having the HAL control the CS is acceptable.

[Bob-the-Kuhn]

I did some serious work on a similar approach a couple of years ago.

The Marlin side is a lot of work but certainly manageable.

I think there are some major implementation problems:

• I don't see a way to break this up into a series of smaller PRs. As far as I can tell this is an architectural change and we must change everything at the same time.
• I'm expecting that it'll take a long time to write & test the Marlin changes. Marlin changes so fast that rebasing this PR's changes will be a real pain.
• The biggest problem may be converting all the external libraries for the SPI based devices. I've done this for a couple of devices in the past and it is a months long process if they are willing to work with you. We're probably better off just planning on creating Marlin specific versions of the libraries.

The multiple PR approach may be applicable to the external library portion. Maybe we create the libraries for the big items (U8G & TMC) first and the others as people complain.

---

EDIT

Marlin already has it's own U8G library

[Bob-the-Kuhn]

I've been looking through the code and have some questions.

What were you planning for the devices that require a soft SPI?

What are the various SPI device types intended for and how do they differ from each other?

I believe SPI_PHI, SPI_PLO, SPI_LTS, SPI_STL are used to select SPI_MODE_0 ... SPI_MODE_3 when setting up the hardware SPI controllers. If this is the case then I think the pins_YOUR_BOARD.h files should only have SPI_MODE_0 ... SPI_MODE_3 and the SPI_PHI, SPI_PLO, SPI_LTS, SPI_STL detail be hidden from the user in the HAL.

[thinkyhead]

I have merged the latest code into this PR, so please check to make sure everything is where it's supposed to be.

[thinkyhead]

I have merged the latest code into this PR, so please check to make sure everything is where it's supposed to be.