Skip to content

SPI::transferBytes pending changes #4967

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

Closed
6 tasks done
devyte opened this issue Jul 27, 2018 · 2 comments
Closed
6 tasks done

SPI::transferBytes pending changes #4967

devyte opened this issue Jul 27, 2018 · 2 comments

Comments

@devyte
Copy link
Collaborator

devyte commented Jul 27, 2018

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: all
  • Core Version: latest git
  • Development Env: all
  • Operating System: all

Settings in IDE

  • Module: all
  • Flash Mode: all
  • Flash Size: all
  • lwip Variant: all
  • Reset Method: all
  • Flash Frequency: 40Mhz
  • CPU Frequency: 80Mhz|160MHz
  • Upload Using: SERIAL
  • Upload Speed: 115200

Problem Description

This issue is meant to track pending changes to SPI::transferBytes, specifically a missing assert and some api optimizations.
Ref: #4925 #2677 .

edit 19/01/22 d-a-v: The API has to be clarified. transfer(buffer) and transferBytes(buffer) have the same signature, the first accepts unaligned buffer, not the second.

@earlephilhower
Copy link
Collaborator

I would suggest supporting a simple alignment check and fix for transferBytes() because I believe it used to support unaligned transfers and libs like SDFat were using it that way. The simple fixup should decompse to an "AND" of the address and a jump-on-zero (2 or 3 insns total) addition to transferBytes() and make things easier for existing older libs.

@earlephilhower
Copy link
Collaborator

Specifically, transferBytes needs to handle misaligned pointers and non-by-4 lengths.
greiman/SdFat#125

@d-a-v d-a-v mentioned this issue Feb 1, 2019
6 tasks
earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Feb 1, 2019
Fixes esp8266#4967

Support any alignment of input and output pointers and transfer lengths
in SPI::transferBytes.  Use 32-bit transfers and FIFO as much as
possible.
devyte pushed a commit that referenced this issue Feb 3, 2019
* Allow unaligned input/output to SPI::transferBytes

Fixes #4967

Support any alignment of input and output pointers and transfer lengths
in SPI::transferBytes.  Use 32-bit transfers and FIFO as much as
possible.

* Refactor misaligned transfer, avoid RMW to FIFO

The SPI FIFO can't properly do RMW (i.e. bytewise updates) because when
you read the FIFO you are actually reading the SPI read data, not what
was written into the write FIFO.

Refactor the transferBytes to take account of this.  For aligned input
and outputs, perform as before (but handle non-x4 sizes properly).  For
misaligned inputs, if it's unidirectional then do bytewise until the
direction data pointer is aligned and then do 32b accesses.  Fod
bidirectional and misaligned inputs, copy the output data to an aligned
buffer, do the transfer, then copy the read back data from temp aligned
buffer to the real input buffer.

* Fix comments, clean condition checks, save stack

Add more comments and adjust naming to be more informative in
transferBytes_ and *aligned_.  Save 64bytes of stack in double
misaligned case.

* Optimize misaligned transfers, reduce code size

On any misaligned input or output, always use a temp buffer.  No need
for special casing and bytewise ::transfer().  This should be faster as
bytewise ::transfer involves a significant number of IO register
accesses and setup.  Thanks to @devyte for the suggestion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants