-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
Prefix SD SPI pins (SCK, MISO, MOSI, SS) #20606
Prefix SD SPI pins (SCK, MISO, MOSI, SS) #20606
Conversation
For much of the long history of Marlin the SPI bus for your LCD and your SD Card were shared, so I assume they shared the same pins and speed settings. Are they well separated now, so that we can be sure these SPI settings only pertain to SD? Are these SPI settings pertinent at all when |
Yes and no. Some (minor, in fact) code reconfigure the spi before use. The sd code always reconfigure the spi for the sd settings before the use.
Yes, but not yet. The idea is that each peripheral define your own spi pins and settings. All settings separated from each other. But to avoid breaking everything, I'm trying to do these changes in "isolated" tasks. For example: this renaming will give us a better understanding for what is relying on SD spi settings to work.
Yes and no again hahaha. Currently, some boards define another spi port on pins when using sdio, they normally set the spi of the exp ports, so the spi can be used with a lcd. Even if lcd or tft or whatever share the same spi pins with SD, having this prefix will be necessary for board that don't share. And make everything more clear. I changed the SD_SPI_SPEED to advanced config too, because it should not be something "common" to change. As board already does with SDIO, soon they will provide their "supported" SD spi speed and sd spi data mode. |
I didn't changed the lcd or the others places using SD spi pins/speed because this is a planned task: I'm reviewing all SPI usages. I don't know if worth start part of this task now, as the issue is not only speed, but data mode, port, etc... |
Marlin/src/HAL/LPC1768/HAL_SPI.cpp
Outdated
@@ -55,27 +55,55 @@ | |||
#include <lpc17xx_pinsel.h> | |||
#include <lpc17xx_clkpwr.h> | |||
|
|||
#include "../shared/HAL_SPI.h" | |||
|
|||
#ifndef HAL_MISO_PIN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The objective is exactly the opposite... remove “generic” and “un-prefixed” spi configs... not add one more...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things inside a .cpp file are distinct from the rest of the codebase, and this fits what is going on here, while also following a convention.
In general, calling the first exposed SPI port on all boards the "SD SPI Port" is slightly problematic, because it doesn't really apply as an "SD port" unless SDSUPPORT
is enabled. I expect the vast majority of users will be using this SPI for SD connection primarily, but I'm hesitant to label it as "the SD port."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't calling the first spi port as SD spi... we are making the board set the right names for what they mean to.
There's no such a thing as "THE" SPI. This is only valid for old avr that we have only one single spi...
Each device should define its own spi config and pins, and the SD too.
This un-prefixed pins is one of the reasons that all over the code it's wrongly used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thinkyhead Re-reading your comments, I think here began the misunderstanding of this PR. You said this:
In general, calling the first exposed SPI port on all boards the "SD SPI Port" is slightly problematic, because it doesn't really apply as an "SD port" unless SDSUPPORT is enabled.
But this is not true. The boards only set SPI pins for specific usages. If you look carefully at the board pins, lots of board doesn't even define SCK/MOSI/MISO when SD is not SPI (lcd or onboard).
If some DOGM (or another) are sharing SPI configs with SD, this is an error that we are starting to fix!!
Random boards that doesn't declare MISO/MOSI/SCK pins when the SD is not SPI: pins_BTT_GTR_V1_0.h, pins_MKS_ROBIN_NANO_V2.h (doesn't declare it at all, because the SD is SDIO), pins_BTT_SKR_common.h (all SKR)
...
It's more than clear the (MOSI/MISO/SCK/SS)_PIN
are 100% related with SD.
We have now:
TOUCH:
SPI_FLASH:
All following the same standard Why SD would be different?? Why it is called only |
The handling of SPI pins for LCD devices has evolved, but there is still the need for a 'generic' first SPI port. It seems to me it would be preferable to add this to #if ENABLED(SDSUPPORT)
#define SD_MISO_PIN MISO_PIN
#define SD_MOSI_PIN MOSI_PIN
#define SD_CLK_PIN CLK_PIN
#define SD_SS_PIN SS_PIN
#endif … and then only use the SD-specific pin names when the SD device connected to this bus is being specifically referenced in the code. |
So it’s better the other way around. Board define the devices spi. The board don’t define a generic spi. It’s only exist on avr. Board define how devices uses some of their spi. Lcd spi is the spi of the exp port. Sd is the spi for onboard sd, or the same as the lcd spi if using a sd on lcd. The marlin code must not use a generic spi because it don’t make sense. All the spi usages is an device using it, with its own configs. That “global” spi must be removed in sometime. It’s prone and lead to error. |
Worst yet: if a common 32 bit board would define a “global/generic/default” SPI, it would be the SPI on the exp slot, because it is intended to expansion, not the hard wired spi on the SD slot... |
One more case: when supporting multiple medias at same time, one may want to use an onboard SD + a LCD SD. So, we will need the definitions for two SD SPI, not only one, at same time. #if HAS_ONBOARD_SD
#define SD_ONBOARD_MISO_PIN 1
#define SD_ONBOARD_MOSI_PIN 2
#define SD_ONBOARD_CLK_PIN 3
#define SD_ONBOARD_SS_PIN 4
#endif
#if HAS_LCD_SD
#define SD_LCD_MISO_PIN 5
#define SD_LCD_MOSI_PIN 6
#define SD_LCD_CLK_PIN 7
#define SD_LCD_SS_PIN 8
#endif |
* [cron] Bump distribution date (2020-12-31) * SPI and pins cleanup * [cron] Bump distribution date (2021-01-01) * Prefix SD SPI pins (SCK, MISO, MOSI, SS) (MarlinFirmware#20606) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com> * Fix PARKING_EXTRUDER homing with solenoid (MarlinFirmware#20473) * Fix CHAMBER_FAN_MODE 0 build (MarlinFirmware#20621) * [cron] Bump distribution date (2021-01-02) * Fix UBL mesh edit delta moves (MarlinFirmware#20620) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com> * Fix //action prefix (MarlinFirmware#20600) * Assisted Tramming improvements (MarlinFirmware#20298) * Check for misplaced configs on build (MarlinFirmware#20599) Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com> * Fix a comment (MarlinFirmware#20629) * Document, adjust some homing code * Improved bootscreen animation * [cron] Bump distribution date (2021-01-03) * Homing code followup (MarlinFirmware#20632) Patching a87e519 * Animated boot followup * Add ALL_AXES manual move for UBL mesh editing Co-Authored-By: Jason Smith <20053467+sjasonsmith@users.noreply.github.com> MarlinFirmware#20620 * Creality 4.2.10 board (MarlinFirmware#20647) * Update Italian language (MarlinFirmware#20663) * [cron] Bump distribution date (2021-01-04) * Fix thermal error protection, reporting (MarlinFirmware#20655) * Rename FTDI EVE screen data structs * Fix SD SPI Speed override, FTDI mesh edit (MarlinFirmware#20657) Co-authored-by: Scott Lahteine <github@thinkyhead.com> * Fix IDEX reboot on travel after G28 X (MarlinFirmware#20654) * Fix delayed_move_time elapsed test * Move duplication_e_mask Co-authored-by: thinkyhead <thinkyhead@users.noreply.github.com> Co-authored-by: Scott Lahteine <github@thinkyhead.com> Co-authored-by: Victor Oliveira <rhapsodyv@gmail.com> Co-authored-by: zeleps <39417467+zeleps@users.noreply.github.com> Co-authored-by: Jason Smith <jason.inet@gmail.com> Co-authored-by: ellensp <ellensp@hotmail.com> Co-authored-by: qwewer0 <57561110+qwewer0@users.noreply.github.com> Co-authored-by: Giuliano Zaro <3684609+GMagician@users.noreply.github.com> Co-authored-by: Marcio T <mlt4356-github@yahoo.com> Co-authored-by: InsanityAutomation <38436470+InsanityAutomation@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Description
Marlin born with AVR, that have only one HW SPI. So, the name chose for the SD SPI pins have no prefix. It lead to some confusion and don't make sense on boards with multiple SPI, and when we support dozens of different SPI devices.
As others SPI pins (TFT, touch, etc), this PR add prefix the the SD SPI pins. This is part of the on going effort to modernise and fix marlin spi code.
Benefits
More clear usage of the SD SPI pins.
Make clear where SD SPI pins are being used with another devices....
Related Issues