Unable to work with two physical I2C buses

[rkotan]

Description of defect

Some time ago I updated mbed-os to 6.11 and suddenly faced performance issue.
CPU load has become very high, min 95% though it had never been above 45-50% before. After some profiling and digging in the sources I found the cause of this in the I2C driver. I use two physical buses with several devices on each, so it seems appropriate to me to have one I2C class instance per bus. However, I2C class has static members _owner and _mutex (lines 237-239 of I2C.h):

    static I2C  *_owner;
    int    _hz;
    static SingletonPtr<PlatformMutex> _mutex;

So, both of them are shared between all instances in the program and every call of I2C::read or I2C::write from another I2C instance completely reconfigures it via calling I2C::acquire. For obvious reasons this behavior seems completely wrong and in my case after some time I2C::set_frequency starts to eat up all available CPU time, because of deadlock or some improperly configured periphery.

I simply made _owner and _mutex non-static and issue was gone, though in that case I2C::acquire and _owner also seems useless.

I suggest that _owner and _mutex should be non-static for obvious reasons and some consistency with behavior of SPI class where both of them are non-static.

Target(s) affected by this defect ?

probably all, tested on STM32H743

Toolchain(s) (name and version) displaying this defect ?

gcc version 10.2.1 20201103 (release)

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.11.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli 1.10.5

How is this defect reproduced ?

1. Initialize I2C instances for each of two (or more) separate physical buses.
2. Connect a device to each bus.
3. Try to write/read something to/from each device.

[LMESTM]

Probably a duplicate of #14732
@rkotan You may want to revert #14557 until this is fixed - sorry about the inconvenience

[rkotan]

@LMESTM

You may want to revert #14557 until this is fixed - sorry about the inconvenience

Originally I used commit 14e5d30 from May 17 which was made before merging this feature. Nevertheless, I updated my local version to 5047bd3 and reverted d974b47 -- nothing has changed.

Besides, I've fixed this issue locally and can make a PR if it's ok.

[rkotan]

Updated the original post: added static keywords which were missing in the piece of code but are present in the actual header file 😅

[jeromecoutant]

Besides, I've fixed this issue locally and can make a PR if it's ok.

Sure, we will be happy to review your proposition!
Thx

[affrinpinhero-2356]

Hi @rkotan,
I would like to get some more info regarding your application.

• Target clock speed.
• I2C Bus speed
• SHA/Link to repo which u have previously used.
• If possible peace of code which triggered this issue.

[affrinpinhero-2356]

I was not able to reproduce this issue.
Target : H747-Disco.
Slave :

• ADXL345(I2C_1)
• MPU6050(i2C_4).

Method used for reproduce:
Created an I2C example that read and write 2 bytes of data on I2C_1 & I2C_4. Used mbed statistic example mentioned (https://os.mbed.com/docs/mbed-os/v6.11/apis/mbed-statistics.html). To tweak CPU statistics/load.

Observations:
Before Rebase (3ee443b)

ADXL DIV ID 0xe5, MPU DIV ID 0x68
Time(us): Up: 26033081 Idle: 24296451 Sleep: 24296451 DeepSleep: 0
Idle: 91% Usage0x1 : 9%
	
W_T 4100, TRD CND 6
	
ADXL DIV ID 0xe5, MPU DIV ID 0x68
Time(us): Up: 28033081 Idle: 26133548 Sleep: 26133548 DeepSleep: 0
Idle: 91% Usage0x1 : 9%

After Rebase (Commit: c780165)

W_T 400, TRD CND 73
	 
ADXL DIV ID 0xe5, MPU DIV ID 0x0
Time(us): Up: 142033081   Idle: 131993262   Sleep: 131993262   DeepSleep: 0
Idle: 91% Usage0x1 : 9%
	 
W_T 300, TRD CND 81
	 
ADXL DIV ID 0xe5, MPU DIV ID 0x0
Time(us): Up: 144033081   Idle: 133827644   Sleep: 133827644   DeepSleep: 0
Idle: 91% Usage0x1 : 9%

[0xc0170]

Besides the change in the target code (to increase the time).

We still shall consider doing similar work as SPI did in the pull request #9469 (cc @kjbracey-arm - would it make sense to do the same peripheral logic for I2c).

[rkotan]

Hi @affrinpinhero-2356,

I would like to get some more info regarding your application.

Of course, my specs are:

Target: custom, based on the STM32H743VIT6 (mbed-os target inherited from MCU_STM32H743xI)
Target clock speed: 400 MHz
I2C Bus speed: 400 kHz in all cases (for all buses and all devices). I also tried Standard speed (100 kHz) -- there is no change.
Slaves: 24LC16B & BMX160 on I2C1, LIS3MDL & LPS25HB on I2C4.
Sensors update rate: BMX160 -- 100 Hz, LIS3MDL -- 20 Hz, 24LC16B & LPS25HB -- on demand
Old mbed version: dde7b22

I use a blocking read/write for configuring sensors and managing settings in the EEPROM and asynchronous transfer to obtain measurements from sensors. For each physical bus I have a dedicated thread in which this piece of code is called every time when sensor measurements are requested:

	void sendNextMsgFromQueue()
	{
		I2CMessage *cur_msg;
		const bool result = this->msg_queue_.try_get(&cur_msg);
		if(result)
		{
			i2c_.transfer(cur_msg->addr_, cur_msg->tx_buf_, cur_msg->tx_length_,
						  cur_msg->rx_buf_, cur_msg->rx_length_,
						  mbed::Callback<void (int)>(this, &I2C_Bus::dataCallback), I2C_EVENT_TRANSFER_COMPLETE, false);
			this->cur_event_ = cur_msg->callback_;
		}

	}

My program currently hangs during the asynchronous transfer to BMX160 which contains 1 TX byte (which is 0x04 -- start address of measurement results) and 20 RX bytes. I can share sensor configurations in case you need it.

[affrinpinhero-2356]

Hi @rkotan
Thanks for the update.
I will check this scenario and update it here.

[kjbracey]

Besides the change in the target code (to increase the time).

We still shall consider doing similar work as SPI did in the pull request #9469 (cc @kjbracey-arm - would it make sense to do the same peripheral logic for I2c).

Quite possibly, yes - conceptually the I2C is akin to the SPI. The C++ object really represents an endpoint on a shared bus, so to cope with two independent users you need both objects referencing one bus to be using the same mutex. The suggestion in the OP to remove the static on the mutex breaks the single-bus case.

But having only one mutex impairs performance of multiple buses - they become needlessly mutexed against each other.

The format switching as if on a single bus shouldn't be totally breaking multiple bus use though. The buses should be used sequentially, and if the other bus was last used, then the bus will get reconfigured. But that shouldn't be doing anything other than wasting time.

I guess that is just the complaint? Low performance? I'm surprised that any sort of peripheral reconfiguration would be slow compared to actual transfer speed - I2C is so slow!