Skip to content

cpu/sam0-common: rename mkr ldscript to more a generic name#7646

Merged
jnohlgard merged 2 commits intoRIOT-OS:masterfrom
aabadie:samd21_bootloader
Oct 10, 2017
Merged

cpu/sam0-common: rename mkr ldscript to more a generic name#7646
jnohlgard merged 2 commits intoRIOT-OS:masterfrom
aabadie:samd21_bootloader

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Sep 26, 2017

This is because this ldscript can be used with any samd21 with an Arduino compatible bootloader preflashed. The idea is just to use a more generic name for the ldscript file.

This is the case for all Arduino MKR boards, Adafruit Feather boards (see #7510) and others.

This PR should address the question of #7645.

@aabadie aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Sep 26, 2017
@aabadie aabadie added this to the Release 2017.10 milestone Sep 26, 2017
@aabadie aabadie requested a review from jnohlgard September 26, 2017 07:42
@aabadie aabadie mentioned this pull request Sep 26, 2017
Closed
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 26, 2017
@aabadie aabadie mentioned this pull request Sep 27, 2017
Merged
@ynezz
Copy link
Copy Markdown
Contributor

ynezz commented Sep 28, 2017 •
edited
Loading

The idea is just to use a more generic name for the ldscript file.

Small nitpick :-) From my point of view, it's not generic linker script, it's linker script which targets SAMD21G18A MCU with Arduino bootloader pre-flashed, so probably the more appropriate linker filename would be samd21g18a_arduino_bootloader.ld ?

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Sep 28, 2017

probably the more appropriate linker filename would be samd21g18a_arduino_bootloader.ld ?

Good point, I buy it. Thanks!

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Sep 28, 2017 •
edited
Loading

using the suffix _bootloader is a bit ambiguous. I mean it can also be interpreted as "a linker script to use when building a bootloader" instead of "a linker script to use when running the application from a bootloader".

Edit: To be constructive: I suggest adding a README.md file to the ldscripts directory to explain which script should be used for what.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Sep 28, 2017 •
edited
Loading

To be constructive: I suggest adding a README.md file to the ldscripts directory to explain which script should be used for what.

Added and comments welcome.

@aabadie aabadie force-pushed the samd21_bootloader branch 2 times, most recently from 14fb08c to 0aef67d Compare September 30, 2017 15:27
jnohlgard
jnohlgard approved these changes Oct 4, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I don't have the hardware to test it.

cpu/sam0_common/ldscripts/README.md Outdated
that starts after a preflashed Arduino bootloader. The firmware is copied to
the flash memory using [Bossa](https://github.com/shumatech/BOSSA).
This is the kind of configuration used with Arduino MKR and Adafruit Feather
M0 boards. No newline at end of file
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Oct 4, 2017

Choose a reason for hiding this comment

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

Missing newline at end of file

Copy link
Copy Markdown
Contributor Author

@aabadie aabadie Oct 4, 2017

Choose a reason for hiding this comment

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

Fixed and directly squashed.

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Oct 4, 2017

Thanks for adding the README

@aabadie aabadie force-pushed the samd21_bootloader branch from 0aef67d to eb0c537 Compare October 4, 2017 19:38
@aabadie aabadie force-pushed the samd21_bootloader branch from eb0c537 to 282f691 Compare October 7, 2017 21:07
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Oct 10, 2017

It would be nice to merge this one, it's required for #7510.

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Oct 10, 2017

I did not test on actual hardware, but this is only a rename and should be working on real hardware if the build succeeded and it worked before.

@jnohlgard jnohlgard merged commit 7c1c6ac into RIOT-OS:master Oct 10, 2017
@aabadie aabadie deleted the samd21_bootloader branch February 26, 2018 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard approved these changes

Assignees

@jnohlgard jnohlgard

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2017.10

Development

Successfully merging this pull request may close these issues.

3 participants

@aabadie @ynezz @jnohlgard