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

Add -D{board.variant} to build.extra_flags of PCA1000X softdevices #224

Merged
merged 1 commit into from
Jan 16, 2018
Merged

Conversation

oyooyo
Copy link
Contributor

@oyooyo oyooyo commented Jan 14, 2018

The build.extra_flags for PCA1000X boards include -D{board.variant}:
PCA1000X.build.extra_flags=-DNRF51 -D{board.variant}
However, this extra flag is currently not also part of the build.extra_flags of the PCA1000X softdevices:

PCA1000X.menu.softdevice.s110.build.extra_flags=-DNRF51 -DS110 -DNRF51_S110
[...]
PCA1000X.menu.softdevice.s130.build.extra_flags=-DNRF51 -DS130 -DNRF51_S130

Because of this, -D{board.variant} will actually only be used on PCA1000X boards when selecting softdevice "none".
On PCA1000X boards that require hardware flow control for UART communication (like the PCA10000 and PCA10001), this for example leads to the strange effect that serial port communication currently only works when selecting softdevice "none".

@sandeepmistry
Copy link
Owner

@oyooyo these changes look ok to me, could you please also update the Travis CI file to leverage the new variant, see: https://github.com/sandeepmistry/arduino-nRF5/blob/master/.travis.yml#L30-L31

You'll just need a new line.

dlabun
dlabun previously requested changes Jan 14, 2018
Copy link
Collaborator

@dlabun dlabun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like changes from PR#226 to add the new board is also included in this PR... Could you remove them?

@dlabun dlabun dismissed their stale review January 14, 2018 16:39

I'm dismissing my review (Sandeep already caught this and commented on #226)

@sandeepmistry sandeepmistry mentioned this pull request Jan 14, 2018
@oyooyo
Copy link
Contributor Author

oyooyo commented Jan 16, 2018

I believe I was successful at removing the duplicate commit, so this PR should be okay now.

@dlabun dlabun merged commit 41f2aad into sandeepmistry:master Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants