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

Fix flashing problem on `nRF51DK` & `nRF52DK` #1094

Merged
merged 1 commit into from Jul 7, 2018

Conversation

Projects
None yet
4 participants
@niklasad1
Copy link
Member

niklasad1 commented Jul 7, 2018

Pull Request Overview

This pull request closes #1093

Testing Strategy

Tested on both boards!

TODO or Help Wanted

N/A

Documentation Updated

- [ ] Updated the relevant files in /docs, or no updates are required.

Formatting

- [ ] Ran make formatall.

Fix flashing problem on `nRF51DK` & `nRF52DK`
* Copy-paste Makefile from `nRF52840` with some changes
@ppannuto
Copy link
Member

ppannuto left a comment

Thanks for tracking this down!

# Upload programs over uart with tockloader
ifdef PORT
TOCKLOADER_GENERAL_FLAGS += --port $(PORT)
endif

This comment has been minimized.

@ppannuto

ppannuto Jul 7, 2018

Member

Should we make this general / included as an option on all boards?

This comment has been minimized.

@alevy

alevy Jul 7, 2018

Member

I vote no.

I'm personally fairly resistant to how much board specific stuff is getting pushed into the the common Makefile. It really makes maintaining a separate board without checking Tock out as a subrepository just for the Makefiles kind of un-tennable (and I think using LLVM's tools that come with Rust will help with that significantly). There aren't other boards in upstream that use JLink anyway.

This comment has been minimized.

@bradjc

bradjc Jul 9, 2018

Contributor

Is there a lot of board-specific content in the makefile? PLATFORM and TARGET are passed in as variables, but otherwise it is generic assuming rust can compile to your target and objcopy can create a valid binary file.

This comment has been minimized.

@alevy

alevy Jul 9, 2018

Member

It's not terrible right now, though it's still more complex than I would like (again, I suspect switching to LLVM will let us simplify the common Makefile significantly).

Adding tockloader stuff to the common Makefile feels board specific to me---not all boards will use tockloader, and certainly most early ones won't. If tockloader is that simple to use (I think it's either there or getting there for the boards it supports), it should be at least as easy and readable to write the one liner tockloader rule directly in the board Makefile as it is to rely on a particular structure and API that the common Makefile exports.

That's all. And it's really just a feeling. Like, had the PR added this option to the common Makefile instead of the board Makefile I likely wouldn't have objected (if I even noticed).

@alevy

alevy approved these changes Jul 7, 2018

@alevy alevy merged commit c7f008a into master Jul 7, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@alevy alevy deleted the fix-build-nrf52dk-nrf51dk branch Jul 7, 2018

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