Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hal] Support 16bit transfers for Spi #690

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Sep 22, 2021

While solving #666 i've templatized Spi::transfer(..).

<std::unsigned_integral T> now controls wether to send uint8_t or uint16_t. It was a small step to make it work in bit-resolution so i've changed the argument to <int Bits>.

When sending uint16_t data with 16-bit. most of these fromBigEndian(...) sanities, spreaded over modm::drivers become obsolete. Moreover there's kind of Spi-integrated 'endianess' by varying 8 and 16 bits:

Transfer same data with 8 and 16 bits. No DMA for clarity

union {
	uint8_t data8[2] = {0xDE, 0xAD};
	uint16_t data16;
};
RF_CALL(SpiMaster::transfer(data8, (uint8_t*)(nullptr), 2)); // outputs 0xDEAD
RF_CALL(SpiMaster::transfer(data16)); // outputs 0xADDE

0xDEAD_endianess_small

Same with DMA

0xDEAD_endianess+DMA_small

For driver::Touch2046 it's code beautyfication first hand but also good for performance.
-> See 2nd commit

Sending color::Rgb565 (uint16_t underlying) with true 16-bit Spi and getting rid of all these endianess sanities is extremely healthy for driver::ili9341 performance
-> Fixed in 3rd commit.

I've spotted lot's of endianess sanities in I2c driven devices. Gonna see if the recipe applies here too.

ToDo

  • Modernize Spi API but keep it backwards compatible
  • Support direct uint16_t Spi transfers
    • original API: 'Spi::transfer16(uint16_t)' + derivates
    • new API 'Spi::transmit(T data)' + derivatives, T is deduced
  • implement non-Dma Spi-service with ISRs to prevent blocking
    • AVR
    • STM32
  • Update some drivers with improved API for reference.
    • driver::Ili9341 - uses new repeated transfer of same data: 'transmit<>(T data, std::size_t repeat)': evaluation: local huge buffer is redundant, improved performance for any single pixel
    • driver::Touch2046 - uses modernized API with STL style syntax: transmit(std::begin(tx16), std::end(tx16), std::begin(rx16)) evaluation: easy2read code, +improved performance

@salkinium
Copy link
Member

I think half of the STM32 devices only have 8 or 16 bit transfers. Not sure the AVR has anything else but 8bit.

I would only have two transfer functions: transfer and transfer16, since the register access is the same, so the code is the same. The bit width should be set outside the transfer function (since we also set everything else like SPI Mode externally).

@salkinium
Copy link
Member

salkinium commented Sep 22, 2021

There's also this->attachConfigurationHandler([]() { SPI::setTransferWidth(16); }); which is called only when the SpiDevice changes (ie. when you have multiple devices on the same SPI peripheral).

@TomSaw TomSaw mentioned this pull request Sep 22, 2021
20 tasks
@TomSaw
Copy link
Contributor Author

TomSaw commented Sep 22, 2021

There's also this->attachConfigurationHandler([]() { SPI::setTransferWidth(16); }); which is called only when the SpiDevice changes (ie. when you have multiple devices on the same SPI peripheral).

ili9341 requires both, 8bit and 16bit. There's no way to attach multiple Handlers as far as i know.
EDIT: I'm switching the configurationHandler between 8 and 16bit in ili9341_spi.hpp. driver:touch2046 requires only 16bit configuration.

@TomSaw TomSaw changed the title [hal] Bit resolution Spi [hal] Teach Spi more tricks to improve graphics performance Sep 23, 2021
@TomSaw TomSaw changed the title [hal] Teach Spi more tricks to improve graphics performance [hal] Teach Spi more tricks for improve graphics performance Sep 23, 2021
@TomSaw TomSaw changed the title [hal] Teach Spi more tricks for improve graphics performance [hal] Teach Spi more tricks for better graphics performance Sep 23, 2021
@TomSaw

This comment has been minimized.

@TomSaw TomSaw force-pushed the bit-resolution-Spi branch 3 times, most recently from 14c9c88 to 321b0af Compare September 27, 2021 08:36
@TomSaw TomSaw changed the title [hal] Teach Spi more tricks for better graphics performance [hal] Add 16bit support to Spi (and I2c) Sep 27, 2021
@TomSaw

