Skip to content

drivers/periph_spi: let spi_acquire return void#15902

Merged
fjmolinas merged 6 commits intoRIOT-OS:masterfrom
maribu:spi-api-change-1
Sep 2, 2021
Merged

drivers/periph_spi: let spi_acquire return void#15902
fjmolinas merged 6 commits intoRIOT-OS:masterfrom
maribu:spi-api-change-1

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Feb 1, 2021 •
edited
Loading

Contribution description

  • Changed signature from int spi_acquire(...) to void spi_acquire(...)
    • Most SPI implementations and 99.9% of all users treat it like this anyway
    • By adding assert()s to verify parameters, this actually increases error checking compared to before, where both SPI implementations and user mostly didn't care about errors
  • Updated soft_spi to match the periph_spi API
  • Updated implementations
  • Updated the few users, which actually checked the return value
    • Most checked the return value only during <foo>_init(), but afterwards never again
    • This is updated to add a pair of spi_acquire() and spi_release() if and only if NDEBUG is not defined.
    • The reasoning is: If assert()ions are disabled anyway, we won't trigger them no matter how hard we try :-)

Testing procedure

This should be mostly unscary. However, without assert()ions enabled, timing behavior changes a bit during initilization, as a useless pair of spi_acquire() and spi_release() is simply dropped.

Issues/PRs references

Depends on and includes:

@maribu maribu added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers Area: cpu Area: CPU/MCU ports Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Feb 1, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 1, 2021

Let's see if the CI finds any implementations / users that I missed.

Beware: While the WIP label is still present, I will squash without further notice ;-) I'll drop the WIP label once the CI is green.

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 1, 2021
@maribu maribu changed the title drivers/periph_spi: Let spi_acquire return void drivers/periph_spi: let spi_acquire return void Feb 1, 2021
@maribu maribu mentioned this pull request Feb 1, 2021
Closed
@maribu maribu force-pushed the spi-api-change-1 branch 4 times, most recently from f76eba8 to a8401a6 Compare February 2, 2021 07:25
@maribu maribu force-pushed the spi-api-change-1 branch 2 times, most recently from 0645dc9 to 64e2878 Compare February 2, 2021 07:48
@maribu maribu added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Feb 2, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 2, 2021

The remaining failed build is an CI hiccup, so this is no longer WIP.

@maribu maribu removed Area: arduino API Area: Arduino wrapper API Area: sys Area: System labels Aug 6, 2021
@github-actions github-actions bot added Area: arduino API Area: Arduino wrapper API Area: sys Area: System Area: tests Area: tests and testing framework labels Sep 1, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Sep 1, 2021

Cherry-picked review fixes that address @hugueslarrive's review in #15904 into this PR as well

dylad
dylad reviewed Sep 1, 2021
View reviewed changes
dylad
dylad approved these changes Sep 1, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

LGTM.
Please squash !

maribu
maribu commented Sep 2, 2021
View reviewed changes
cpu/atmega_common/periph/spi.c
int spi_acquire(spi_t bus, spi_cs_t cs, spi_mode_t mode, spi_clk_t clk)
void spi_acquire(spi_t bus, spi_cs_t cs, spi_mode_t mode, spi_clk_t clk)
{
(void)bus;
Copy link
Copy Markdown
Member Author

@maribu maribu Sep 2, 2021

Choose a reason for hiding this comment

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

Note that if an argument is used by an assert() only, it will be unused once the assert is disabled via NDEBUG. The reason is that assert() is (as required by the C standard) implement as a preprocessor macro.

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Sep 2, 2021

Go!

@fjmolinas fjmolinas merged commit a1cbcc9 into RIOT-OS:master Sep 2, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Sep 2, 2021

Thanks!

@maribu maribu deleted the spi-api-change-1 branch September 2, 2021 07:02
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 2, 2021
@hugueslarrive
Remove duplicated includes introduced in RIOT-OS#15902
480aba9
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 2, 2021
@hugueslarrive
drivers/periph_spi: remove duplicated includes introduced in RIOT-OS#…
f292cfc 
…15902
dylad added a commit that referenced this pull request Sep 2, 2021
@dylad
Merge pull request #16798 from hugueslarrive/15902_fixes
6e1bf61 
Remove duplicated includes introduced in #15902
hugueslarrive
hugueslarrive reviewed Sep 4, 2021
View reviewed changes
@hugueslarrive hugueslarrive mentioned this pull request Sep 4, 2021
Open
38 tasks
@maribu maribu mentioned this pull request Sep 6, 2021
Merged
@MarcelStenzel MarcelStenzel mentioned this pull request Sep 6, 2021
Merged
@benpicco benpicco mentioned this pull request Nov 17, 2021
Merged
@gschorcht
Copy link
Copy Markdown
Contributor

gschorcht commented Nov 24, 2021

Shouldn't we do the same for i2c_acquire? All implementations just return 0 with only one exception

RIOT/cpu/cc2538/periph/i2c.c

Lines 234 to 242 in 97758b8

int i2c_acquire(i2c_t dev)
{
DEBUG("%s\n", __FUNCTION__);
if (dev < I2C_NUMOF) {
mutex_lock(&lock);
return 0;
}
return -1;
}
which is a typical case for an assert. This was already the topic of a discussion in issue #10673, but after we removed the return value of spi_acquire, there is no argument to keep it for i2c_acquire.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 24, 2021

I totally agree!

@gschorcht
Copy link
Copy Markdown
Contributor

gschorcht commented Nov 25, 2021

Ok, I will try to provide a PR in next weeks.

@gschorcht gschorcht mentioned this pull request Nov 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@dylad dylad dylad approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@gschorcht gschorcht Awaiting requested review from gschorcht gschorcht is a code owner

@daniel-k daniel-k Awaiting requested review from daniel-k

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@smlng smlng Awaiting requested review from smlng

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@basilfx basilfx Awaiting requested review from basilfx basilfx is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@keestux keestux Awaiting requested review from keestux keestux is a code owner

@kYc0o kYc0o Awaiting requested review from kYc0o kYc0o is a code owner

@roberthartung roberthartung Awaiting requested review from roberthartung

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

+1 more reviewer

@hugueslarrive hugueslarrive hugueslarrive left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: arduino API Area: Arduino wrapper API Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: AVR Platform: This PR/issue effects AVR-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: MSP Platform: This PR/issue effects MSP-based platforms Platform: native Platform: This PR/issue effects the native platform Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK

Projects

None yet

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

7 participants

@maribu @MrKevinWeiss @benpicco @fjmolinas @gschorcht @dylad @hugueslarrive