XPT2046 portduino tweaks

[jp-bennett]

This works for me, but may be a problem for other targets. If so, I think we can add some ifdef ARDUINO to fix it up. Feedback welcome.

The basic idea here is that some targets (namely Portduino) may have SPI devices that automatically manage their Chip Select lines, and are not just the "SPI" device. So this adds a configurable SPI device for touch, and doesn't fail when CS is unset.

[jp-bennett]

After a couple false starts, this finally works. I'll point out that @mverch67 thinks this is not a great approach, and has an idea to re-use the cfg.spi_host value to specify the SPI device. Not sure how we want to handle this. Maybe pull this code once it's in shape, and then revisit when he writes his alternative?

Related to that, is arduino_default used by anything other than Portduino/Meshtastic? I'm assuming it is, which is why I'm trying to keep the code that goes in there minimal, and as broadly applicable as I can.

[mverch67]

As discussed (internally) I think this is not a good design to pass pointers of lowers layers through the entire architecture. Also the lovyanGFX interface becomes now rather ugly because of this inconsistency and the artificial dependency to a lower layer which is not necessary.

My preferred solution would be:

• SPIClass (=HardwareSPI) has to provide a ctor HardwareSPI(uint8_t spi_host); like it is already available for esp32-arduino
• The _arduinodefault Bus_SPI class implementation defines a variable SPIClass spi; (I honestly don't like the name "PrivateSPI"; also local class variables do not start with a capital letter)
• In Bus_SPI::init() it can then based on the configured spi_host do spi = SPIClass(_cfg.spi_host);
• The new ctor and HardwareSPI::begin must implement the usage of the correct spi_host in the portduino-framework

That should do it.

[mverch67]

Here, this ctor is missing in the portduino-framework and this is exactly the reason why we are having the issue to configure the SPI interface properly:

https://github.com/espressif/arduino-esp32/blob/cf448906b3836fbe9368934713b697469254c62f/libraries/SPI/src/SPI.h#L62

[tobozo]

yup Touch.hpp should stay pure from any bus class and I'm not sure all those ifdefs should spread that far outside platforms/arduino_default

lets keep this PR open for the meantime and see if a lighter approach is possible

[jp-bennett]

I've struggled with how to make this lighter, and yet support all the needed cases. We can't define an SPI bus on Linux with just a single int. Is new HardwareSPI(1) referring to spidev0.1 or spidev1.0? Also, declaring a new HardwareSPI could be a problem for the cases where we actually want to use the same SPI device, and let the LovyanGFX library manage the CS line. To handle all the edge cases is to move further and further away from the arduino API we're trying to use.

I'm about convinced that the way forward here is to just create three global SPI devices in Portduino instead of just one, and use spi_host to pick the right one.

[tobozo]

this ctor is missing in the portduino-framework

can this be addressed ? this would solve a few problems

if not, then maybe it'll be easier to just create a platforms/portduino folder and put the modified bus in there?

here's what I suggest for this PR: only keep the changes in touch/Touch_XPT2046.cpp and discard the rest, then create a discussion about portduino and spi buses where each POC is presented as an [env] in a basic platformio project (easy to checkout/update/test) and a commit hash

[jp-bennett]

only keep the changes in touch/Touch_XPT2046.cpp and discard the rest,

They don't do anything by themselves. isSPI() will always return false, if CS is set to a negative value. Should we remove the isSPI() call for touch devices that are SPI only?

[tobozo]

Should we remove the isSPI() call for touch devices that are SPI only?

according to the readme the XPT2046 support is for SPI versions only so it's fine to remove that

[jp-bennett]

@tobozo Minimized this PR to the XPT2046 change. We have a workable solution for the SPI bus issue, that I'll make a separate PR once it's fully tested.