nRF52 I2CSlave Implementation #12161

AGlass0fMilk · 2019-12-20T19:31:20Z

Summary of changes

Implemented I2CSlave functionality for nRF52 targets. Requires PR #12160 .

Impact of changes

Migration actions required

Documentation

Update I2CSlave documentation to reflect new example showing interaction between an Mbed I2C master and I2CSlave.

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@desmond-blue

ciarmcom · 2019-12-20T20:00:39Z

@AGlass0fMilk, thank you for your changes.
@desmond-blue @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-core please review.

AGlass0fMilk · 2019-12-20T20:06:43Z

@maclobdell

desmond-blue

Why targets.json involves other platform's change? Others look good to me.

bulislaw · 2019-12-23T09:39:10Z

I think there's a merge error in the targets.json as some of the newly introduced changes are being removed.

Also our I2CSlave implementation saddens me everytime I look at it... but we didn't have any customer request to making it better. Open for community contributions, if someone needs it and has time.

AGlass0fMilk · 2019-12-23T13:20:57Z

Yeah, I must have messed up something with targets.json. I’ll fix it shortly

@bulislaw what don’t you like about the I2CSlave API? It is kind of Arduino-like (blocking, wasteful of CPU and power) but at least we have an API... I was thinking it was going to be axed soon.

One improvement would be an asynch API for slave operation.

I2CSlave is nice for multi-MCU designs where you just need simple communication. I’m using it for collecting beacon data from a BLE SoC and sending it over CAN using an STM32F0.

AGlass0fMilk · 2019-12-23T16:14:57Z

@desmond-blue @bulislaw I reverted the unrelated changes in targets.json

adbridge · 2019-12-24T12:18:09Z

@bulislaw could you address @AGlass0fMilk's comments ?

bulislaw · 2019-12-27T09:21:03Z

@bulislaw what don’t you like about the I2CSlave API? It is kind of Arduino-like (blocking, wasteful of CPU and power) but at least we have an API... I was thinking it was going to be axed soon.
One improvement would be an asynch API for slave operation.
I2CSlave is nice for multi-MCU designs where you just need simple communication. I’m using it for collecting beacon data from a BLE SoC and sending it over CAN using an STM32F0.

Just ranting here... Yeah, current spinning design is not very efficient. I would say asynch is the only reasonable design pattern for slave.

AGlass0fMilk · 2020-01-02T19:08:37Z

@bulislaw Does this still need work? Please update the labels accordingly.

What's stopping this from being merged?

0xc0170

2 styling issues, otherwise LGTM

0xc0170 · 2020-01-03T15:15:40Z

targets/targets.json

@@ -7472,6 +7472,7 @@
            "FLASH",
            "I2C",
            "I2C_ASYNCH",
+	    "I2CSLAVE",


can you fix alignment here and line below?

0xc0170 · 2020-01-03T15:17:31Z

targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/i2c_api.c

