Incorrect transfer of SPI packets with BitOrder LSBFIRST when count is >= 4

[gentilkiwi]

When using void transfer(void *buf, size_t count); of the SPI Class, with BitOrder LSBFIRST, group of 4 bytes are reversed.
It seems to be here because of the optimization to handle arrays 4 bytes by 4 bytes (then byte(s) left), but it reverse the whole DWORD, not only bits of the byte.

Captures

MSBFIRST - OK

(left is using void transfer(void *buf, size_t count); right is multiple transfer(uint8_t data);)

LSBFIRST - KO

(left is using void transfer(void *buf, size_t count); right is multiples transfer(uint8_t data);)

Remarks:

• Problem is not present on R3, only on R4 ;
• LSBFIRST is not very common, but NXP PN532 is using it by eg ;
• I'm not sure about this issue, it's too big to have been missed by real users more advanced than me - do not hesitate to close this issue if I'm totally wrong!

[gentilkiwi]

• Did not check on the receive side (but lots of chance it can be the same)

ping @RudolphRiedel

[RudolphRiedel]

I took a brief look at this and I have an idea what is causing it and how to fix it.

[RudolphRiedel]

I can confirm the issue, looks like so far nobody tried to use LSBFIRST with buffer transfers.
And well, that includes me, sorry.

I fixed it here and this is how I currently have it up and running, MSBFIRST:

LSBFIRST:

Note, I am using two decoders to not switch between readings, the top is configured for LSBFIRST, the bottom for MSBFIRST.

I changed line 447 of SPI.cpp from
_spi_ctrl.p_regs->SPDCR2_b.BYSW = 1;

to:
_spi_ctrl.p_regs->SPDCR2_b.BYSW = ~bit_order;

This one does a byte-swap for the 32 bit transfers that happen as optimization when doing buffered transfers.

And for the background, while I was happy to see that Renesas finally released new versions of the datasheet and user manual back in September 2023, the user-manual is at Rev.1.10 now, for the SPI they only fixed the issue that in register SPDCR the bit SPBYT was not documented.
The register SPDCR2 is still not mentioned in the user-manual.
Please fix @renesas.

[gentilkiwi]

Thank you for this quick & efficient analysis @RudolphRiedel !
I can confirm LSB is now ok :)

Will continue to test against real device.

BTW, any reason to use ~bit_order instead of !bit_order for a on/off value ?

[RudolphRiedel]

BTW, any reason to use ~bit_order instead of !bit_order for a on/off value ?

No, I just wanted to inverse the value after I saw that SPCMD_b[0].LSBF needed a 1 to activate LSBFIRST.
And bit_order is not a boolean, it is spi_bit_order_t which is a typedef for an enum, so this is of type int (at least).

I just checked, !bit_order does also work.
And what would be more confusing, "bitorder + 1" and "bitorder - 1" also do work. :-)

Now I am not sure what the most appropriate variant is, especially in the C++ context.

[per1234]

Thanks for your report @gentilkiwi and to @RudolphRiedel for your pull request.

Although it is very interesting, I would appreciate it if we can keep this discussion tightly focused on the original subject matter. Discussion of @gentilkiwi's project is off topic for this issue and for this repository.

You are welcome to continue the conversations on Arduino Forum:

https://forum.arduino.cc/