New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ONSEMI: Fix a few issues related to I2C #5394

Merged
merged 1 commit into from Nov 6, 2017

Conversation

Projects
None yet
6 participants
@danclement
Contributor

danclement commented Oct 27, 2017

Three main issues were found and corrected:

  1. The 0x13 special case section in write data in ncs36510_i2c.c didn't have a write++ command.
  2. In the same write function, the WDAT8 command was put before the 0x13 section and this is not correct
  3. Needed to add wait_us(0) before and after the register writes for apparent clock domain crossing delay times until registers are stable in HW

There were also a handful of other tweaks related to general code maintenance and moving some status register checks to the proper locations.

Description

This pull request addresses some I2C issues that were found by CI test shield and by a customer. This pull request solves #5007 and #3511.

Status

Fully tested by extensive ON Semiconductor tests based on CI test shield hardware as well as fully verified on CI test shield with CI test shield tests for I2C.

@jacobjohnson-ON
@maclobdell

Checking in the fixes related to I2C issues.
Three main issues:
1) The 0x13 special case section in write data in ncs36510_i2c.c didn't have a write++ command.
2) In the same write function, the WDAT8 command was put before the 0x13 section and this is not correct
3) Needed to add wait_us(0) before and after the register writes for apparent clock domain crossing delay times until registers are stable in HW

There were also a handful of other tweaks related to general code maintenance and moving some status register checks to the proper locations.
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 28, 2017

Fully tested by extensive ON Semiconductor tests based on CI test shield hardware as well as fully verified on CI test shield with CI test shield tests for I2C.

+1 for testing and seeing these fixes. If you can also add/paste here the test results for this patch , to share what tests have been run.

// The wait_us(0) command is needed so the I2C state machines have enough
// time for data to settle across all clock domain crossings in their
// synchronizers, both directions.
#define SEND_COMMAND(cmd) wait_us(0); obj->membase->CMD_REG = cmd; wait_us(0);

This comment has been minimized.

@0xc0170

0xc0170 Oct 28, 2017

Member

It was wait_us(1), now it is 0? What is intention with 0 as a time waiting period?

This comment has been minimized.

@danclement

danclement Oct 28, 2017

Contributor

Hello @0xc0170, I ran the I2C tests for the CI test shield, all the mbed tests, and our own test program below with 0 failures after many runs, even at different system clock divider frequencies.

For the delay going to 0 instead of 1us, only 0 is needed. The system just needs a handful of extra clock cycles so giving the MCU something to do so the internal state machines can synchronize the data. There are clock domain crossings as the data crosses from APB bus to internal and then back. It's not exactly a bug but not exactly what was intended either.

Here is the test code. I've ran tens of thousands of transactions and no longer see any failures.

// check if I2C is supported on this device
#if !DEVICE_I2C
  #error [NOT_SUPPORTED] I2C not supported on this platform, add 'DEVICE_I2C' definition to your platform.
#endif

#include "mbed.h"
#include "I2CEeprom.h"
#include <string.h>

#define I2C_ADDRESS 0xA0
#define SIZE_DATA 33
#define ITERATIONS 512

int total_failures = 0;
int total_num_tests = 0;

Serial pc(USBTX, USBRX);
I2CEeprom memory(I2C_SDA, I2C_SCL, I2C_ADDRESS, 32, 0, 100000);

// Fill array with random characters
void init_string(char* buffer, int len)
{
    srand(time(NULL));
    int x = 0;
    for(x = 0; x < len; x++){
        buffer[x] = 'A' + (rand() % 26);
    }
    buffer[len-1] = 0; // add \0 to end of string
    //pc.printf("\r\n****\r\nBuffer Len = `%d`, String = `%s`\r\n****\r\n",len,buffer);
}

void flash_WR(PinName I2C_SDA, PinName I2C_SCL, int size, int eeprom_address)
{
    //I2CEeprom memory(I2C_SDA, I2C_SCL, eeprom_address, 32, 0);
    int num_read = 0;
    int num_written = 0;
    volatile char test_string[size] = {0};
    volatile char read_string[size] = {0};
    init_string((char *)test_string,size); // populate test_string with random characters
    for(int x = 0; x< size;x++){
        read_string[x] = 0;
    }
    //pc.printf("\r\n****\r\n Test String = `%s` \r\n****\r\n",test_string);

    num_written = memory.write(eeprom_address,(char *)test_string,size);
    num_read = memory.read(eeprom_address,(char *)read_string,size);

	pc.printf("\r\n****\r\n eeprom_address = 0x%x\r\n Len = %d\r\n Num Bytes Written = %d \r\n Num Bytes Read = %d \r\n Written String = %s \r\n Read String = %s \r\n****\r\n",eeprom_address,size,num_written,num_read,test_string,read_string);

	if(num_written != num_read) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Wrote %d bytes but only read back %d bytes...\r\n****\r\n", num_written, num_read);
		return;
	}

	if(strcmp((const char *)test_string, (const char *)read_string) != 0) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Strings don't match!...\r\n****\r\n ");
		return;
	}
}