This comment has been minimized.

@TomSaw TomSaw force-pushed the bit-resolution-Spi branch 10 times, most recently from 2170987 to b7a28d7 Compare September 28, 2021 13:20
@TomSaw TomSaw changed the title [hal] Add 16bit support to Spi (and I2c) [hal] Support 16bit transfers for Spi and I2c Sep 29, 2021
@salkinium
Copy link
Member

Much better, thanks for fixing the SPI ISR. There was an lbuild option once to enable blocking SPI transfers for fast speeds, since walking up and down the call chain takes so long. An ISR may be a little slower, but at least its non-blocking.

@TomSaw
Copy link
Contributor Author

TomSaw commented Oct 5, 2021

So then I'll add this to STMs non-DMA SPI as well.

How do you like the use of MODM_FLAGS for the SPIs state variable @salkinium ? Found the answer on my own: It's wanted ✅

@TomSaw
Copy link
Contributor Author

TomSaw commented Oct 6, 2021

Ahhh... dreamed another great, apparent idea for the modernized versions of Spi::transfer:
STL algorithm style interface!

  1. Its intuitive to C++ people
  2. Is a safe bet on the future
  3. works with C-array & std::array (EDITED)

...I ❤️ it, hope you too. The interfaces would be:

static modm::ResumableResult<T>
transmit(const T data);

static modm::ResumableResult<void>
transmit(const T data, std::size_t repeat);

// STL algorithm style
static modm::ResumableResult<void>
transmit(const T *tx_first, const T *tx_last, T *rx_first = nullptr);

@salkinium
Copy link
Member

Ok, but let's focus on finishing this PR first before refactoring everything again.

Also keep in mind that the advantage of using stl iterators is that the Iterator is an actual separate type that overloads operator++ etc to implement any kind of data access. This is generally a good idea, where an non-sequential buffer access can be abstracted.

However, this only works generically with a template function, where this iterator type is known, and becomes difficult with an IRQ, which can be defined only once. You now need to use a function pointer or a virtual interface and then you are basically implementing the I2cTransaction that sucks a little.

I also don't see a significant usability advantage of using transmit(begin(), end(), begin()) vs the API that we have now.

@salkinium
Copy link
Member

I've looked into a "stringly-typed" printf-style generic transfer interface, which would probably result in a significantly more compact implementation, since it explicitly exposes a state-machine interface, uses a shared parser between all peripherals and that can be translated at compile-time into a trivially DMA-friendly representation. Here is a prototype for SoftwareI2C that worked in hardware.

@TomSaw TomSaw force-pushed the bit-resolution-Spi branch 7 times, most recently from 138c867 to 1f7c5c8 Compare October 9, 2021 11:30
Comment on lines 81 to 78
{
if constexpr ( SupportsBit16<SPI> )
SPI::setDataSize(SPI::DataSize::Bit16);
SPI::transferBlocking16(rgb565.color, repeat);
Copy link
Contributor Author

@TomSaw TomSaw Oct 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16bit hardware detection works with constexpr and concept now. The concept is on top of ili9341_spi.hpp for now, may be refined and moved closer to Spi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fancy shit I like! 👍👍👍👍

Copy link
Contributor Author

@TomSaw TomSaw Oct 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jupp, the concepts concept is epic and i'm very grateful that I have only dived into the complex realms of templates after C++20

@TomSaw
Copy link
Contributor Author

TomSaw commented Oct 9, 2021

Also keep in mind that the advantage of using stl iterators is that the Iterator is an actual separate type that overloads operator++ etc to implement any kind of data access. This is generally a good idea, where an non-sequential buffer access can be abstracted.

To continue: Containers with ContiguousIterator may return plain pointers for begin() and end().
There's also a concept contiguous_iterator
Quote The contiguous_iterator concept refines random_access_iterator by providing a guarantee the denoted elements are stored contiguously in the memory.

These Containers come into question:

  • classic C-array
  • std::array
  • std::basic_string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants