felis / USB_Host_Shield_2.0

Revision 2.0 of USB Host Library for Arduino.
https://chome.nerpa.tech
1.8k stars 779 forks source link

How do you change the SPI assigned pins? #822

Closed DrJaymz closed 3 months ago

DrJaymz commented 3 months ago

What is the recommended way to change the GPIO pins for SPI where the device lets you use any pin? On the ESP32 you can map HSPI and VSPI to many suitable pins but and on various boards the standard pins are not available anyway.

How do you change the pins? Usually you just define them in the SPI constructor but that was burried 7 layers deep, then I played hide and seek with some weird MAKE_PIN stuff that isn't documented and I can't really see what it is doing. I had to open no less than 5 source files just to get that far. I don't really know why it has to be so many levels of abstraction when none or one would do.

My best guess was that its in avrpins.h but the way its written means you need to make changes is 5 parallel dimensions and doesn't work anyway so I'm doing it wrong.

herraa1 commented 3 months ago

I can't tell if this is the recommended way but AFAIK, you can indicate the library which pins you use for SPI by just defining all PIN_SPI_SCK, PIN_SPI_MOSI, PIN_SPI_MISO and PIN_SPI_SS before including any library header.

I think the pins have to be defined anyway in avrpins.h (on the section for your specific platform).

You can look at usbcore.h to see why this works.

For example:

define PIN_SPI_SCK 18 // SCK

define PIN_SPI_MOSI 21 // SDA

define PIN_SPI_MISO 22 // SCL

define PIN_SPI_SS 5 // SS

include

DrJaymz commented 3 months ago

I can't tell if this is the recommended way but AFAIK, you can indicate the library which pins you use for SPI by just defining all PIN_SPI_SCK, PIN_SPI_MOSI, PIN_SPI_MISO and PIN_SPI_SS before including any library header.

I think the pins have to be defined anyway in avrpins.h (on the section for your specific platform).

You can look at usbcore.h to see why this works.

For example:

define PIN_SPI_SCK 18 // SCK #define PIN_SPI_MOSI 21 // SDA #define PIN_SPI_MISO 22 // SCL #define PIN_SPI_SS 5 // SS

include

Then it doesn't compile. Unless it already knows about the pins you want. So I probably need to make changes in 3 places, one under esp32 def, another where it passes to the template and another in my code. Thats what I mean about layers of complexity.

define PIN_SPI_SCK 33 // SCK

define PIN_SPI_MOSI 5 // SDA

define PIN_SPI_MISO 36 // SCL

define PIN_SPI_SS 13 // SS

include "usbhost.h"


.pio/libdeps/esp32-poe-iso/USB-Host-Shield-20/usbhost.h:103:25: error: 'P33' was not declared in this scope

define APPEND_PIN(pin) P ## pin // Appends the pin to 'P', e.g. 1 becomes P1

                     ^

.pio/libdeps/esp32-poe-iso/USB-Host-Shield-20/usbhost.h:100:23: note: in expansion of macro 'APPEND_PIN'

define PASTE(x, ...) x ## __VA_ARGS__

                   ^

.pio/libdeps/esp32-poe-iso/USB-Host-Shield-20/usbhost.h:101:34: note: in expansion of macro 'PASTE'

define EVALUATING_PASTE(x, ...) PASTE(x, __VA_ARGS__)

                              ^~~~~

I'll see if I can't figure that out and get back....

DrJaymz commented 3 months ago

The reason it doesn't work is because theres a line in usbhost.h:

/ SPI pin definitions. see avrpins.h /

if defined(PIN_SPI_SCK) && defined(PIN_SPI_MOSI) && defined(PIN_SPI_MISO) && defined(PIN_SPI_SS)

which if all of them are defined should define like this: typedef SPi< MAKE_PIN(PIN_SPI_SCK), MAKE_PIN(PIN_SPI_MOSI), MAKE_PIN(PIN_SPI_MISO), MAKE_PIN(PIN_SPI_SS) > spi;

But that doesn't work or contains a bug, because instead it evaluates

elif defined(ESP32)

I tried commenting that out because ESP32 is also true but then you just get No Spi entry in subhost.h.

So I bodged my pins in there.

elif defined(ESP32)

typedef SPi< MAKE_PIN(PIN_SPI_SCK), MAKE_PIN(PIN_SPI_MOSI), MAKE_PIN(PIN_SPI_MISO), MAKE_PIN(PIN_SPI_SS) > spi;

