'global' i2c mutex across all i2c buses causing contention

Mbed OS
1

Hi,

The problem I have is that 3 i2c devices, each on its own i2c bus and therefore instance, are running into contention when being accessed from 3 different threads (1 thread per i2c bus instance)

Although the i2c the driver is thread safe upon digging deeper into the driver I’ve found that there is a mutex defined as a static member in the class which looks to be the cause of the contention, please see below for more background info.

My question is, has any one got any tips on how to work around this or have I missed something or am I doing something wrong?

Thanks in advance,
Wayne

Background:

For the code in question please see line 240 here.

I have an STM32F4 device where we are using all 3 of it’s i2c buses, using mbedOS 6.9.0

On one bus is a very slow device that takes, say 200+ ms, to update (e.g. return from an i2c.write)
On the two other i2c buses is a single sensor of the same type. It’s not possible to change the i2c address on this particular sensor and it’s particularly sensitive to being read ‘often’.

Each device is being written and read from it’s own thread.

2

I’m wondering if using the low-level C i2c_api directly and managing locking myself is the way togo?

3

In this case, it looks like rolling your own is the way to go. That single lock for any I2C traffic looks wrong to me. It should be a lock per bus.

4

Hi, thanks, yes, a single lock across all buses does indeed seem wrong.

5

Hello,

methods lock() and unlock() are declared as public virtual. So if I’m not wrong it can be overridden and you can implement your own mutex system and by this way you will workaround the Singleton mutex.

So you can place something like this into your main.cpp

class myI2C : public I2C {
public:
    myI2C(PinName sda_pin, PinName scl_pin) : I2C(sda_pin, scl_pin) { }
    void lock() {
        // your own implementation or empty
    }
    void unlock() { 
        // your own implementation or empty
    }
};

BR, Jan

6

Hiya,

Thank you for the info, much appreciated.

All the best
Wayne

7

There is a shared state variable _owner that is used to determine if a call the HAL API i2c_frequency is “needed”. The function sets all i2c bus frequencies, so needs to be skipped or managed as it could set the frequency of another bus mid transaction.

class myI2C: public I2C {
  public: myI2C(PinName sda_pin, PinName scl_pin): I2C(sda_pin, scl_pin) {}
  void lock() {
    // your own implementation or empty
  }
  void unlock() {
    // your own implementation or empty
  }
  void aquire() {
    // avoid the call to i2c_frequency() or manage it yourself 
  }
};
8

Did you tried that? Or where did you saw that?

Mbed c++ APIs are based on C HAL drivers. Functions of HAL drivers, what I saw, usually contain an object variable as a parameter. This object usually hold a C struct of necessary info about current peripheral. By this way it avoid unwanted changes on unwanted peripherals.
Of course it is my understanding and maybe I am wrong.

So according to my understanding above is aquire method save to use, respectively there is no need to solve it .

Method aquire() is declared as protected void so it can not be override, only called in a subclass(derived class) of that class.

From my point of view the the Mbed::I2C API is designed for multiple devices on one single bus which is standard usage. That is reason for these things like Singleton mutex and the aquire() method.

BR, Jan

9

Hi,

I’ve tried it, take a look at the i2c cpp file.

In this case it’s not a low level HAL i2c object, the _owner member variable is just a pointer to this, but as _owner is static it is shared across all instances its value will vary from bus instance to bus instance and in the case of my target what acquire does is not ideal…

Take a look at the method implementation:

void I2C::aquire()
{
    lock();
    if (_owner != this) {
        i2c_frequency(&_i2c, _hz);
        _owner = this;
    }
    unlock();
}

Then if you look at the i2c_frequency HAL API function you’ll se what I mean about it setting the frequency for all i2c buses, not just this one. Which happens on every write, which means a high chance of a collision across i2c instances when they point to different buses.

For instance, my relevant one is: mbed-os/targets/TARGET_STM/i2c_api.c

Hence the need to deal with acquire in the subclass. BTW a protected method of a superclass can be overridden by a subclass.

Cheers,
Wayne

1 Like
10

Hello Wayne,

Nice analyses! And you are right. There is a flaw in the acquire function.

I think the intent of the I2C class designer was to allow connecting I2C slaves using various clock frequencies to the same I2C bus (have a look at this bug). One solution could be to check the associated peripheral (I2C controller) in the aquire member function.

For example as follows:

void I2C::aquire()
{
    struct i2c_s *_owner_s = I2C_S(_owner);
    struct i2c_s *this_s = I2C_S(this); 

    lock();
    if (_owner != this) {
        if  (_owner_s->i2c == this_s->i2c) {
            i2c_frequency(&_i2c, _hz);
        }
        _owner = this;
    }
    unlock();
}

NOTE: The same flaw is present in the SPI class.

11

I wonder, does every processor support setting I2C frequency on a per-peripheral basis? Or are there some where it’s global across all I2Cs? That would make the Mbed design make sense

12

Hello Jamie,

I wonder, does every processor support setting I2C frequency on a per-peripheral basis?

I haven’t checked all the supported targets, but since each I2C (SPI) controller is a separate piece of hardware equipped with own registers, albeit on the same chip, I think there must be an option to select a frequency per peripheral (I2C controller) basis.