RobTillaart / MCP_DAC

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

RP2040 constructor rework #19

Closed Intubun closed 2 years ago

Intubun commented 2 years ago

Issue #18 Should work right out of the box... Validated with MCP4822 and RPi Pico

Intubun commented 2 years ago

Yeehaw! It worked the first time!

RobTillaart commented 2 years ago

@Intubun Think you did a good job. In the begin() function you have

    #elif defined(ARDUINO_ARCH_RP2040)
    mySPI->end();
    mySPI->begin();
    #else              // generic hardware SPI
    mySPI = &SPI;
    mySPI->end();
    mySPI->begin();
    #endif

I think this must be replaced by

    #else              // generic hardware SPI
    mySPI->end();
    mySPI->begin();
    #endif

mySPI is already set in the constructor and would be overwritten in case of non-RP2040

Should work for RP2040 too.

Intubun commented 2 years ago

Oh.. Forgot one there... Well another commit is necessary...

RobTillaart commented 2 years ago

In the derived classes in the mcp_dac.h you use many times the conditional construct.

class MCP4801 : public MCP_DAC
{
public:
  #if defined(ARDUINO_ARCH_RP2040)
  MCP4801(uint8_t dataOut = 255, uint8_t clock = 255, SPIClassRP2040 *inSPI = &SPI);
  #else
  MCP4801(uint8_t dataOut = 255, uint8_t clock = 255, SPIClass *inSPI = &SPI);
  #endif
};

technically this is OK, but in the mcp_dac.cpp you do it with one #if RP2040 in a big block. I like that much better as when I read the code I do not need to switch context that often. (it is not wrong, but the ".cpp way" is far easier to maintain imho.

RobTillaart commented 2 years ago

No further remarks, thanks!

Intubun commented 2 years ago

Your requested changes are done...

RobTillaart commented 2 years ago

Great work, I will update the version and release later today / tomorrow.