But that doesn't work because it doesn't know what MAKE_PIN is at that point.

It would have been a whole lot easier just to change the line where it calls spi.init({with the spi pins})

DrJaymz commented 3 months ago

I did what I threatened to do.

in usbhost.h: USB_SPI.begin(33,36,5,32); <-- changed to the pins I wanted.

On the scope I can see MOSI and CLK traffic but no MISO. then I noted that the SS is not being set. So around Usb.init() and Usb.task() I set the pin LOW and then MISO now talks too, so the chip is talking back.

Then the init works, and usb.task returns USB_DETACHED_SUBSTATE_ILLEGAL Which I think is because the library has a constant conversation with the SPI device which will only work when SS is low so I'll need to check that.

I'm so frustrated because it really should just have been as simple as setting

define PIN_SPI_SCK 33 // SCK

define PIN_SPI_MOSI 5 // SDA

define PIN_SPI_MISO 36 // SCL

define PIN_SPI_SS 13 // SS

I don't know why that doesn't work. Its a bit like the library is loaded before it sees the #define. But I have and then the defines and then the library so I can't really make it any higher?

herraa1 commented 3 months ago
#define PIN_SPI_SCK 33 // SCK
#define PIN_SPI_MOSI 5 // SDA
#define PIN_SPI_MISO 36 // SCL
#define PIN_SPI_SS 13 // SS

#include "usbhost.h"
.pio/libdeps/esp32-poe-iso/USB-Host-Shield-20/usbhost.h:103:25: error: 'P33' was not declared in this scope
#define APPEND_PIN(pin) P ## pin // Appends the pin to 'P', e.g. 1 becomes P1
^
.pio/libdeps/esp32-poe-iso/USB-Host-Shield-20/usbhost.h:100:23: note: in expansion of macro 'APPEND_PIN'
#define PASTE(x, ...) x ## VA_ARGS
^
.pio/libdeps/esp32-poe-iso/USB-Host-Shield-20/usbhost.h:101:34: note: in expansion of macro 'PASTE'
#define EVALUATING_PASTE(x, ...) PASTE(x, VA_ARGS)
^~~~~

