RobTillaart / MCP_DAC

Arduino library for MCP_DAC MCP48xx and MCP49xx series SPI-DAC
MIT License
32 stars 8 forks source link

Compatibility with ESP32c3 and ESP32s3 - VSPI not valid for these chipsets #24

Closed BlackOilCoder closed 11 months ago

BlackOilCoder commented 12 months ago

I'm trying to incorporate the library into my ESP32s3 project. I started my work by just #including the library in my code (with no calls to any functions), but get compile errors related to VSPI.

Further research indicates changes in the S3 architecture/drivers - which also apparently apply to the C3 - removed the VSPI definition.

The esp32-hal-spi.h included via spi.h in the current ESP-IDF has the following definitions:

#if CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3
#define FSPI  0
#define HSPI  1  
#else  
#define FSPI  1 //SPI bus attached to the flash (can use the same data lines but different SS)  
#define HSPI  2 //SPI bus normally mapped to pins 12 - 15, but can be matrixed to any pins  
#if CONFIG_IDF_TARGET_ESP32  
#define VSPI  3 //SPI bus normally attached to pins 5, 18, 19 and 23, but can be matrixed to any pins  
#endif  
#endif

I believe only the FSPI and HSPI is valid for the S3/C3 units. In the current spi.h the following is tested in .begin():

        _sck = (_spi_num == FSPI) ? SCK : -1;  
        _miso = (_spi_num == FSPI) ? MISO : -1;  
        _mosi = (_spi_num == FSPI) ? MOSI : -1;  
        _ss = (_spi_num == FSPI) ? SS : -1;

I have to admit I have no idea what the = (_spi_num == FSPI) ? SCK: -1 means. I assume this is testing to see if the SCK/MISO/MOSI/SS terms are defined and if not, assigning them null values.

I believe the following section of the MCP_DAC.h needs to be extended to test for another ESP version being defined. Adding a function "selectFPSI()" and variable "usesFPSI()".

#if defined(ESP32)                    // ESP32 specific  

  void     selectHSPI();  
  void     selectVSPI();  
  bool     usesHSPI();  
  bool     usesVSPI();  

  // to overrule the ESP32s default hardware pins  
  void     setGPIOpins(uint8_t clk, uint8_t miso, uint8_t mosi, uint8_t select);  

#elif defined(ARDUINO_ARCH_RP2040)    // RP2040 specific  

  // to overrule the RP2040s default hardware pins  
  void     setGPIOpins(uint8_t clk, uint8_t miso, uint8_t mosi, uint8_t select);  

#endif

I do worry that the selection criteria may have to be more stringent as the s3 model likely also qualifies as ESP32 - which is likely why my code fails to compile as the current code sees it as an "ESP32". So maybe:

#if defined(ESP32 && (CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3)

#elif defined(ESP32)...

I have no idea if those are the correct definition to test for, but the spi.h uses them. I need to trace down exactly where #define ESP32 is located as well.

Following this of course, the MAC_DAC.cpp modified such that the inner class MCP_DAC defined begin function would need to be modified to include the configuration for the newly defined ESP32s3. Also need to add a function for selectFPSI().

I may try a pull request, but I'll continue my honestly by saying that you have seen the extent of my C++ knowledge in this post. That knowledge has rotted - being 20+ years out from my last class on the subject. I'm just a hobbyist, no developer. Also, I have to figure out all the git stuff.

RobTillaart commented 12 months ago

Thanks for this issue I have no ESP32-S3 so it is difficult to recreate. (Need to buy a few)

Will try to look into this tomorrow.

BlackOilCoder commented 12 months ago

Let me know if you need to test anything. I have a board I'm working with.

RobTillaart commented 12 months ago

@BlackOilCoder Created a branch ESP32_S3

Can you test this on your ESP32_S3?

//
//    FILE: MCP4921_xxxxx.ino
//  AUTHOR: Rob Tillaart
// PURPOSE: test MCP4921 lib
//     URL: https://github.com/RobTillaart/MCP4921

#ifndef ESP32
#error ESP32 only example, please select appropriate board
#endif

#include "MCP_DAC.h"

MCP4921 MCP(HSPI);  // HW SPI  <<<<<<<<< Select your SPI port here

volatile int x;
uint32_t start, stop;

void setup()
{
  Serial.begin(115200);
  Serial.println(__FILE__);

//  MCP.selectVSPI();     // needs to be called before begin()
                        // uses default HSPI SCLK=14, MISO=12, MOSI=13, SELECT=15
                        // uses default VSPI SCLK=18, MISO=19, MOSI=23, SELECT=5
  MCP.begin(5);         // 5 for VSPI and 15 for HSPI

  // experimental
  // MCP.setGPIOpins(23, 18, 19, 15);  //  CLK MISO MOSI SELECT

  Serial.print("MCP_DAC_LIB_VERSION: ");
  Serial.println(MCP_DAC_LIB_VERSION);
  Serial.println();
  Serial.print("CHANNELS:\t");
  Serial.println(MCP.channels());
  Serial.print("MAXVALUE:\t");
  Serial.println(MCP.maxValue());
  delay(100);

  // MCP.setSPIspeed(100000);  // for slower scopes
  performance_test();

  Serial.println("\nDone...");
}

