periph/i2c: return values of i2c_aquire and i2c_release

[gschorcht]

Description

I have read very carefully all related issues #6577 and PRs #6575, #6576 to figure out the answer to the following question:

For which purpose do the i2c_acquire and i2c_release function still have a return value. In fact they simply lock and unlock a mutex on all platforms. Since i2c_acquire blocks until the mutex can be locked, the function will not fail.

Some implementations check whether the device parameter is valid, but there is no reason to have a return parameter for it. The recommended way is to check the device parameter is using assert. In all other cases, all implementations simply return 0.

Disadvantages of the Current API

Since the API claims that the return value might be -1 in case of error, the return value would have to be checked.

The reality looks completely different. The return value is checked in less than 10 % (19 of 227) of i2c_acquire function calls and in 0% (0 of 380) i2c_release function calls.

Proposal

It is quite tedious to use something like the following over and over again just because the API defines return values, although the function can not cause an error.

if (i2c_aquire(I2C_DEV(0)) != 0) {
   ...
}

The return values should be removed. Otherwise, all drivers would need a rework for the case that the function can fail.

[jnohlgard]

I agree, the way we use the api and the way it is implemented makes the return value an unnecessary complexity.

[dylad]

For which purpose do the i2c_acquire and i2c_release function still have a return value. In fact they simply lock and unlock a mutex on all platforms. Since i2c_acquire blocks until the mutex can be locked, the function will not fail.

This is true, nevertheless, when we started the I2C rework, we were thinking about expanding stuff within those functions. So that they can also setup/disable peripheral clock (to save power), setup DMA or CPU specific actions.
I think this is why we kept a return value.

But I am not opposed to remove it :)

[gschorcht]

So that they can also setup/disable peripheral clock (to save power), setup DMA or CPU specific actions.

Indeed, some CPUs are doing this, but at least for the moment without error handling.

[dylad]

Indeed, some CPUs are doing this, but at least for the moment without error handling.

This is mostly because we didn't put much effort on this specific part during the refactoring because we already had enough problems. So we kept it as it was in master at this time but added the return value for potential future use.
I wanted to use it on SAM0 I2C driver but never found time for this.

[MrKevinWeiss]

Although I am not strongly opposed to removing it as well, however, I tend to think that, since it isn't introducing bugs, it allows better future proofing, and it doesn't add that much to code size, it would be better you spend time on more bug fixes, testing, board support, features, etc.

[gschorcht]

@MrKevinWeiss A return value that is ignored for 90% of the calls has no value. If there is a return value, drivers should not rely on the knowledge that the return value is not set anyway by the called function, since this can be changed in the future. That means the drivers would have to check that. However, checking the return value increases the code size, which in turn makes no sense for a return value that is not set by the called function.

IMHO, it is a consistency problem. If an API defines a return value, it should be checked to be fault tolerant. If code size is important, a function should only have a return value if something can fail.

[dylad]

@gschorcht is right here. The question that we must answer is: Should we use these return values for the purpose we want and thus increase code size ?
I think dropping these return values makes sense for the sake of simplicity. Even if we add more features to the acquire/release like setting clock, this should not fail (if the configuration is correct). This way, assertions should be enough for developing and testing (with DEVELPHELP=1).

We may reconsider this assumption later if we develop sophisticate stuff but I think we're far from it right now.

[kaspar030]

+1 for removing the return value. The parameter is, by definition, only allowed to be used after having been successfully initialized through i2c_init(). If the latter returns an error, the dev argument is invalid. Otherwise, it doesn't need to be checked. Hence, the failure case will never hit in code that correctly initializes i2c.

[smlng]

I'm against removing it, and the rational wit i2c_init does not hold (@kaspar030) when considering the following:

1. i2c_init is (supposed to be) called by periph_init
2. a driver (e.g. sensor) requiring I2C should not (IIRC) call i2c_init (anymore) as per 1. and also because it could interfere with other devices using the same bus
3. considering 1. and 2. a driver cannot deduce that the I2C bus it wants to use, that is acquire, is valid

hence, the return value makes sense, specifically considering that the (future, far away) goal is to allow developers to link against RIOT (libs) without recompiling core parts, such as CPU and periph driver implementations.

True is, that many driver do not verify the return code of i2c_acquire and simply work on with the rest of their code, which is wrong according to the API spec (at least to me).

[kaspar030]

1. `i2c_init` is (supposed to be) called by `periph_init`

I see your point. I'm still against adding the checks, they're completely waste of cycles after the first call, and noone checks them anyways.

Maybe introducing static inline int i2c_is_dev_valid(i2c_t dev) return (dev < I2C_NUMOF); would make it possible to be explicit about the check within a driver's init() function?

[MrKevinWeiss]

I think I like removing the return value so void acquire(i2c_t dev) and void release(i2c_t dev) and adding asserts along with an optional static inline int i2c_is_dev_valid(i2c_t dev) return (dev < I2C_NUMOF);

It can be the smallest footprint and allows for the most versatility.

[maribu]

Let's at least remove the return value of i2c_release(). There is really no error handling possible, if returning a resource fails.

[dylad]

Can we consider we reach an agreement here and move forward ?

[maribu]

+1 for dropping the return value of the release function. For acquire, it would be more consisted with the SPI API to keep the return value.

#12063 would only need an ACK to get this in master.

Btw: There was so far no opposition to dropping tge return value of the release call, so this is uncontroversial.

[gschorcht]

+1 I'm fine with dropping the return value from i2c_aquire.

But, I'm still convinced that we should also drop the return value from i2c_aquire for the following reasons:

• As I understand, i2c_aquire can't fail if i2c_init doesn't fail, at least at the moment.
• If i2c_aquire could fail in future, e.g., because of additional configuration, the problem should already happen during the development phase which could be handled by an assertion.
• The return value isn't checked by 90 % of the drivers.
• Checking the return value by all drivers would increase the complexity and the code size.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.