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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Jan 20, 2015
Merged

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

merged 12 commits into from
Jan 20, 2015

Conversation

jnohlgard
Copy link
Member

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
jnohlgard reviewed Jan 16, 2015
View reviewed changes
drivers/nrf24l01p/nrf24l01p.c
@@ -189,12 +199,10 @@ int nrf24l01p_on(nrf24l01p_t *dev)
char read;
int status;

gpio_clear(dev->cs);
nrf24l01p_read_reg(dev, REG_CONFIG, &read);
hwtimer_spin(DELAY_CS_TOGGLE_TICKS);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@haukepetersen haukepetersen mentioned this pull request Jan 16, 2015
Closed
44 tasks
@jnohlgard
Copy link
Member Author

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

@jnohlgard jnohlgard mentioned this pull request Jan 16, 2015
Merged
@PeterKietzmann
Copy link
Member

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

@jnohlgard jnohlgard added Area: drivers Area: Device drivers 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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation swp-telematic-sose14 and removed swp-telematic-sose14 labels Jan 16, 2015
@jnohlgard
Copy link
Member Author

Updated according to #2290 (comment)

PeterKietzmann
PeterKietzmann reviewed Jan 19, 2015
View reviewed changes
drivers/nrf24l01p/nrf24l01p.c
gpio_clear(dev->cs);
hwtimer_spin(DELAY_CS_TOGGLE_TICKS);
status = spi_transfer_regs(dev->spi, CMD_R_RX_PAYLOAD, 0, answer, size);
hwtimer_spin(DELAY_CS_TOGGLE_TICKS);
gpio_set(dev->cs);
hwtimer_spin(DELAY_AFTER_FUNC_TICKS);
/* Release the bus for other threads. */
spi_release(spi-dev);
Copy link
Member

Choose a reason for hiding this comment

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

missing ">"

Copy link
Member

Choose a reason for hiding this comment

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

it must be spi_release(dev->spi);

@PeterKietzmann
Copy link
Member

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

@jnohlgard
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member Author

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

@PeterKietzmann
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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

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: Remove redundant gpio_clear/gpio_set.
a991d7c 
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]>
nrf24l01p: Remove superfluous delays in nrf24l01p_on, nrf24l01p_off.
b6d94d9 
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]>
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 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
@jnohlgard
Copy link
Member Author

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
Copy link
Member

I'm happy. ACK&go

PeterKietzmann added a commit that referenced this pull request Jan 20, 2015
@PeterKietzmann
Merge pull request #2317 from gebart/pr/spi-locking
0c30832 
drivers/periph/spi: Implement thread safety for all SPI devices
@PeterKietzmann PeterKietzmann merged commit 0c30832 into RIOT-OS:master Jan 20, 2015
@PeterKietzmann PeterKietzmann mentioned this pull request Jan 20, 2015
Merged
@jnohlgard jnohlgard deleted the pr/spi-locking branch February 27, 2015 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@PeterKietzmann PeterKietzmann

Labels
Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jnohlgard @PeterKietzmann @LudwigKnuepfer