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

Add SCL and SDA defs for I2C[0-2]; redefine I2C_[SCL,SDA] to I2C2 #4102

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
7 participants
@wdwalker
Contributor

wdwalker commented Mar 31, 2017

Description

See discussion in issue #4099. I've only tested this by compiling an I2C app with an LPC1768 target and then running the result. I have yet to be able to successfully run 'mbed test' in my environment (against stable, head, etc.), so I'm not able to demonstrate the results of that.

Status

READY/IN DEVELOPMENT/HOLD

Todos

  • Tests
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 31, 2017

@bridadan Is this a /morph test candidate? or do we need a different command? asked offline. just running morph test is good enough.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 31, 2017

@0xc0170 Could we tag the NXP maintainers in this one?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 31, 2017

@wdwalker do you plan on adding documentation? If not, could you edit the status and todo sections of the top comment to reflect the status and remaining todos of this PR?

@theotherjimmy

👍 I like the small diff.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 31, 2017

/morph test

@geky

geky approved these changes Mar 31, 2017

Looks good 👍

// Not connected
NC = (int)0xFFFFFFFF
NC = (int)0xFFFFFFFF,

This comment has been minimized.

@geky

geky Mar 31, 2017

Member

Actually, this should probably be declared at the end of the enum. If someone came along and added a new pin, the enum may overflow (since enums increment from the last value). I'm not entirely sure what happens at that point.

This comment has been minimized.

@geky

geky Mar 31, 2017

Member

Actually I just looked it up, the enum will just upgrade to uint64_t. It's up to you if you want it at the end.

This comment has been minimized.

@theotherjimmy

theotherjimmy Mar 31, 2017

Contributor

Moving the NC to the bottom and removing the , would result in a smaller diff.

This comment has been minimized.

@wdwalker

wdwalker Mar 31, 2017

Contributor

The thing I was attempting to do here was use NC to indicate I2C0 was not connected on the LPC1768 and allow any error-checking code to kick in. In order to do that, NC needs to be ahead of these lines:

I2C_SCL0 = NC,
I2C_SDA0 = NC,

Alternatively, I could just omit these lines and leave NC at the bottom of the file.

This comment has been minimized.

@wdwalker

wdwalker Mar 31, 2017

Contributor

BTW - at this point in this particular file, it seems as though all auto-increment for the enum is pretty much done with and we're in a section where the rest of the enum items are forced to have a specific value.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 31, 2017

clarification: the tag means that we are waiting on the continuous integration to finish.

@0xc0170

0xc0170 approved these changes Apr 3, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 3, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 3, 2017

@bridadan morph test restart needed

@bridadan

This comment has been minimized.

Contributor

bridadan commented Apr 3, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Apr 3, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1814

All builds and test passed!

@wdwalker

This comment has been minimized.

Contributor

wdwalker commented Apr 7, 2017

Hi there - this is only my second PR, so I'm not fully clear on the process. With the exception of the "needs CI" label, it looks like all is good (including the CI checks). What needs to be done to close this PR?

@bridadan

bridadan requested changes Apr 7, 2017 edited

Pretty sure this is a breaking change, please see below

Please ignore 😄

I2C_SCL2 = p27, // pin used by application board
I2C_SDA2 = p28, // pin used by application board
I2C_SCL = I2C_SCL2,
I2C_SDA = I2C_SDA2,

This comment has been minimized.

@bridadan

bridadan Apr 7, 2017

Contributor

I'm pretty sure this is going to be a breaking change. I2C_SCL used to follow this map I2C_SCL->D15->P0_28. After these changes, I2C_SCL will follow this map: I2C_SCL->I2C_SCL2->p27->P0_11. I2C_SDA is also following a different mapping. This will break any current application that use the I2C_SCL and I2C_SDA pin names.

@bridadan bridadan added needs: review and removed needs: CI labels Apr 7, 2017

@bridadan

...and I was completely wrong! I should have read your in-depth and very comprehensive analysis in #4099.

I think this change is perfectly valid and a welcome improvement. The current assignments appear to be bogus and are most likely a copy-paste error from the ARCH_PRO platform, which uses the LPC1768 MCU.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 7, 2017

So, LGTM

@sg- sg- merged commit 833a201 into ARMmbed:master Apr 10, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has 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