void performance_test()
{
  Serial.println();
  Serial.println(__FUNCTION__);

  start = micros();
  for (uint16_t value = 0; value < MCP.maxValue(); value++)
  {
    x = MCP.analogWrite(value, 0);
  }
  stop = micros();
  Serial.print(MCP.maxValue());
  Serial.print(" x MCP.analogWrite():\t");
  Serial.print(stop - start);
  Serial.print("\t");
  Serial.println((stop - start) / (MCP.maxValue() + 1.0) );
  delay(10);

  start = micros();
  for (uint16_t value = 0; value < MCP.maxValue(); value++)
  {
    MCP.fastWriteA(value);
  }
  stop = micros();
  Serial.print(MCP.maxValue());
  Serial.print(" x MCP.fastWriteA():\t");
  Serial.print(stop - start);
  Serial.print("\t");
  Serial.println((stop - start) / (MCP.maxValue() + 1.0) );
  delay(10); 
}

void loop()
{
  performance_test();
}

// -- END OF FILE --
BlackOilCoder commented 12 months ago

It appears to be working! Here's some console output

performance_test
4095 x MCP.analogWrite():       57689   14.08
4095 x MCP.fastWriteA():        56871   13.88

performance_test
4095 x MCP.analogWrite():       57689   14.08
4095 x MCP.fastWriteA():        56871   13.88

performance_test
4095 x MCP.analogWrite():       57684   14.08
4095 x MCP.fastWriteA():        56871   13.88

performance_test
4095 x MCP.analogWrite():       57689   14.08
4095 x MCP.fastWriteA():        56871   13.88

performance_test
4095 x MCP.analogWrite():       57692   14.08
4095 x MCP.fastWriteA():        56871   13.88

performance_test
4095 x MCP.analogWrite():       57684   14.08
4095 x MCP.fastWriteA():        56871   13.88

performance_test
4095 x MCP.analogWrite():       57689   14.08
4095 x MCP.fastWriteA():        56871   13.88

I have the DAC chip arriving at my place on Friday to verify actual performance.

A general question I have. I had to move the performance_test() function above the setup() as VSCode platform.io didn't like it in the basic config. This is something that is particular to the Arduino IDE that lets you declare functions in any order, correct?

I'll keep reviewing things as I knocked this out before heading to work this morning, but it does compile and that test seems to run. I can verify performance with a chip this weekend.

RobTillaart commented 12 months ago

The output looks pretty good.
As SPI does not check if device is actually present, it indicates the performance you get.

A general question I have. I had to move the performance_test() function above the setup() as VSCode platform.io didn't like it in the basic config. This is something that is particular to the Arduino IDE that lets you declare functions in any order, correct?

Yes, not being able to cope with "any order" is a known issue of some environments. It would take several months to check all my libraries and make them run instantly. I make a note for it for my winter 2024-2025 evenings as a possible activity.


I have installed the ESP32-S3 2.0.11 version this morning and I get an error on analogWrite(). Which board are you using?

BlackOilCoder commented 12 months ago

I'm using the Unexpected Maker TinyS3. Which ESP-IDF version are you using? I'm just loading platform: espressif32 in platformio.

The function order thing isn't an issue. That's only in example code, which is easy enough to adjust.

RobTillaart commented 12 months ago

2.0.11 gave the problem when I selected the Arduino Nano Esp32s3. Think i need to report a bug as the analogWrite() is defined as a macro. Which is a bad idea imgo.

BlackOilCoder commented 12 months ago

Hmmm. In Platformio, I'm just calling the following in my platformio.ini.

framework = arduino

I need to determine what version that is calling.

RobTillaart commented 12 months ago

Found this - https://github.com/mathertel/Arduino_GFX/pull/1/files

Describes exactly the problem of using analogWrite() as a method in the MCP_DAC class. for the Arduino Nano 32 there is a macro that renames analogWrite() in io_pin_remap.h.

Solution / patch implemented in the above PR is renaming analogWrite() et al in the library.

Problem might be around with other boards too.

BlackOilCoder commented 12 months ago

I connected up my TinyS3 to a MCP4922 and PAM8203 amp this weekend. I'm able to fully utilize the wave generator example. Seems as if things work well!

RobTillaart commented 12 months ago

f you change (sw) the address of the 4922 in the sketch, and you change (hw) the address pins of the device accordingly does it still work?

Sorry wrong issue

BlackOilCoder commented 12 months ago

Just to make sure I'm following, you mean selecting FSPI or HSPI? I've tried both of those and they work. Do you want me to try using any available pin set for the SPI as well you mean in the second part of your question?

RobTillaart commented 12 months ago

My mistake, was working from my phone and mixed up issues.

Testing HSPI and FSPI is of course a good test.

RobTillaart commented 11 months ago

@BlackOilCoder

Rewrote the constructor interface (and removed a lot) in the ESP32_S3 branch. (created a PR) See https://github.com/RobTillaart/MCP_DAC/tree/ESP32_S3

breaking change

If you have time, please give it a try on the ESP32_S3

BlackOilCoder commented 11 months ago

I will test this weekend. It's a holiday this week where I'm at so I'll be doing family stuff until the weekend. Thanks again for your support updating this! I should be able to get my project moving with it. I have some test boards showing up so I can test my real time audio generation. Using an amp on a breadboard doesn't really work (too much distortion), but my trials indicate it should work.

RobTillaart commented 11 months ago

Merged the develop branch into master and released 0.3.0

(Still interested in your experiences)