// Test single byte R/W
void single_byte_WR_commands(PinName I2C_SDA, PinName I2C_SCL, int eeprom_address)
{
	srand(time(NULL));
    char test = 0;
    char read;
    int r = 0;
    int w = 0;

    for(int i = 0; i < 0x20; i++) {
    	test = i;
    	total_num_tests++;

		w = memory.write(eeprom_address,test);
		r = memory.read(eeprom_address,read);

		pc.printf("\r\n****\r\n eeprom_address = 0x%x\r\n Num Bytes Read = %d \r\n Num Bytes Written = %d \r\n Read byte = 0x%x \r\n Written Byte = 0x%x \r\n****\r\n",eeprom_address,r,w,read,test);
		if(w != r) {
			total_failures++;
			pc.printf("\r\n****\r\n ERROR: Wrote %d bytes but only read back %d bytes...\r\n****\r\n", w, r);
			return;
		}

		if(test != read) {
			total_failures++;
			pc.printf("\r\n****\r\n ERROR: Bytes don't match! Sent 0x%x, received 0x%x...\r\n****\r\n ", test, read);
			return;
		}
    }
}

// Test single byte R/W
void single_byte_WR(PinName I2C_SDA, PinName I2C_SCL, int eeprom_address)
{
	srand(time(NULL));
    char test = rand() % 0xFF;

    char read;
    int r = 0;
    int w = 0;

    total_num_tests++;

    w = memory.write(eeprom_address,test);
    r = memory.read(eeprom_address,read);

	pc.printf("\r\n****\r\n eeprom_address = 0x%x\r\n Num Bytes Read = %d \r\n Num Bytes Written = %d \r\n Read byte = 0x%x \r\n Written Byte = 0x%x \r\n****\r\n",eeprom_address,r,w,read,test);
	if(w != r) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Wrote %d bytes but only read back %d bytes...\r\n****\r\n", w, r);
		return;
	}

	if(test != read) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Bytes don't match! Sent 0x%x, received 0x%x...\r\n****\r\n ", test, read);
		return;
	}
}

int genRandAddress() {
	srand(time(NULL));
	return (rand() % 0x7FFF);
}

// Entry point into the tests
int main()
{	
	pc.printf("\r\n****\r\n Starting single byte tests... \r\n****\r\n ");
	single_byte_WR_commands(I2C_SDA, I2C_SCL, genRandAddress());
	if(1) {
		for(int i=0; i < ITERATIONS; i++) {
			total_num_tests++;	
			single_byte_WR(I2C_SDA, I2C_SCL, genRandAddress());
			pc.printf("\r\n Test iteration = %d \r\n", total_num_tests);
		}
	}
	pc.printf("\r\n****\r\n Starting multi-byte tests... \r\n****\r\n");
	if(1) {
		for(int i=0; i < ITERATIONS; i++) {
			total_num_tests++;
			flash_WR(I2C_SDA, I2C_SCL, SIZE_DATA, genRandAddress());
			pc.printf("\r\n Test iteration = %d \r\n", total_num_tests);
		}
	}
	pc.printf("\r\n****\r\n Done... \r\n****\r\n");
	pc.printf("\r\n****\r\n FAILED %d tests out of %d total tests \r\n****\r\n", total_failures, total_num_tests);
}

This comment has been minimized.

@0xc0170

0xc0170 Nov 2, 2017

Member

Thanks, that explains it

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 30, 2017

0xc0170 are you happy with this now ?

@maclobdell

This comment has been minimized.

Contributor

maclobdell commented Nov 1, 2017

Nice work Dan!

@0xc0170

0xc0170 approved these changes Nov 2, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 2, 2017

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Nov 2, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 2, 2017

Build : SUCCESS

Build number : 413
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5394/

Triggering tests

/morph test
/morph uvisor-test

@theotherjimmy theotherjimmy changed the title from Checking in the fixes related to I2C issues. to Fix a few issues related to I2C Nov 2, 2017

@theotherjimmy theotherjimmy changed the title from Fix a few issues related to I2C to ONSEMI: Fix a few issues related to I2C Nov 2, 2017

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Nov 3, 2017

@theotherjimmy theotherjimmy merged commit 209d2fa into ARMmbed:mbed-os-5.6 Nov 6, 2017

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danclement danclement deleted the danclement:mbed-os-5.6 branch Nov 6, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Nov 7, 2017

This should not have got merged to mbed-os-5.6! PRs always go to master first....
@theotherjimmy please revert this and target master.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 7, 2017

Whoops. I did not notice that. I'll revert this pronto

@0xc0170 0xc0170 referenced this pull request Nov 7, 2017

Merged

ONSEMI: Fix I2C issues #5452

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 7, 2017

Whoops. I did not notice that. I'll revert this pronto

I already sent the fix to master. see the ref above. As this was just a day ago, can be removed also.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 7, 2017

Reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment