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

nRFx: Use us ticker for I2C timeout #5187

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
5 participants
@nvlsianpu
Contributor

nvlsianpu commented Sep 25, 2017

Change implementation of timeout to one that is using us_ticker hal.
Timeout is now configurable by I2C_TIMEOUT_VALUE_US macro and this
value will be imported if was defined externally.

Fix for : #4826

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

Not certain about directly using us ticker API within hal? Shouldn't this use ticker API directly ?

cc @c1728p9 @pan-

targets/TARGET_NORDIC/TARGET_NRF5/i2c_api.c Outdated
// The current transfer may be suspended (if it is RX), so it must be
// resumed before the STOP task is triggered.
nrf_twi_task_trigger(twi, NRF_TWI_TASK_RESUME);
nrf_twi_task_trigger(twi, NRF_TWI_TASK_STOP);
uint32_t remaining_time = TIMEOUT_VALUE;
t0 = us_ticker_read();

This comment has been minimized.

@c1728p9

c1728p9 Sep 26, 2017

Contributor

The upcoming microsecond ticker specification allows the microsecond ticker to use frequencies other than 1MHz, and bit widths fewer than 32 which could cause problems here.

To ensure the microsecond ticker is initialized, and to ensure the right frequency you might want to go through the mbed ticker api. It would look something like this ticker_read(get_us_ticker_data());.

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Sep 27, 2017

Not certain about directly using us ticker API within hal? Shouldn't this use ticker API directly ?

@pan- What is your opinion?

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Sep 28, 2017

@0xc0170 Mabey I can answer to you and myself by quotation of @pan-
#4826 (comment):

It is not desirable to use the C++ Timer API in the C HAL. The HAL should not depends on APIs exposed on higher level. However I believe it is OK to use the timer API from the hal.

Please let me know how this works for you.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 28, 2017

Defenitely not C++, I was referring to tickers API that is part of HAL (generic implementation though). As @c1728p9 pointed out, using ticker_ calls instead of us_ticker.

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Sep 28, 2017

Thanks for the answer. I will update this PR in next week.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Sep 29, 2017

@theotherjimmy theotherjimmy changed the title from fix bug: I2C timeout too early due the clos strething by slave on nRFx SoC to fix bug: I2C timeout too early due the clock streching by slave on nRFx SoC Oct 2, 2017

@theotherjimmy theotherjimmy changed the title from fix bug: I2C timeout too early due the clock streching by slave on nRFx SoC to fix bug: I2C timeout too early due to clock streching by slave on nRFx SoC Oct 2, 2017

@theotherjimmy theotherjimmy changed the title from fix bug: I2C timeout too early due to clock streching by slave on nRFx SoC to fix bug: I2C timeout too early due to clock stretching by slave on nRFx SoC Oct 2, 2017

@theotherjimmy theotherjimmy changed the title from fix bug: I2C timeout too early due to clock stretching by slave on nRFx SoC to fix: I2C timeout too early due to clock stretching by slave on nRFx SoC Oct 5, 2017

@theotherjimmy theotherjimmy changed the title from fix: I2C timeout too early due to clock stretching by slave on nRFx SoC to nRFx: Use us ticker for I2C timeout Oct 5, 2017

fix bug: I2C timeout due the clos strething by slave on nRFx SoC
Change implementation of timeout to one that is using us_ticker hal.
Timeout is now configurable by I2C_TIMEOUT_VALUE_US macro and this
value will be imported if will be defined externaly.

@nvlsianpu nvlsianpu force-pushed the nvlsianpu:fix_i2c_timeout branch to 2a0d38e Oct 9, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 9, 2017

@0xc0170

@c1728p9 Please rereview

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 10, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

Build : SUCCESS

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

Triggering tests

/test mbed-os

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

Build : SUCCESS

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

Triggering tests

/test mbed-os

@mbed-ci

This comment has been minimized.

@theotherjimmy theotherjimmy merged commit 0b796cd into ARMmbed:master Oct 13, 2017

5 checks passed

Cam-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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment