Skip to content
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

Add SPIClass::setup method for ESP32 #2360

Merged
merged 1 commit into from Aug 26, 2021

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Aug 24, 2021

Allows default bus/pin set to be changed for global SPI instance.

@slaff slaff added this to the 4.4.0 milestone Aug 25, 2021
@slaff
Copy link
Contributor

slaff commented Aug 25, 2021

@mikee47 Can you check the CI errors ?

/home/appveyor/projects/sming-sb483/Sming/Arch/Esp32/Core/SPI.cpp:251:8: error: 'class SPIClass' has no member named 'id'
  this->id = busId;
        ^~
make[2]: *** [/home/appveyor/projects/sming-sb483/Sming/component-wrapper.mk:169: Arch/Esp32/Core/SPI.o] Error 1

@slaff slaff removed the 3 - Review label Aug 26, 2021
@slaff slaff merged commit 858c368 into SmingHub:develop Aug 26, 2021
@kmihaylov
Copy link
Contributor

kmihaylov commented Aug 26, 2021

I might be doing something wrong, but creating an instance of SPIClass SPIClass masterSpi(SpiBus::VSPI); and trying to setup it later with masterSpi.setup(SpiBus::HSPI, spispi); results in non-working SPI.

If I create it with SPIClass masterSpi(SpiBus::HSPI, spispi); then it works.

The only change resulting in this behavior is the instance creation w/ VSPI or HSPI. in both cases I call the setup method.

===========

Edit 1: Please wait to verify which SPI peripheral is in use. I changed HSPI to VSPI (in SPI.setup()) and now it works.

===========

Edit 2: OK, I don't remember why I choose these pins, probably I was looking in the datasheet for HSPI pins, however there's no HSPI_MISO or HSPI_MOSI, so... Idk.

But the use of the HSPI/VSPI perhipheral bugs me. Is it possible, that I used HSPI in my app's instance together with the VSPI in the global instance, therefore the app's SPI object really used HSPI (because VSPI should have been already taken by the global SPI instance)?

Then if I leave the app only with the global SPI instance, it shouldn't be a problem to use HSPI or VSPI (and the specified pins). I don't get it.

@mikee47 mikee47 deleted the feature/esp32-spi-setup branch August 26, 2021 09:33
@mikee47
Copy link
Contributor Author

mikee47 commented Aug 26, 2021

But the use of the HSPI/VSPI perhipheral bugs me. Is it possible, that I used HSPI in my app's instance together with the VSPI in the global instance, therefore the app's SPI object really used HSPI (because VSPI should have been already taken by the global SPI instance)?

Then if I leave the app only with the global SPI instance, it shouldn't be a problem to use HSPI or VSPI (and the specified pins). I don't get it.

Bear in mind that SPI resources are not allocated until begin() is called. If you provide minimal code examples of working/non-working I'll try to shed more light on what's happening!

@mikee47
Copy link
Contributor Author

mikee47 commented Aug 26, 2021

See also comments at top of #2130 regarding HSPI.

@kmihaylov
Copy link
Contributor

Thanks, I'll prepare a sample tonight or tomorrow. Otherwise my current setup uses 3 chips, CS5460, MAX31865 and ADuM1401. They share the same bus and they do work fine now, with lower speeds though. (1M/4M).
Thanks for your work!

@kmihaylov
Copy link
Contributor

kmihaylov commented Aug 27, 2021

The sample below has 4 possible scenarios:
Global SPI instance w/ VSPI
Global SPI instance w/ HSPI
Secondary SPI instance (w/ constructor initialization) w/ VSPI
Secondary SPI instance (w/ constructor initialization) w/ HSPI

Of all of them only the last one gives SPI communication.

Here's the code:

#include <SmingCore.h>
#include <Debug.h>

#define SPI_MISO 14
#define SPI_MOSI 27
#define SPI_CLK 12

#define DEVICE_CS 26

struct SpiPins spiPins = {SPI_CLK, SPI_MISO, SPI_MOSI, 255};

SPIClass someSpi(SpiBus::HSPI, spiPins); //HSPI works, VSPI doesn't work
SPIClass spiTarget;

Timer procTimer;
bool state = true;

void testSpi() {
	digitalWrite(DEVICE_CS, LOW);
	spiTarget.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE1));
	spiTarget.transfer(131);
	uint8_t res = spiTarget.transfer(0x00);
	debug_hex(ERR, "SPI res: ", &res, 1);
	spiTarget.endTransaction();
	digitalWrite(DEVICE_CS, HIGH);
}

void init()
{
	spiTarget = someSpi;
	//spiTarget = SPI;
	//spiTarget.setup(SpiBus::HSPI, spiPins); //needed for the global SPI instance; //neither HSPI nor VSPI works (global instance)
	Serial.begin(115200, SERIAL_8N1,
						 SERIAL_FULL);
	Debug.setDebug(Serial);
	Debug.initCommand();
	Serial.systemDebugOutput(true);
	pinMode(DEVICE_CS, OUTPUT);
	spiTarget.begin();
	procTimer.initializeMs(1000, testSpi).start();
}

Edit: the code above is wrong, here's the proper code with a pointer to the SPI obect:

#include <SmingCore.h>
#include <Debug.h>

#define SPI_MISO 14
#define SPI_MOSI 27
#define SPI_CLK 12

#define DEVICE_CS 26

struct SpiPins spiPins = {SPI_CLK, SPI_MISO, SPI_MOSI, 255};

//SPIClass someSpi(SpiBus::VSPI, spiPins); //HSPI works, VSPI doesn't work
SPIClass *spiTarget;