Your code does not compile because you are using pins 33, 36 and 13 which are not defined in avrpins.h for ESP32 (pin 5 is already defined). Edit your ~/Arduino/libraries/USB_Host_Shield_Library_2.0/avrpins.h and add them to the ESP32 section, like this patch does (I'm assuming your board falls under the ESP32 code):

--- avrpins.h.orig  2024-07-25 12:41:15.172867052 +0200
+++ avrpins.h   2024-07-25 12:43:56.596628826 +0200
@@ -1939,6 +1939,9 @@ MAKE_PIN(P23, 23); // MOSI
 MAKE_PIN(P18, 18); // SCK
 MAKE_PIN(P5, 5); // SS
 MAKE_PIN(P17, 17); // INT
+MAKE_PIN(P33, 33);
+MAKE_PIN(P36, 36);
+MAKE_PIN(P13, 13);

 #endif

Also, do not use "usbhost.h" directly on your sketch, as the file itself indicates.

Never include usbhost.h directly; include Usb.h instead

If you want to test if defining the SPI pins as I told you works for your ESP32 board, then do the already mentioned modification in avrpins.h and then grab the example sketch board_qc from the Examples (go to your Arduino IDE, File | Examples | USB Host Shield Library 2.0 | board_qc), then add the following lines to the very top of the file (before any includes) and compile it and test it.

#define PIN_SPI_SCK 33
#define PIN_SPI_MOSI 5
#define PIN_SPI_MISO 36
#define PIN_SPI_SS 13

I'm not saying this is straightforward or easy, just trying to comment how I would do it with the fewer changes to the library.

DrJaymz commented 3 months ago

Thanks I added these pins a while ago. And I'm still stuck on the fact that it doesn't respect the pin assignments. I have added debug to the constructors / init by adding a GetPin method to MAKE_PIN so I can see what is going on.

The init function for the MAX3421 is where the problem lies. Which means I must need to also change it somewhere else. The structure makes this incredibly hard to find unless you already knew where it was.

int8_t MAX3421e< SPI_SS, INTR >::Init() SPI_SS: 5 <----- where does this come from? It should be 32 - its picking up a default from somewhere. SPI_INR: 17

These are correct. template< typename SPI_CLK, typename SPI_MOSI, typename SPI_MISO, typename SPI_SS > class SPi S_SPI_CLK:33 S_SPI_MOSI:5 S_SPI_MISO:36 S_SPI_SS:32 SPI_BEGIN_CLK: 33 SPI_BEGIN_MOSI: 5 SPI_BEGIN_MISO: 36 SPI_BEGIN_SS: 32 USB initialised {0} USB_DETACHED_SUBSTATE_ILLEGAL

<1 hour later.....> Ah ha..... #elif defined(ESP32) typedef MAX3421e MAX3421E; // ESP32 boards <-- here it is in usbcore.h I found that only by breaking the code on purpose so I could see what tried to use it. Thats why I hate this way of doing it in 30 lines on 5 files where a single line would have worked. And Even if the line #if defined(PIN_SPI_SCK) && defined(PIN_SPI_MOSI) && defined(PIN_SPI_MISO) && defined(PIN_SPI_SS) had worked... it still wouldn't have worked because its effectively hard coded. All you needed to do was have the pins exposed on Usb.Init(). As it is, on a trip from a village to the next village I had to drive into London rush hour twice and reboot my children. And suddenly .... It works! Listen, thanks for your help, I didn't enjoy this AT ALL. But I had given up on it last night until you replied, so thanks for the encouragement.
herraa1 commented 3 months ago

Oh, yes. So the problem is you used P5 for MOSI but P5 is hardcoded as SS in MAX3421E constructor for ESP32, thus you end up with a non-working config.

I'm sure developers here would accept patches if you contribute a cleaner solution to that hardcoding spree.

As in Usb.h the order of the includes is first usbhost.h and then UsbCore.h, maybe all that is needed is make sure that PIN_SPI_SS ends up always defined (either manually before including any files or in usbhost.h for each platform when not defined manually) and use that definition in UsbCore.h for MAX3421E constructor.

So usbhost.h would look like (if you use ESP32 and do not predefine PINSPI{SS,CLK,MOSI,MISO}):

#elif defined(ESP32)
#define PIN_SPI_CLK P18
#define PIN_SPI_MOSI P23
#define PIN_SPI_MISO P19
#define PIN_SPI_SS P5

and then a common definition for all platforms like:

typedef SPi< PIN_SPI_CLK, PIN_SPI_MOSI, PIN_SPI_MISO, PIN_SPI_SS > spi;

and UsbCore.h like this:

elif defined(ESP32)
#define PIN_MAX3421E_INT P17

and then a common definition like

typedef MAX3421e<PIN_SPI_SS, PIN_MAX3421E_INT> MAX3421E;

Just thinking out loudly...

DrJaymz commented 3 months ago

Oh, yes. So the problem is you used P5 for MOSI but P5 is hardcoded as SS in MAX3421E constructor for ESP32, thus you end up with a non-working config.

I'm sure developers here would accept patches if you contribute a cleaner solution to that hardcoding spree.

As in Usb.h the order of the includes is first usbhost.h and then UsbCore.h, maybe all that is needed is make sure that PIN_SPI_SS ends up always defined (either manually before including any files or in usbhost.h for each platform when not defined manually) and use that definition in UsbCore.h for MAX3421E constructor.

So usbhost.h would look like (if you use ESP32 and do not predefine PINSPI{SS,CLK,MOSI,MISO}):

#elif defined(ESP32)
#define PIN_SPI_CLK P18
#define PIN_SPI_MOSI P23
#define PIN_SPI_MISO P19
#define PIN_SPI_SS P5

and then a common definition for all platforms like:

typedef SPi< PIN_SPI_CLK, PIN_SPI_MOSI, PIN_SPI_MISO, PIN_SPI_SS > spi;

and UsbCore.h like this:

elif defined(ESP32)
#define PIN_MAX3421E_INT P17

and then a common definition like

typedef MAX3421e<PIN_SPI_SS, PIN_MAX3421E_INT> MAX3421E;

Just thinking out loudly...

I agree, but that would have to be done by someone who knows what they are doing and have used c templates recently, for me it's been 20 years. I'm using platformio with vscode and it could not navigate the code which made life difficult.
The issue for me was that what I wanted to do sounded incredibly simple but it was hard to navigate an unfamiliar codebase from a standing start. But I appreciate its the way it is because of all the classes and platforms you can use. However, if one is going to do that it should still have a simple implementation interface and not require hardcoding in multiple places in the library itself - and there is no reason why it can't.
I see a couple of reported issues, basically asking for the same. I think you're absolutely right that if you could just define the pins and then call it and it actually worked, that would be ideal.

The important thing is that we got there in the end and the library is working fine.