drivers/periph/spi: Implement thread safety for all SPI devices by jnohlgard · Pull Request #2317 · RIOT-OS/RIOT

jnohlgard · 2015-01-16T12:55:09Z

This implements #2314 for the SPI drivers. (edit: Except for STM32F4 which is implemented in #2290)
I added the generic implementation of SPI thread safety as proposed by @haukepetersen in #2290 to all CPU periph/spi implementations and made all drivers which are currently using periph/spi to acquire/release the bus when necessary.

I also found a mistake in the nrf24l01p driver, the CS line was cleared/set twice when turning on/off the device.

jnohlgard · 2015-01-16T12:58:07Z

drivers/nrf24l01p/nrf24l01p.c

Are these delays needed, or are they accidental left-overs from a previous refactoring?

@gebart, I assume they are not. They have been introduced in #1704 in cause of "inexplicable" timing errors. Just today we found one possible reason for this in #2315. I will test this soon but I have not time for this today.

@gebart yes these delays are superfluous. Would be nice if you could remove them.

jnohlgard · 2015-01-16T13:05:02Z

@haukepetersen I added you as author on the affected $CPU/periph/spi.c files since you made the actual implementation work.

PeterKietzmann · 2015-01-16T13:38:45Z

BTW: thanks for doing all this task requiring great diligence!

jnohlgard · 2015-01-16T13:59:47Z

Updated according to #2290 (comment)

PeterKietzmann · 2015-01-19T12:48:30Z

drivers/nrf24l01p/nrf24l01p.c

missing ">"

it must be spi_release(dev->spi);

PeterKietzmann · 2015-01-19T13:14:05Z

@gebart: except from the small changes in nrf24l01p.c I would ACK this PR

jnohlgard · 2015-01-19T15:31:51Z

@PeterKietzmann Fixed the typo according to your comment, squash and go? (once Travis is done)

Should I leave each individual component as a separate commit as now and only squash the small comment fixes or squash it all into one big commit?

PeterKietzmann · 2015-01-19T16:03:39Z

@gebart I would propose to squash the nrf24l01p-, cc110x- and at86rf231-commits but I'm not sure if it's necessary to keep all the other ones separately. Maybe @LudwigOrtmann ha an idea about that?

jnohlgard · 2015-01-19T16:08:28Z

I'd prefer to leave it at one commit per cpu, just for history clarity.

PeterKietzmann · 2015-01-19T16:12:13Z

Yep that's what was in my mind too. I was just to stupid to phrase. But still, I think you can squash nrf24l01p-, cc110x- and at86rf231-commits. Would like to hear another opinion, maybe from Ludwig?

LudwigKnuepfer · 2015-01-19T17:20:17Z

I completely agree with the atomicity of the commits not prefixed 'squash' ;)
However, I notice fluctuations in the naming of commits. Sometimes "Require" is used instead of "Acquire". Is that on purpose?

jnohlgard · 2015-01-19T17:51:09Z

@LudwigOrtmann Require was probably just a typo on my side. I will rewrite the message during squashing.

Signed-off-by: Joakim Gebart <[email protected]>

This change removes extra gpio_clear(dev->cs) before calling nrf24l01p_read_reg(), nrf24l01p_write_reg(). The GPIO handling is not necessary since nrf24l01p_{read,write}_reg() handle the CS pin internally. Signed-off-by: Joakim Gebart <[email protected]>

The delays were introduced in an attempt to fix "inexplicable timing errors", although the errors were in the SPI bus driver rather than the nrf24l01p driver. See also: - RIOT-OS#1704 - RIOT-OS#2315 Signed-off-by: Joakim Gebart <[email protected]>

Signed-off-by: Joakim Gebart <[email protected]>

jnohlgard · 2015-01-19T18:09:52Z

I think the removing of the extra delays in nrf24l01p shall remain as a stand alone commit for easier bisection in any future regressions.

@PeterKietzmann ACK if you are fine with the current commit line

PeterKietzmann · 2015-01-20T06:27:46Z

I'm happy. ACK&go

