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

cpu/cc2538: fix spi_transfer_bytes() #13598

Merged
merged 2 commits into from
Mar 17, 2020

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Mar 9, 2020

Contribution description

In the in_buf == NULL case, spi_transfer_bytes() would only write DR for every out byte, but then loop reading DR only while SPI is busy.

This is not correct, we have to read as many data bytes as we write, otherwise data bytes remain in the FiFo and will corrupt subsequent reads.

While at it, I also took the chance to clean up spi_transfer_bytes() by factoring out DR access into a helper function.

Testing procedure

Run @guyyst's tests application from #12128 (comment) on the openmote-b.

Before this patch, the driver would sometimes read both interrupt status registers as 0 (or some other weird value) and in turn not service the interrupt.

With this the test is running smoothly.

Issues/PRs references

The issue was discovered while testing the at86rf215 driver on the openmote-b.

benpicco added 2 commits March 9, 2020 16:22
We have to read the DR for every byte that we write.
Just reading DR while SPI is busy in a loop can lead to bytes being
left in the fifo, corrupting subsequent reads.
Use a common helper function to read/write the data register.
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: cpu Area: CPU/MCU ports labels Mar 9, 2020
@benpicco benpicco requested a review from fjmolinas March 9, 2020 15:44
@benpicco benpicco requested a review from MrKevinWeiss as a code owner March 9, 2020 15:44
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 9, 2020
@fjmolinas
Copy link
Contributor

Had to go back though some of my SPI basics, but change makes sense (was confused by the fact that the Rx_FIFO and TX_FiFO need to behave like a shift register for SPI). I'll test ASAP.

@fjmolinas
Copy link
Contributor

I'm trying to replicate the failure using the mentioned example, its been going for 1hour and no failure yet...

@fjmolinas fjmolinas self-assigned this Mar 10, 2020
@fjmolinas
Copy link
Contributor

I'm trying to replicate the failure using the mentioned example, its been going for 1hour and no failure yet...

Argh, I'm unable to reproduce the issue now... Since the change still makes sense, I'll do a no-regression test.

@fjmolinas
Copy link
Contributor

Argh, I'm unable to reproduce the issue now... Since the change still makes sense, I'll do a no-regression test.

Otherwise maybe @guyyst can verify it fixes the issue for him?

@guyyst
Copy link

guyyst commented Mar 10, 2020

I'll have access to our OpenMotes towards the end of the week. I'll try out the fix and report back then.

@guyyst
Copy link

guyyst commented Mar 12, 2020

Weird, I'll have to echo @fjmolinas findings. I tried the example on 2.4-GHz and Sub-GHz with and without this patch and observed the same behaviour. Neither of them stopped receiving even after 5+ minutes.

My only guess is that some other commit to #12128 since the last time I tried this in November already fixed the issue.

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

I've had a lot of trouble reproducing the issue. I had it come back a couple of time to the disappear. None the less I haven't seen it when this change has been. I'm going to ACK this because:

  • The change itself makes sense
  • I have tested there is no regression
  • I have not seen the bug when this PR was in
  • @benpicco provided offline more info on how he found the fix:

I've never seen the issue on the sam0 boards, so I checked what else it might be. And the SPI implementation on cc2538 was broken before (#12336), so I checked if there is something strange still
and yea, reading & writing bytes to and from SPI should always work the same no matter what values the in or out buffers have, so I fixed that
afterwards I would not see the issue again
(I also printed the values I read from the interrupt flags register and they would make no sense, e.g I would get an interrupt and both registers would read 0)

@fjmolinas fjmolinas merged commit d77ecc1 into RIOT-OS:master Mar 17, 2020
@benpicco benpicco deleted the cpu/cc2538-spi-fix branch March 17, 2020 16:56
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants