Better RTOS integration for mbed-drivers

[anpilog]

Hi,

As @0xc0170 suggested I created this issue as place to discuss RTOS integration to drivers (SPI, I2C, etc).

lock()/unlock() methods should address common user needs in better way.

As one of the option it could be:

class SPI {
public:
    virtual bool lock(uint32_t millisec=osWaitForever) {
        return _mutex->lock(millisec) == osOK;
    }
}

IMHO those lock/unlock wrappers around mutex should be used only by user.
Current approach when each driver method user lock()/unlock() covers only integrity of register access.
But in case of SPI, for example, more important to cover a transaction.
And so far user has to use separate mutex for this.

[0xc0170]

cc @c1728p9 @geky

[geky]

This makes sense to me for bus-based transactions. The most appropriate locking would be over specific peripherals but from what I understand that information isn't exposed yet?

Also just exposing lock/unlock methods is a good idea to hide mutex details.

[neilt6]

@anpilog - The underlying mutex implementation in RTX supports nesting, so if the user explicitly calls lock(), the driver will remain locked across all method calls until the user explicitly calls unlock(). If the driver methods didn't call lock()/unlock(), legacy code that didn't explicitly lock the driver would still be able to wreak havoc.

[geky]

@neilt6, that's a good point. Though could that problem be avoided by locking the underlying bus before entering the old driver?

[neilt6]

@geky - Only if the user had another driver instance that they could lock()/unlock() the bus from before using the legacy code. Barring that, all legacy code that uses internal driver instances would have to be re-written to prevent rampant concurrency issues. In my opinion, this only works if locking a driver truly locks it for all users, and that is more or less the way it works right now.

[geky]

Ah I see. Well as you point out the recursive nature of the mutex will let us block on legacy transactions.

An aside, I agree with @anpilog that we should add the millisecond argument to match the Mutex api.

[anpilog]

@neilt6 I don't understand how your point related to this question.
Could you give an code example?

[ciarmcom]

ARM Internal Ref: IOTMORF-98

[neilt6]

@anpilog - You suggested that lock()/unlock() should only be used by the user, since using them on a per-method level only protects register accesses and not the entire transaction. This is not the case, since the underlying mutex implementation supports nesting (i.e. it counts how many times it's been locked).

Example

//Shared SPI bus code on thread 1
spi1.lock();        //Mutex is now "locked", and at level 1
spi1.write(0xBE);   //Mutex was internally "locked" again, bumping it to level 2, then "unlocked" back to level 1
spi1.write(0xEF);   //Mutex was internally "locked" again, bumping it to level 2, then "unlocked" back to level 1
spi1.unlock();      //Mutex was at level 1, so now it is truly "unlocked"

//Shared SPI bus code on thread 2
//NOTE: This is some legacy code that hasn't been updated to use lock()/unlock() yet
spi2.write(0x12);   //Internal calls to lock()/unlock() ensures the transaction on thread 1 is protected

[anpilog]

Had to go down to rt_Mutex.c to get it.
Looks good to me!