drivers/periph/spi: Implement thread safety for all SPI devices

jnohlgard reviewed Jan 16, 2015
View reviewed changes

haukepetersen mentioned this pull request Jan 16, 2015

drivers/periph: making bus drivers thread safe #2314

Closed

44 tasks

jnohlgard assigned PeterKietzmann Jan 16, 2015

jnohlgard mentioned this pull request Jan 16, 2015

drivers/periph/spi: added means to make SPI thread-safe #2290

Merged

jnohlgard force-pushed the pr/spi-locking branch from 560a9ac to 6b0fda6 Compare January 16, 2015 15:05

PeterKietzmann reviewed Jan 19, 2015
View reviewed changes

jnohlgard force-pushed the pr/spi-locking branch from 4518a51 to b7baf85 Compare January 19, 2015 16:06

jnohlgard force-pushed the pr/spi-locking branch from b7baf85 to 365e5b9 Compare January 19, 2015 18:00

Joakim Gebart added 4 commits January 19, 2015 19:05

sam3x8e: Basic implementation of spi_acquire(), spi_release()

b20f21f

Signed-off-by: Joakim Gebart <[email protected]>

samd21: Basic implementation of spi_acquire(), spi_release()

bec43f1

Signed-off-by: Joakim Gebart <[email protected]>

stm32f0: Basic implementation of spi_acquire(), spi_release()

98c88b0

Signed-off-by: Joakim Gebart <[email protected]>

stm32f1: Basic implementation of spi_acquire(), spi_release()

f64c54b

Signed-off-by: Joakim Gebart <[email protected]>

Joakim Gebart added 8 commits January 19, 2015 19:05

stm32f3: Basic implementation of spi_acquire(), spi_release()

3292987

Signed-off-by: Joakim Gebart <[email protected]>

stm32l1: Basic implementation of spi_acquire(), spi_release()

d68b4ef

Signed-off-by: Joakim Gebart <[email protected]>

at86rf231: Acquire exclusive access to SPI bus.

1f577b4

Signed-off-by: Joakim Gebart <[email protected]>

cc110x: Acquire exclusive access to SPI bus.

af1ddef

Signed-off-by: Joakim Gebart <[email protected]>

nrf24l01p: Acquire exclusive access to SPI bus.

c457cd9

Signed-off-by: Joakim Gebart <[email protected]>

tests/periph_spi: Lock bus for exclusive access when communicating.

dfb0d58

jnohlgard force-pushed the pr/spi-locking branch from 365e5b9 to dfb0d58 Compare January 19, 2015 18:05

jnohlgard removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Jan 19, 2015

PeterKietzmann added a commit that referenced this pull request Jan 20, 2015

Merge pull request #2317 from gebart/pr/spi-locking

0c30832

drivers/periph/spi: Implement thread safety for all SPI devices

PeterKietzmann merged commit 0c30832 into RIOT-OS:master Jan 20, 2015

PeterKietzmann mentioned this pull request Jan 20, 2015

drivers/nrf24l01p: fixes #2330

Merged

jnohlgard deleted the pr/spi-locking branch February 27, 2015 09:04

Conversation

jnohlgard commented Jan 16, 2015

Uh oh!

jnohlgard Jan 16, 2015

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Jan 16, 2015

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Jan 19, 2015

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jan 16, 2015

Uh oh!

PeterKietzmann commented Jan 16, 2015

Uh oh!

jnohlgard commented Jan 16, 2015

Uh oh!

PeterKietzmann Jan 19, 2015

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Jan 19, 2015

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Jan 19, 2015

Uh oh!

jnohlgard commented Jan 19, 2015

Uh oh!

PeterKietzmann commented Jan 19, 2015

Uh oh!

jnohlgard commented Jan 19, 2015

Uh oh!

PeterKietzmann commented Jan 19, 2015

Uh oh!

LudwigKnuepfer commented Jan 19, 2015

Uh oh!

jnohlgard commented Jan 19, 2015

Uh oh!

jnohlgard commented Jan 19, 2015

Uh oh!

PeterKietzmann commented Jan 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants