Skip to content
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

nrf52 - fix i2c/twi driver #11676

Merged
merged 1 commit into from Oct 15, 2019
Merged

Conversation

@maciejbocianski
Copy link
Member

maciejbocianski commented Oct 11, 2019

Sync TWI driver to sdk version 15.3.0 to get rid of data length limitation

Description

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@jamesbeyond

Release Notes

@maciejbocianski maciejbocianski changed the title nrf5x - fix i2c/twi driver nrf52 - fix i2c/twi driver Oct 11, 2019
@ciarmcom ciarmcom requested review from jamesbeyond and ARMmbed/mbed-os-maintainers Oct 11, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 11, 2019

@maciejbocianski, thank you for your changes.
@jamesbeyond @ARMmbed/mbed-os-maintainers please review.

@40Grit

This comment has been minimized.

Copy link

40Grit commented Oct 11, 2019

I could always tell whoever wrote the nrf i2c driver probably had not used it very extensively.

Was really annoying bug, especially when trying to dump an accelerometers large sample buffer.

Though using int like the mbed os api does is not much better.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:nrf5X_fix_i2c_twi_sdk branch from 0e7cf55 to 4369743 Oct 12, 2019
Sync TWI driver to sdk version 15.3.0 to get rid of data length limitation
@maciejbocianski maciejbocianski force-pushed the maciejbocianski:nrf5X_fix_i2c_twi_sdk branch from 4369743 to 7cc0b61 Oct 12, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 14, 2019

Though using int like the mbed os api does is not much better.

Is this related to drivers using basic types like int ?

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 14, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Oct 14, 2019

Is this related to drivers using basic types like int ?

Just like uint8_t can be too small for data size, so could int. size_t is technically correct.

But in practice, in Mbed OS-size embedded systems, int is probably fine, and may be preferable. For example, Nanostack tends to avoid size_t, due to 8 or 16-bit platforms that use a large memory model to address ROM (>64K), but have <64K RAM. In those platforms, size_t is 32-bit to cope with ROM objects, but it's overkill and bloat for RAM buffers. So I'd be sympathetic for int if envisaging portability to those systems.

But if assuming 32-bit ARM or bigger, then size_t is solid - there's no overhead, and only a benefit when you go 64-bit.

@40Grit

This comment has been minimized.

Copy link

40Grit commented Oct 14, 2019

@0xc0170, @kjbracey-arm

My main beef with int is its signed-ness and the fact that it provides no implication as to it's use.

Maybe if we had an '''i2c_tranceive()''' function signed length would make sense. I joke.

I am a huge fan of type defining for context as well.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Oct 14, 2019

My main beef with int is its signed-ness and the fact that it provides no implication as to it's use.

I'm aware that there's a large contingent of people who regard the unsignedness of size_t as a significant problem. (eg https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-nonnegative) I can see the arguments both ways.

I am a huge fan of type defining for context as well.

Agreed - if doing that, I'd prefer to have some sort of buffer_size_t typedef to at least indicate what it is. (Even if it doesn't actually cause any real compiler type checking).

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 15, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 15, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit ea3ead0 into ARMmbed:master Oct 15, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8696 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.