spi_api bugfix - many targets

[bmcdonnell-ionx]

NB: NOT FULLY TESTED; REVIEW NEEDED.

Description

In hal/spi_api.h, the spi_slave_receive() function reportedly "Check[s] if a value is available to read". In spi_api.c of many targets, a return value of 1 (true) from this function is dependent on the SPI peripheral being "not busy". I think this is incorrect logic. This PR attempts to fix the issue.

Briefly, my reasoning is captured in the commit comment:

Don't wait for "not busy" in spi_slave_receive(). Busy-ness is with respect to SPI bus activity, but we can read whenever there is something in the receive buffer, regardless of whether bus activity is (still) occurring.

Background / Context

In a project, I'm working on adding interrupt-based asynchronous SPI slave code, on an Embedded Artists' LPC4088 QuickStart Board (which uses an NXP LPC4088 micro). (I've also slowed down the micro clocks.) In the interrupt handler, I'm basically doing this:

      while (spi_slave_receive(&spi))
      {
         RdRingBuf.push(spi_slave_read(&spi));
      }

      SPIHandlingThread.signal_set(SPI_DATA_RCVD_FLAG);

With the current implementation, that makes for a race condition in the interrupt handler. AFAICT, sometimes it'll exit the handler early - potentially without even grabbing any data - because the SPI bus happens to have activity at the moment, even though I could still read from the FIFO. Consequently, it intermittently misses data.

With this patch, it seems to be working reliably on my target.

Evidence

For a bit of supporting evidence that this is true across platforms, see for instance section 28.3.7 of ST's RM0090.pdf (for STM32F405, etc.), which says,

Note: Do not use the BSY flag to handle each data transmission or reception. It is better to use the TXE and RXNE flags instead.

EDIT: For the NXP LPC408x/407x, see section 21.6 of UM10562.pdf.

Change details

In this PR, I took the liberty of making the changes that I think need to be made on all the targets I could find. I have only personally tested on the LPC4088. I'm hoping the Mbed OS team can pick up verification from here.

For LPC15XX and LPC81X, I removed the "busy" test from spi_slave_receive() as I did for the others. However, I think the spi_busy() return value is determined incorrectly. (I did not change that.) Currently, it is based on the "Receive Overrun" status. (See section 25.6.3 of UM10736.pdf for LPC15XX.) I don't think that the overflow status ("you missed data") is related to "busy-ness".

EDIT: The change I'm least sure of is TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A. It's the most different from the others, and I couldn't find the corresponding documentation easily.

Pull request type

• Fix
• Refactor
• New target
• Feature
• Breaking change

[0xc0170]

Looking at the SPISlave::receive() , it defines what is the intention here: Polls the SPI to see if data has been received

This will be a good candidate for upcoming SPI spec (to define well the API, tests and then targets implementation).

[bmcdonnell-ionx]

@0xc0170, related:

This will be a good candidate for upcoming SPI spec (to define well the API, tests and then targets implementation).

I plan to make a PR for my enhanced SPISlave driver with interrupt handling. I only implemented the spi_api parts for my target (NXP LPC4088), but I modeled it after the CAN driver, so it should be straight-forward (if not easy) to add support for other targets. Maybe the trickiest part will be dealing with devices' different SPI module interrupts, but I've got an idea for that...

Anyway, in light of the "upcoming SPI spec" you mentioned, what's the best way forward? Should I just do the PR and we go from there?

[0xc0170]

Anyway, in light of the "upcoming SPI spec" you mentioned, what's the best way forward? Should I just do the PR and we go from there?

HAL extensions are not that simple. We currently are improving few, there are multiple feature branches with specifications, tests and implementation (any change to HAL should have wide support in targets).

In this PR, I took the liberty of making the changes that I think need to be made on all the targets I could find. I have only personally tested on the LPC4088. I'm hoping the Mbed OS team can pick up verification from here.

Feedback like this helps us, appreciated. Will do pick it up. Thinking if this shall be parked (closed) and will be tracked here so we will review during SPI specs

[bmcdonnell-ionx]

@0xc0170

Anyway, in light of the "upcoming SPI spec" you mentioned, what's the best way forward? Should I just do the PR and we go from there?

HAL extensions are not that simple. We currently are improving few, there are multiple feature branches with specifications, tests and implementation (any change to HAL should have wide support in targets).

I don't see how this answers my questions, but instead of doing so here, it would probably be better if you did so on #6504, where I give more details.

Feedback like this helps us, appreciated. Will do pick it up. Thinking if this shall be parked (closed) and will be tracked here so we will review during SPI specs

From what I gather, it's customary to close PRs by merging, and not before (unless rejected). IMO, it would be preferable to keep this open until then, to reflect its status.

[cmonr]

@bmcdonnell-ionx What'll likely end up happening is that this PR will cease being updated (either via comments or lack of commits), at which point we'll have to close it due to inactivity.

Closed PRs however can be reopened at a later time when updates are available.

[cmonr]

Closing due to inactivity.

Related, it looks like changes are coming to the SPI API in this PR: #6782

[bmcdonnell-ionx]

@cmonr, @0xc0170, @bulislaw,

Was abandoning this really the right course of action? I still think this is a change that should be made to the current implementation. What's the ETA on the SPI enhancements? (See #6782, #7671)

I'm not sure what the testing effort would involve on your end for this PR, though.

[cmonr]

It looks like #7671 has settled down and is ready to progress. Unfortunately, it still needs to go through CI since we don't have any docs-only-pr exceptions (we'll be looking into it though), but it'll be merged after that.

As for the timeline after that merge, @bulislaw and/or @ithinuel would have a better grasp on that.