BUG: Library is unable to share SPI device or specify custom ports

[petrkr]

Libraries should NOT initialize other hardware than for which is that library written. For example here you initialize SPI without parameters, which leads this library does not work on other than default pins defined somewhere in Arduino pinout.

Library should NOT change parameters of used interfaces like SPI, instead user must pass SPI object instead..

Also it can not change speed or SPI mode, because it may cause collisions with other hardwars on same bus (Mode must be same, but speed may broke something what does not support such fast SPI or slow down fast device like SPI ethernet or TFT screen).

I suggest to use SPI Class instead doing SPI.begin() Something like here is TwoWire used https://github.com/petrkr/SegLCDLib/blob/master/src/SegDriver_PCx85.h#L38

[bartoszbielawski]

Hi,

You're not wrong, it would be better to pass the SPIClass object to the constructor.

Now, I'm using SPI.beginTransaction() that takes an instance of SPISettings, and this struct contains speed, mode and bit order.
I guess this needs to be replaced with something else. Any suggestions?

I'm open to pull requests, otherwise this bug may wait a bit till I find some spare time to tinker with it.

Cheers,
Bartosz

[petrkr]

I made it in fork like this way:
petrkr@643f688

But it is not backward compatible with that, how it is now.

That I do not need for now. So for me this is fine. And yea. Everyone tells me "open pullrequest".. Problem is my time, if I will repair and optimize all opensource code, which tells me "do it and make PR" then I will do it already on full-time. Power of open source is that contribution and ever one do some part, not everything.

Thats is why I opened bug report and not did it whole. For other open source project, where I uses it (and where now I am busy with time) I fixed it somehow, how I think it is fine and now do not have too much time to make it proper and best way here...

Anyway thanks for reply

[bartoszbielawski]

Okay, this is how I would do it myself too. Unfortunately the code still changes settings of the SPI and I don't see an obvious way of reverting changes after the transaction in an easy way.

Once I find time for it I will just add the second constructor and keep the original one which will use the SPI object by default.

BB

[petrkr]

Once I find time for it I will just add the second constructor and keep the original one which will use the SPI object by default.

BB

Yes that was something I was think about too. 2nd constructor how I made it, and original with SPI.begin().. maybe deduplicate same code somewhere outside in some private _begin or something there do everything and in one ctor just call begin() in second with default SPI call begin() and SPI.begin after.

Meanwhile for me this fork is working, so maybe I can catch it to event, where I want present ESP32C3 device with it. But would be nice to use upstream version, because for other users will be easier to just use "library manager" instead of cloning it from forked repo.

73

[bartoszbielawski]

There's a new branch explicit-spi that implements this additional constructor.

I'm actually surprised because apparently I wrote a code like that 4 years ago and never merged it - it just sits there...

I can test the code tomorrow and make a release if all is good.

BB

[petrkr]

Maybe nobody had that problem, because probably using default SPI pinout. Me also was using ESP8266 on default SPI pins, that is why it works... But today I had update hardware to ESP32C3, where SPI does not sits on default pins on that board, which I have, so I started debug why it does not works.

Do not know if other peoples just used different library or whatever... I like this one, because it uses SPI object, others use some hand-made SPI emulation by digitalWrite, sleep, digitalWrite, what is nonsence, because they can not use more SPI devices on same lines then.. And since ESP32C3 have really small amount of GPIO, there is no reason to waste 3 extra gpio lines when only 1 (cs) is actually needee