Skip to content

RP2040 constructor rework#19

Merged
RobTillaart merged 4 commits into
RobTillaart:masterfrom
Intubun:master
Oct 18, 2022
Merged

RP2040 constructor rework#19
RobTillaart merged 4 commits into
RobTillaart:masterfrom
Intubun:master

Conversation

@Intubun
Copy link
Copy Markdown
Contributor

@Intubun Intubun commented Oct 18, 2022

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

@Intubun Intubun changed the title RP2040 contructor rework RP2040 constructor rework Oct 18, 2022
@Intubun
Copy link
Copy Markdown
Contributor Author

Intubun commented Oct 18, 2022

Yeehaw! It worked the first time!

@RobTillaart RobTillaart self-assigned this Oct 18, 2022
@RobTillaart
Copy link
Copy Markdown
Owner

RobTillaart commented Oct 18, 2022

@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
Copy link
Copy Markdown
Contributor Author

Intubun commented Oct 18, 2022

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

@RobTillaart
Copy link
Copy Markdown
Owner

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
Copy link
Copy Markdown
Owner

No further remarks, thanks!

@RobTillaart RobTillaart linked an issue Oct 18, 2022 that may be closed by this pull request
@Intubun
Copy link
Copy Markdown
Contributor Author

Intubun commented Oct 18, 2022

Your requested changes are done...

@RobTillaart RobTillaart merged commit 4123df8 into RobTillaart:master Oct 18, 2022
@RobTillaart
Copy link
Copy Markdown
Owner

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

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.

RP2040 interface - redo

2 participants