search

autowp / arduino-mcp2515

Arduino MCP2515 CAN interface library
MIT License
795 stars 279 forks source link

Added optional parameter for SPI instance #54

Closed FStefanni closed 2 years ago

FStefanni commented 3 years ago

Hi,

this pr adds an optional parameter to allow passing a custom SPI instance. Implements the proposal of issue #53. Closes #53.

Regards

morcibacsi commented 3 years ago

This is almost good, except that the spi.begin is called from inside the library. This way it can't be used with boards which allow you to use custom spi pins (for example ESP32, BluePill). As that requires you to call the spi.begin this way: spi.begin(SCK_PIN, MISO_PIN, MOSI_PIN, CS_PIN). I would refactor this in a way that the library gets a pointer to an already instantiated and correctly set up SPIClass.

Something like this: https://github.com/morcibacsi/arduino-mcp2515/commit/7fd6523b224a10cdad916a96a63b393fe4268d0c

I know it is a breaking change but this gives the most flexibility across boards.

FStefanni commented 3 years ago

Hi,

I agree, but doing this change is not the point of this pr.

Nevertheless, doing your proposed change is probably possible without breaking anything, by adding an additional boolean parameter, to signal whether to perform the begin inside the library. If this further improvement is of interest, I could check the code and in case improve the pr.

Regards.

morcibacsi commented 3 years ago

I see... my point was that IMO if we modify the constructors it is worth considering to inject the dependency as this way it adds more flexibility to the library.

FStefanni commented 3 years ago

Hi,

@morcibacsi I added the boolean, to support your case, with fully backward compatibility. I hope this helps.

Regards.

morcibacsi commented 3 years ago

@FStefanni nice, thank you very much!

FStefanni commented 3 years ago

Hi,

the regression pipeline fails due to missing #include <core_cm4.h>. Maybe something not properly configured? Seems not something up to this pr.

Regards.

CarlosHdezM commented 2 years ago

Hi @autowp, @FStefanni, and @morcibacsi. As I consider the possibility of using an SPI other than the default a huge step up, I decided to implement the functionality (#84) by continuing in the line of @FStefanni and taking the suggestions of @morcibacsi for greater flexibility with boards that support assigning different pins to the SPI used.

I hope it can be considered to be merged. Thank you!

autowp commented 2 years ago

Sounds good but there is some extra changes in library.properties to be removed from PR Also "doBegin" sounds not clear

CarlosHdezM commented 2 years ago

@autowp, I'm sorry for the confusion. I commented on this PR since I based the proposed changes on it. My proposed changes are actually in PR #84. I hope you can take a look at it. Thanks in advance!

FStefanni commented 2 years ago

Hi,

@CarlosHdezM thank you for the update, since my pr is very old: this has been a missing feature I needed in a (now) old project.

Regards.