Cleanup SPI constructor and destructor

[deepikabhavnani]

Description

The SPI class has a member "static SPI *_owner;" to indicate need to set SPI parameters or not. Clear the _owner in constructor to allow SPI to initialize everytime a object is constructed.

Resolves: #4969

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

[kjbracey]

This constructor is a bit weird. Why is the constructor doing the _acquire() at all? We're not doing any operation now, so why try to claim the bus to set the format?

And, actually, doing the _acquire without the lock isn't safe - the comment is incorrect, as the lock is on a static thing, not just this object. This means construction of an SPI object isn't threadsafe versus an ongoing SPI operation.

So I think the _acquire() should be removed from the constructor.

The constructor acquire was added in b09a87f, but that replaced some direct configuration which shouldn't have been happening in the first place.

The problem in #4969 can still happen whenever the first _acquire occurs on write or whatever, but that particular problem is more clearly addressed by logic in the destructor, as the original reporter suggested. However I think it should be conditional:

if (_owner == this) {
    _owner = NULL;
} 

That's the actual point at which _owner becomes a stale pointer.

[deepikabhavnani]

The constructor acquire was added in b09a87f, but that replaced some direct configuration which shouldn't have been happening in the first place.

As I understand your comment even that direct configuration of format / frequency should not be done since we are not doing any operation.
As per the commit message of b09a87f scenario was

SPI spi1(...);      // _owner is NULL, peripheral config is 1
spi1.transfer(...); // _owner is 1, config is 1
SPI spi2(...);      // _owner is 1, config is 2
spi1.transfer(...)  // 1 thinks it still owns peripheral, doesn't reconfigure

Which should be

SPI spi1(...);      // _owner is NULL, peripheral config is NULL
spi1.transfer(...); // _owner is 1, config is 1
SPI spi2(...);      // _owner is 1, config is 1 (Dont change config in constructor)
spi1.transfer(...)  // 1 thinks it still owns peripheral, doesn't reconfigure (No need)

?

[deepikabhavnani]

The problem in #4969 can still happen whenever the first _acquire occurs on write or whatever

I can add a destructor, but it will not be actually freeing SPI bus (till new SPI HAL is in place).

[kjbracey]

Yes, your sequence for the b09a87f scenario looks correct to me. I think taking the acquire out of the constructor is also a Good Thing because it will help ensure (a little) that the other methods are actually doing their job and acquiring properly.

[kjbracey]

GitHub comment/title should be updated with new commit message.

[0xc0170]

This simple fix is now ready for CI

cc @ithinuel (already requested for review).

[NirSonnenschein]

/morph build

[mbed-ci]

Build : SUCCESS

Build number : 3227
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8090/

Triggering tests

/morph test
/morph mbed2-build

[mbed-ci]

Test : SUCCESS

Build number : 3030
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8090/3030

[NirSonnenschein]

/morph export-build

[mbed-ci]

Exporter Build : FAILURE

Build number : 2813
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8090/

[NirSonnenschein]

/morph export-build

[mbed-ci]

Exporter Build : FAILURE

Build number : 2817
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8090/

[deepikabhavnani]

/morph export-build

[mbed-ci]

Exporter Build : SUCCESS

Build number : 2820
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8090/