+			} else {
+
+				/* Initialze driver in blocking mode. */
+				nrfx_twi_init(&nordic_nrf5_instance[instance],


this is HAL file, isn't it using spaces as defined in astyle config ? I can see this is 8 spaces at least, or even tabs?

As it was , it was following the style.

0xc0170 · 2020-01-03T15:18:39Z

Once updated, we will run CI

AGlass0fMilk · 2020-01-03T15:27:02Z

Tabs are the bane of my existence... I will revise.

I recently discovered Eclipse has TWO settings you need to change to use spaces instead of tabs so hopefully this will be the last time this happens... 🤞

AGlass0fMilk · 2020-01-08T21:31:39Z

Alright! I fixed my configurations for tabs in both my laptop's eclipse instances and my emacs settings! I also untabified targets.json and i2c_api.c.

This PR should be good to go when the PR it relies on is fixed up. It may need a rebase though.

0xc0170 · 2020-01-09T14:21:54Z

This PR should be good to go when the PR it relies on is fixed up. It may need a rebase though.

Rebased needed, correct.

AGlass0fMilk · 2020-01-09T21:20:15Z

@0xc0170 I will rebase once #12160 is merged.

AGlass0fMilk · 2020-01-09T23:56:12Z

@0xc0170 I realize there are a lot of stupid little whitespace commits/changes in this PR currently. I will deal with those when I rebase.

mergify · 2020-01-20T16:36:28Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

adbridge · 2020-01-22T17:09:20Z

@AGlass0fMilk as per our guidelines, as this is a feature update the following fields need to be filled in with sufficient information for adopters of this functionality:
Impact of changes
Migration actions required

Thanks.

0xc0170 · 2020-02-06T09:13:29Z

@AGlass0fMilk Shall we close and reopen once rebased and fixed or an update incoming anytime soon?

Pull request has been modified.

AGlass0fMilk · 2020-04-03T10:08:48Z

Rebased. Tested and working.

0xc0170 · 2020-04-06T13:54:42Z

@AGlass0fMilk We are making 5.15 CI jobs priority currently, this will be scheduled when we can.

0xc0170 · 2020-04-08T10:31:07Z

CI started

mbed-ci · 2020-04-08T11:03:10Z

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM

AGlass0fMilk · 2020-04-08T14:19:10Z

@0xc0170 Seems like unrelated failures on this target:
DISCO_L475VG_IOT01A

0xc0170 · 2020-04-09T13:36:48Z

There was an example broken, will restart testing

0xc0170 · 2020-04-09T18:56:31Z

CI started

mbed-ci · 2020-04-09T20:02:43Z

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM

AGlass0fMilk · 2020-04-13T16:16:27Z

@0xc0170 Seems to be another CI issue.

Just to be sure, it's not related to the fact that this PR modifies the I2CSlave example?

mbed-os/drivers/I2CSlave.h

Lines 37 to 126 in 6baab6c

    
            * Example Simple I2C slave and master (requires two Mbed-boards): 
        
            * @code 
        
            * #include <mbed.h> 
        
            * #include <mbed_wait_api.h> 
        
            * #include <string.h> 
        
            * 
        
            * #define BUILD_I2C_SLAVE 1 // Build for slave or master of this example 
        
            * 
        
            * #define SLAVE_ADDR 0xA0 
        
            * #define BUFFER_SIZE 6 
        
            * 
        
            * #if BUILD_I2C_SLAVE 
        
            * 
        
            * // Slave side of the example 
        
            * 
        
            * #if !DEVICE_I2CSLAVE 
        
            * #error [NOT_SUPPORTED] I2C Slave is not supported 
        
            * #endif 
        
            * 
        
            * I2CSlave slave(I2C_SDA, I2C_SCL); 
        
            * 
        
            * int main() { 
        
            * 
        
            *     char buf[BUFFER_SIZE] = "ABCDE"; 
        
            * 
        
            *     slave.address(SLAVE_ADDR); 
        
            *     while (1) { 
        
            *         int i = slave.receive(); 
        
            *         switch (i) { 
        
            *             case I2CSlave::ReadAddressed: 
        
            *                 // Write back the buffer from the master 
        
            *                 slave.write(buf, BUFFER_SIZE); 
        
            *                 printf("Written to master (addressed): %s\n", buf); 
        
            *                 break; 
        
            * 
        
            *             case I2CSlave::WriteGeneral: 
        
            *                 slave.read(buf, BUFFER_SIZE); 
        
            *                 printf("Read from master (general): %s\n", buf); 
        
            *                 break; 
        
            * 
        
            *             case I2CSlave::WriteAddressed: 
        
            *                 slave.read(buf, BUFFER_SIZE); 
        
            *                 printf("Read from master (addressed): %s\n", buf); 
        
            *                 break; 
        
            *         } 
        
            *     } 
        
            * } 
        
            * 
        
            * #else 
        
            * 
        
            * // Master side of the example 
        
            * 
        
            * I2C master(I2C_SDA, I2C_SCL); 
        
            * 
        
            * static const char* to_send[] = { "abcde", "12345", "EFGHI" }; 
        
            * 
        
            * int main() { 
        
            *     char buf[BUFFER_SIZE]; 
        
            *     int send_index = 0; 
        
            * 
        
            *     while (1) { 
        
            *         strcpy(buf, to_send[send_index]); 
        
            * 
        
            *         // Write the new message to the slave 
        
            *         if (master.write(SLAVE_ADDR, buf, BUFFER_SIZE)) { 
        
            *             printf("Failed to write to slave!\n"); 
        
            *         } else { 
        
            *             printf("Written to slave: %s\n", buf); 
        
            *         } 
        
            * 
        
            *         // Read what the slave has (should be identical) 
        
            *         if (master.read(SLAVE_ADDR, buf, BUFFER_SIZE)) { 
        
            *             printf("Failed to read from slave!\n"); 
        
            *         } else { 
        
            *             printf("Read from slave: %s\n", buf); 
        
            *         } 
        
            * 
        
            *         // Change the message we're writing to the slave 
        
            *         send_index++; 
        
            *         if (send_index > 2) { 
        
            *             send_index = 0; 
        
            *         } 
        
            * 
        
            *         wait_us(500000); // Wait 0.5s 
        
            *     } 
        
            * } 
        
            * 
        
            * #endif 
        
            * 
        
            * @endcode

0xc0170 · 2020-04-14T07:04:27Z

Correct, will restart

mbed-ci · 2020-04-14T11:44:15Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 3
Build artifacts

ciarmcom requested review from desmond-blue and a team December 20, 2019 20:00

ciarmcom added the needs: review label Dec 20, 2019

desmond-blue reviewed Dec 23, 2019

View reviewed changes

bulislaw added needs: work and removed needs: review labels Dec 23, 2019

desmond-blue approved these changes Dec 24, 2019

View reviewed changes

bulislaw previously approved these changes Dec 27, 2019

View reviewed changes

0xc0170 previously requested changes Jan 3, 2020

View reviewed changes

0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Jan 3, 2020

0xc0170 removed the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Jan 13, 2020

0xc0170 self-requested a review February 6, 2020 09:12

Fixed typo in target.json

2a2295c

Changed pin names in example to be more universal

6baab6c

0xc0170 added needs: CI needs: review and removed needs: work needs: CI labels Apr 6, 2020

0xc0170 approved these changes Apr 6, 2020

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Apr 6, 2020

mergify bot added needs: work and removed needs: CI labels Apr 8, 2020

0xc0170 added needs: CI and removed needs: work labels Apr 9, 2020

mergify bot added ready for merge and removed needs: CI labels Apr 14, 2020

0xc0170 added the release-version: 6.0.0-beta-1 label Apr 14, 2020

0xc0170 merged commit 098c72a into ARMmbed:master Apr 14, 2020

mergify bot removed the ready for merge label Apr 14, 2020

nRF52 I2CSlave Implementation #12161

nRF52 I2CSlave Implementation #12161

Uh oh!

Conversation

AGlass0fMilk commented Dec 20, 2019

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Dec 20, 2019

Uh oh!

AGlass0fMilk commented Dec 20, 2019

Uh oh!

desmond-blue left a comment

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Dec 23, 2019

Uh oh!

AGlass0fMilk commented Dec 23, 2019

Uh oh!

AGlass0fMilk commented Dec 23, 2019

Uh oh!

adbridge commented Dec 24, 2019

Uh oh!

bulislaw commented Dec 27, 2019

Uh oh!

AGlass0fMilk commented Jan 2, 2020

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jan 3, 2020

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jan 3, 2020

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jan 3, 2020

Uh oh!

AGlass0fMilk commented Jan 3, 2020

Uh oh!

AGlass0fMilk commented Jan 8, 2020

Uh oh!

0xc0170 commented Jan 9, 2020

Uh oh!

AGlass0fMilk commented Jan 9, 2020

Uh oh!

AGlass0fMilk commented Jan 9, 2020

Uh oh!

mergify bot commented Jan 20, 2020

Uh oh!

adbridge commented Jan 22, 2020

Uh oh!

0xc0170 commented Feb 6, 2020

Uh oh!

AGlass0fMilk commented Apr 3, 2020

Uh oh!

0xc0170 commented Apr 6, 2020

Uh oh!

0xc0170 commented Apr 8, 2020

Uh oh!

mbed-ci commented Apr 8, 2020

Test run: FAILED

Uh oh!

AGlass0fMilk commented Apr 8, 2020

Uh oh!

0xc0170 commented Apr 9, 2020

Uh oh!

0xc0170 commented Apr 9, 2020

Uh oh!

mbed-ci commented Apr 9, 2020

Test run: FAILED

Uh oh!

AGlass0fMilk commented Apr 13, 2020

Uh oh!

0xc0170 commented Apr 14, 2020