Timer procTimer;
bool state = true;

void testSpi() {
	digitalWrite(DEVICE_CS, LOW);
	spiTarget->beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE1));
	spiTarget->transfer(131);
	uint8_t res = spiTarget->transfer(0x00);
	debug_hex(ERR, "SPI res: ", &res, 1);
	spiTarget->endTransaction();
	digitalWrite(DEVICE_CS, HIGH);
}

void init()
{
	spiTarget = &SPI;
	spiTarget->setup(SpiBus::VSPI, spiPins); //needed for the global SPI instance; //neither HSPI nor VSPI works (global instance)
	Serial.begin(115200, SERIAL_8N1,
						 SERIAL_FULL);
	Debug.setDebug(Serial);
	Debug.initCommand();
	Serial.systemDebugOutput(true);
	pinMode(DEVICE_CS, OUTPUT);
	spiTarget->begin();
	procTimer.initializeMs(1000, testSpi).start();
}

And now with this code the only working condition is with the global SPI object and VSPI. ???

@mikee47
Copy link
Contributor Author

mikee47 commented Aug 27, 2021

I've simplified your example and checked operation using loopback (connecting MISO to MOSI) and all configurations produce the same results. What device are you testing this with? There could be a more subtle issue at work here.

Note: Tested using ESP-WROOM-32 module

#include <SmingCore.h>

constexpr struct SpiPins spiPins = {
	.sck = 12,
	.miso = 14,
	.mosi = 27,
	.ss = SPI_PIN_DEFAULT,
};

SPIClass someSpi(SpiBus::HSPI, spiPins);
SPIClass spiTarget;

Timer procTimer;

void testSpi()
{
	spiTarget.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE1));
	PSTR_ARRAY(buf, "Test data");
	auto len = strlen(buf);
	spiTarget.transfer(reinterpret_cast<uint8_t*>(buf), len);
	m_printHex(_F("SPI"), buf, len);
	spiTarget.endTransaction();
}

void init()
{
	Serial.begin(SERIAL_BAUD_RATE);
	Serial.systemDebugOutput(true);
	Serial.println(F("SPI loopback test running"));

	// spiTarget = someSpi;
	spiTarget = SPI;
	spiTarget.setup(SpiBus::VSPI, spiPins);
	spiTarget.begin();
	procTimer.initializeMs(1000, testSpi).start();
}

@kmihaylov
Copy link
Contributor

@mikee47 I use ESP32-WROVER, but I guess it only has PSRAM and different pinout. Let me use your code and here I'll find one board for testing with MISO connected to MOSI.

@kmihaylov
Copy link
Contributor

@mikee47 My tests confirm yours. Your example works with all combinations.

I confirm that ESP32-WROVER SPI works flawlessly.

I found two problems:

  1. My board probably had something between the MISO and other line. A footprint is missing its component and I suspect there was some particle over the pads?
  2. The CS5460 library has pinMode(SPI_MISO, INPUT_PULLUP); that drives the MISO line high. I checked the datasheet and found no reason for this. I'll pepare a PR later with removal of this line (and the corresponding variables in component.mk) if they're really useless.

I hope my SPI will be robust now. Thanks for your time! :)

@mikee47
Copy link
Contributor Author

mikee47 commented Aug 27, 2021

Internal pullups are usually pretty weak, properly driven that should be easily overcome... Maybe there's a problem with the pinMode implementation...

@kmihaylov
Copy link
Contributor

If I see unusual pinMode behavior I'll alarm. Otherwise the SPI communication was (almost) working. The pullup is weak for sure, resulting in long slope after the last bit.

Btw may I ask you about the instance declaration? I want to use a reference to the SPI object

I changed my classes to look like

class someClass{
private:
SPIClass &spiReference;
}

someClass::someClass(SPIClass &globalSPI) : spiReference(globalSPI) {}

Earlier I did something like

class someClass{
private:
SPIClass spiReference;

someClass::someClass(SPIClass globalSPI){
spiReference = &globalSPI;
}

And finally I see you just declare it only with SPIClass spiTarget and later spiTarget = SPI. But isn't it allocating twice the memory? For spiTarget that later is referenced to SPI?

@mikee47
Copy link
Contributor Author

mikee47 commented Aug 27, 2021

Yes, taking a copy of the object works and (sort of) takes a reference as the important member variable is busId. Semantically it's wrong though so the SPIClass copy constructor should be deleted. I'll include that in #2358.

@kmihaylov
Copy link
Contributor

kmihaylov commented Sep 2, 2021

@mikee47 you were right about the existence of some problem with the SPI, but it was my fault. I made a bug in one of my methods:

uint8_t kulPT100Class::readReg(uint8_t reg) {
	setSpi();
	digitalWrite(ifaceCS, LOW);
	privateSpi.transfer(reg);
	return privateSpi.transfer(0);
	digitalWrite(ifaceCS, HIGH);
	privateSpi.endTransaction();
}

It is pretty interesting that I got a lot of time with both ICs working properly. I suspect that their call timing was reversed (like IC2, IC1 instead IC1, IC2).

I just had another problem - there are two objects both working with the SPI peripheral. One of them does SPI transaction each second, and the other one each 500 ms.

I found that once the first one is initialized and started its 1 second loop, breaks the initialization of the second IC. Therefore I first initialize the IC that doesn't have internal Timer started and later the one with the timer.

How these two timers execute their tasks in relation to the SPI peripheral?

@slaff slaff mentioned this pull request Sep 15, 2021
5 tasks
slaff pushed a commit that referenced this pull request Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants