Intend to clean up and add function to SPI

Post Reply
stevestrong
Posts: 2054
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Intend to clean up and add function to SPI

Post by stevestrong » Thu Jul 27, 2017 2:05 pm

Hi all,

my intention is to perform following changes in the SPI lib:

1. cleanup - remove DEBUG lines - as we agreed that it is mature enough not to have these line, and it will anyway not work
Related libs: - all SPI related.
Risk: - I see very low or no risk at all here, as no active lines are going to be touched.

2. change the write() function / implement a new one for 2x8 bit access
Currently it takes an uint16_t as input parameter (see: https://github.com/rogerclarkmelbourne/ ... I.cpp#L346). I changed it a while ago to make it universally usable to write 8 bit and 16 bit data as well. However, this will only write 16 bit in 16 bit mode (DFF=1).
My intention is to change it back to take an uin8_t parameter as it originally was, to write a single byte.
In addition, I would like to implement a true uint16_t write function even if DFF=0, which means to write consecutively two bytes: high byte first, then low byte. This will do either a single 16 bit write (DFF=1) or two consecutive 8 bit writes (DFF=0).

This would help to optimize the Ethernet library, and any other lib which requires at least two consecutive 8 bit writes (for example of an 16 bit address) in 8 bit mode (which is default).

Related libs: - all libs using this function in 16 bit mode. I only know Adafruit_ILI9341_STM which was lately optimized by me to use mostly 16 bit mode accesses. I intend to adapt/optimize this lib accordingly (actually changing the SPI function name would suffice).
Risk: - medium risk as there are not many libs using the 16 bit mode, I actually only know the one mentioned above. The rest of the libs will need no change and should work as before. Anyway, I will do extensive tests on this.

Proposal:

Code: Select all

void SPIClass::write16(uint16 data)
{
    /* Added for 16 bit write access.
     * This writes 16 bits in 16 bit mode (DFF=1) or two consecutive 8 bits [high byte + low byte] (DFF=0)
     */
    if ( !(_currentSetting->spi_d->regs->CR1 & SPI_CR1_DFF) ) { // 8 bit mode ?
        spi_tx_reg(_currentSetting->spi_d, data>>8);              // write high byte
        while (spi_is_tx_empty(_currentSetting->spi_d) == 0); // wait until TXE=1
    }
    spi_tx_reg(_currentSetting->spi_d, data);                       // write low byte or word
    while (spi_is_tx_empty(_currentSetting->spi_d) == 0);    // Wait until TXE=1
    while (spi_is_busy(_currentSetting->spi_d) != 0);            // wait until BSY=0
}
This will introduce a small overhead to check the DFF bit, but the lost run-time will be compensated in the ILI9341 lib in 16bit mode by using direct port writes for DC and CS pins.

Alternatively, one could leave the current write() function as it is for 16 bit mode, and define another function for 8 bit mode. This will remove any overhead and give best speed performance, and reduce the risk to a minimum as no changes are required in the actual libs (one could change them to optimize, but is not mandatory, they shall work as before).

Code: Select all

void SPIClass::write16(uint16 data)
{
    /* Added for 16 bit write access in 8 bit mode (DFF=0)
     * This writes two consecutive 8 bits [high byte + low byte]
     */
    spi_tx_reg(_currentSetting->spi_d, data>>8);          // write high byte
    while (spi_is_tx_empty(_currentSetting->spi_d) == 0); // wait until TXE=1
    spi_tx_reg(_currentSetting->spi_d, data);             // write low byte
    while (spi_is_tx_empty(_currentSetting->spi_d) == 0); // Wait until TXE=1
    while (spi_is_busy(_currentSetting->spi_d) != 0);     // wait until BSY=0
}
In a similar way, the respective 16 bit read function transfer16() is also targeted to be changed to read two consecutive bytes instead of one 16 bit read (https://www.arduino.cc/en/Reference/SPITransfer)

Any comment/opinion/suggestion is welcome.

victor_pv
Posts: 1860
Joined: Mon Apr 27, 2015 12:12 pm

Re: Intend to clean up and add function to SPI

Post by victor_pv » Thu Jul 27, 2017 3:55 pm

Steve, I think the first version seems like a better option to me.
If using the second and the SPI port is set to 16bit more, it would cause 2x 16bit transfers.

stevestrong
Posts: 2054
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Re: Intend to clean up and add function to SPI

Post by stevestrong » Fri Jul 28, 2017 8:38 am

Victor, thanks for your opinion.
Nevertheless, I find somehow option 2 better.
The official Arduino transfer16() specification (see link above) seems to target two 8 bit access, so the corresponding write16() should also target two 8 bit accesses.
The existing write() function would also remain unchanged, which is good, to keep compatibility with existing libs.
Just that the user should know the limitations of these functions, keeping in mind that write16() is not designed for 16 bit mode, similar to official Arduino transfer16(). Anyway, very few users have 16 bit mode in their apps.
Otherwise I don't see any problem with that.
I already tested option 2 together with a reworked transfer16() and they work fine, as expected.
The speed gain is about 20% compared to two consecutive byte accesses, also dependent on the implemented CS toggling (fast IO with direct register access or using digitalWrite). The 20 % is with fast IO @ 36 MHz, with digitalWrite the gain is even more.

stevestrong
Posts: 2054
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Re: Intend to clean up and add function to SPI

Post by stevestrong » Fri Jul 28, 2017 4:04 pm


victor_pv
Posts: 1860
Joined: Mon Apr 27, 2015 12:12 pm

Re: Intend to clean up and add function to SPI

Post by victor_pv » Fri Jul 28, 2017 4:29 pm

stevestrong wrote:
Fri Jul 28, 2017 8:38 am
Victor, thanks for your opinion.
Nevertheless, I find somehow option 2 better.
The official Arduino transfer16() specification (see link above) seems to target two 8 bit access, so the corresponding write16() should also target two 8 bit accesses.
The existing write() function would also remain unchanged, which is good, to keep compatibility with existing libs.
Just that the user should know the limitations of these functions, keeping in mind that write16() is not designed for 16 bit mode, similar to official Arduino transfer16(). Anyway, very few users have 16 bit mode in their apps.
Otherwise I don't see any problem with that.
I already tested option 2 together with a reworked transfer16() and they work fine, as expected.
The speed gain is about 20% compared to two consecutive byte accesses, also dependent on the implemented CS toggling (fast IO with direct register access or using digitalWrite). The 20 % is with fast IO @ 36 MHz, with digitalWrite the gain is even more.
Besides you and me, few other people likely use the 16bit mode, but it's still kind of risky if we have a write16() function that in case of settnig the port to 16 bit mode, will send corrupted data.
In the form above, if called with xxxxxxxxyyyyyyyy, it would first send 00000000xxxxxxxx, and next xxxxxxxxyyyyyyyy (reordered to whatever the port is set).
Did you measure the penalty of including the if statement to check the mode? it may be negligible, and although not likely to happen, I think is better to prevent issues that we can foresee.
Perhaps if the performance impact is enough to be measured, we can writ the check in a why that the transfers are not penalized in 8bit mode and only in 16bit mode?

EDIT: And looking at the way you wrote it, the if statement would only cause a jump in 16bit mode, so the 8bit mode penalty is just the number of instructions needed to check the if is false, and keeps running with no jump. It's hopefully just a couple of instructions.

stevestrong
Posts: 2054
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Re: Intend to clean up and add function to SPI

Post by stevestrong » Fri Jul 28, 2017 4:41 pm

The issue here is exactly the penalty for checking the bit mode. This hurts in both 8 and 16 bit mode, specially in the LCD lib where a lot of writes are performed and the speed counts.
Btw, the time I added a safety mechanism to change the bit mode, 2 more code lines caused partially more than 10% speed penalty by graphicstest: https://github.com/rogerclarkmelbourne/ ... -303317758
That's why I tend to avoid adding extra line to existing function, unless is unavoidable.

And as I said, this implementation is inline with the official Arduino specification.

Otoh, I can understand your concerns, but each of us (those few) using 16 bit mode is aware of these concerns, therefore we can safely use it accordingly.
I think this variant offers more advantages than drawbacks.

victor_pv
Posts: 1860
Joined: Mon Apr 27, 2015 12:12 pm

Re: Intend to clean up and add function to SPI

Post by victor_pv » Fri Jul 28, 2017 9:52 pm

Well if the penalty is 10%, that's a pretty high penalty.
If you can add a note in the comments above the function that doesn't work if the port is set to 16bit mode, at least will be a warning for anyone thinking on using it while in that mode.

User avatar
RogerClark
Posts: 7681
Joined: Mon Apr 27, 2015 10:36 am
Location: Melbourne, Australia
Contact:

Re: Intend to clean up and add function to SPI

Post by RogerClark » Fri Jul 28, 2017 10:17 pm

Does this slow the exiting 8 bit transfers by 10% ???

stevestrong
Posts: 2054
Joined: Mon Oct 19, 2015 12:06 am
Location: Munich, Germany
Contact:

Re: Intend to clean up and add function to SPI

Post by stevestrong » Fri Jul 28, 2017 10:23 pm

NO, Roger, a penalty would be only when using the first variant from my first post.

But the PR uses the second variant which does not slow down anything, just adds new function which increases the speed when accessing two times 8 bits.

User avatar
RogerClark
Posts: 7681
Joined: Mon Apr 27, 2015 10:36 am
Location: Melbourne, Australia
Contact:

Re: Intend to clean up and add function to SPI

Post by RogerClark » Fri Jul 28, 2017 10:25 pm

Ok

Thanks.

I will test during the weekend

Post Reply