Skip to content

cpu: fix returns of i2c_acquire and i2c_release#10856

Closed
smlng wants to merge 7 commits intoRIOT-OS:masterfrom
smlng:pr/i2c/returns
Closed

cpu: fix returns of i2c_acquire and i2c_release#10856
smlng wants to merge 7 commits intoRIOT-OS:masterfrom
smlng:pr/i2c/returns

Conversation

@smlng
Copy link
Member

@smlng smlng commented Jan 24, 2019

Contribution description

This PR replaces asserts with if statements in periph/i2c for all CPUs to return proper error codes according to the API spec. The rational behind this is that (theoretically) one could write a driver and simply link against RIOT without recompiling the periph/i2c, hence asserts will not work but according to the API specs acquire and release should return -1 on error, which in this case mostly means: wrong device

Testing procedure

run tests/periph_i2c (or any drivers requiring I2C) for different boards to match all CPUs, it should still work.

Issues/PRs references

@smlng smlng added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Jan 24, 2019
@smlng smlng requested a review from MrKevinWeiss January 24, 2019 08:55
@dylad
Copy link
Member

dylad commented Jan 24, 2019

@MrKevinWeiss @gschorcht @gebart I think this is related to #10673 ;)
I think it is time to take a decision here.

@aabadie
Copy link
Contributor

aabadie commented Jan 24, 2019

Maybe link to #10673. You could give your opinion there as well ?

@kaspar030
Copy link
Contributor

NACK, we've had this discussion many times. periph device args get checked on init() only.
It is forbidden to call any i2c function with a device value that has not been checked through init(). The checks are not necessary and bloat the code. Any code that triggers the fail case is buggy from the start. Hence asserts must be enough.

Adding this error here would basically force any user to actually check for the error, which is just overkill for embedded use.

@gschorcht
Copy link
Contributor

@MrKevinWeiss @gschorcht @gebart I think this is related to #10673 ;)
I think it is time to take a decision here.

Indeed. As stated in #10673, 95 % of our drivers do not check these return values. An assert might be better than nothing.

BTW, I would rather add a further assert to check that these functions are not called from interrupt context to avoid problems as described in issue #10488 and in PR #10430 which are tried to be solved by PR #10555.

@MrKevinWeiss
Copy link
Contributor

Any movement on this front? Can we agree asserts are enough?

@tcschmidt
Copy link
Member

This almost looks like a rough consensus to me: Those who disagree closing this PR (+related) should speak up asap.

@smlng
Copy link
Member Author

smlng commented May 23, 2019

closing ... as discussed, this should be addressed differently.

@smlng smlng closed this May 23, 2019
@smlng smlng deleted the pr/i2c/returns branch May 23, 2019 10:04
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 Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants