Nordic RF52 does not correctly support simultaneous use of I2C and SPI

[NeilMacMullen]

Mbed OS 5
Target UBLOX_EVK_NINA_B1
Toolchain GCC_ARM

The Nordic RF52 SoC uses digital 'TWI' control blocks to control both I2C and SPI. When declaring an I2C object, the Nordic platform code in targets/TARGET_NORDIC/TARGET_NRF5/i2c_api.c uses the method "i2c_init" to allocate the first available TWI by examining previously-allocated I2C instances. Similar code is used in the SPI drivers (spi_api.c) where previously-allocated SPI instances are checked.

This approach fails when allocating a mixture of I2C and SPI instances since both _init methods then consider TWI0 to be available and allocate it to both the I2C and SPI interfaces. This leads to unpredictable results when the interfaces are used.

If you only have a single I2C and SPI instance, it's possible to work around this by forcing one of the allocators to allocate TWI1 (example code below) but the correct fix would be to implement a central allocator which would be used for both types of interface.

diff --git a/targets/TARGET_NORDIC/TARGET_NRF5/i2c_api.c b/targets/TARGET_NORDIC/TARGET_NRF5/i2c_api.c
index 66063b4..ca551d4 100644
--- a/targets/TARGET_NORDIC/TARGET_NRF5/i2c_api.c
+++ b/targets/TARGET_NORDIC/TARGET_NRF5/i2c_api.c
@@ -283,6 +283,7 @@ static void twi_clear_bus(twi_info_t *twi_info)
     }
}
+#define I2C_TWI 1
void i2c_init(i2c_t *obj, PinName sda, PinName scl)
{
     int i;
@@ -290,7 +291,7 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl)
         if (m_twi_info[i].initialized &&
             m_twi_info[i].pselsda == (uint32_t)sda &&
             m_twi_info[i].pselscl == (uint32_t)scl) {
-            TWI_IDX(obj) = i;
+            TWI_IDX(obj) = I2C_TWI;
             TWI_INFO(obj)->frequency = NRF_TWI_FREQ_100K;
             i2c_reset(obj);
             return;
@@ -299,7 +300,7 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl)
 
     for (i = 0; i < TWI_COUNT; ++i) {
         if (!m_twi_info[i].initialized) {
-            TWI_IDX(obj) = i;
+            TWI_IDX(obj) = I2C_TWI;
 
             twi_info_t *twi_info = TWI_INFO(obj);
             twi_info->initialized = true;
 

Copying @MarceloSalazar by request

[0xc0170]

cc @nvlsianpu @pan-

[nvlsianpu]

Thank you for describing this bug.

[shayo]

@nvlsianpu, we are experiencing the same bug here as well.
Our board has both I2C and SPI.
When we define both SPI and I2C, none of them work.
When I remove the declaration of SPI, I2C works.

Is there a patch to solve this issue? I tried taking the latest code but the problem is still there.
This is a major blocker for us right now.

[shayo]

@NeilMacMullen , I also tried your solution (modifying the i2c_init), but it didn't work for me. Is there something else I can try? Which version did you base your patch on?

[nvlsianpu]

did you test the path #4491?

[shayo]

I've tried c416fdf, after you added the central resource manager. It still doesn't work (both I2C and SPI).

[shayo]

It seems like the system hangs with your branch when I call "wait".
This happens also when I use the main branch.
Are there also changes to us_ticker_read that I should take from somewhere?
This might also explain why the SPI code initialization hang.

[NeilMacMullen]

@nvlsianpu - apologies, I didn't get any notification of #4491 so have only just seen this change. @shayo - I will try this here and let you know whether it works for me.

[NeilMacMullen]

@nvlsianpu @shayo I agree that this seems at first sight to be broken. I did... "git pull origin pull/4491/head" in a new project then verified that the i2c_api file does include the latest changes then copied my application source to the new project and built it. In my case, the symptoms are that "i2c->write(address, NULL, 0);" returns 0 for any address, indicating that the entire bus is populated with devices. The same code without these changes (and with my original workaround) correctly identifies the few I2C devices I have on the bus.

[shayo]

@NeilMacMullen , which branch/version did you apply your changes to?

[NeilMacMullen]

@shayo - I just used 'mbed update to get a recent version. According to git log, last couple of commits were...
commit ed4febe
commit 9ce6141

[shayo]

@nvlsianpu, any news on this?

[NeilMacMullen]

@shayo -see the comment on the PR - looks like there will be another attempt at this.

[nvlsianpu]

Status in progress: #4491 (comment)

[nvlsianpu]

@NeilMacMullen Are you able to test whether #4634 fixes the issue?

[NeilMacMullen]

@nvlsianpu - sorry for not getting back to you earlier. Unfortunately #4634 also appears to have problems. Although I can successfully now detect I2C devices on the bus and also use a serial flash on the SPI bus, the application appears to crash on a subsequent call to Thread::wait. This does not occur with my original changes on earlier mbed.

[NeilMacMullen]

@nvlsianpu - actually the issue above may be a red herring. It looks like something else has changed between 4.5 and 5.5 to break my application.

[nvlsianpu]

OS 5.5 upgrades to the latest CMSIS version, CMSIS5, including the CMSIS-RTOS2 kernel. So changes were big. Did you tried latest minor release 5.5.1?

[NeilMacMullen]

@nvlsianpu I've raised this issue as #4686 - it appears to be a regression on the Serial driver.

[seppestas]

What's the status of the issue? We are also experiencing weird problems with I2C interface that might be explained by this issue (branch based on mbed-os 5.4.7).

It looks like this issue was fixed in PR #4634, but this issue is still open. Is there a reason for this?

[NeilMacMullen]

@seppestas Sorry not to reply earlier. From my testing the issue appears to be resolved on 5.6. Having said that, I am interested in the nature of the problems you are seeing. We still see a high level of I2C failures and SPI lockups even with this fix. I'm still trying to establish whether this is actually a hardware issue or further issues with software/digits.

[nvlsianpu]

This is what I have on output form ci-test-shield test for concurrent I2C/SPI verification (see https://github.com/ARMmbed/ci-test-shield/blob/master/TESTS/concurrent/Comms/Comms.cpp):
Test was compiled against NRF52_DK@ARM_GCC

I ran it by hand for mitigating any IF firmware problems we have.
Can we close this bug?

[0xc0170]

Thanks @NeilMacMullen and @nvlsianpu for the updates