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.

Sign up for GitHub

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

Jump to bottom

tools/edbg: update to latest upstream version #13233

Merged
merged 2 commits into from
Jan 31, 2020
Merged

tools/edbg: update to latest upstream version #13233

merged 2 commits into from
Jan 31, 2020

Conversation

benpicco
Copy link
Contributor

Contribution description

Upstream edbg changed the target names (they already generated a deprecation warning before).
While updating this I moved the cpu -> target name mapping to edbg-devices.inc.mk and removed the EDBG_DEVICE_TYPE declaration from the board Makefiles.

I chose to generate a mapping even for MCUs that are not supported by RIOT yet, this allowed for automated generation of the file and no further work should we gain support for these processors in the future.

Testing procedure

You should still be able to flash the boards as before.
I tested this on same54-xpro and samr21-xpro.

Issues/PRs references

none

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tools Area: Supplementary tools Area: boards Area: Board ports labels Jan 29, 2020
aabadie
aabadie reviewed Jan 29, 2020
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Looks good, I'll test on saml11-xpro and arduino-zero tomorrow.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 29, 2020
kaspar030
kaspar030 reviewed Jan 29, 2020
View reviewed changes
makefiles/tools/edbg-devices.inc.mk
@@ -0,0 +1,142 @@
# Atmel SAM C/D/L/R series:
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks inefficient, all findstrings will always be checked.
Maybe link them with else?

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the board should still pick this. Typically a board knows its CPU_MODEL, right?

Copy link
Contributor Author

@benpicco benpicco Jan 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO, the board should still pick this.

No, that's what I was trying to get rid of.
The board already sets CPU_MODEL - that should be enough.
There is no need to further duplicate that information if it can be avoided.

this looks inefficient, all findstrings will always be checked. Maybe link them with else?

I can sprinkle some else in to speed up make flash 😉
But I doubt that it makes much of a difference.

Copy link
Contributor

@kaspar030 kaspar030 Jan 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

But I doubt that it makes much of a difference.

I couldn't measure a difference, so probably it doesn't matter. But it looks inefficient, especially most of the device types are not even supported. Maybe we just uncomment those?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we just uncomment those?

... and add an error case (`ifeq (, $(EDBG_DEVICE_TYPE)) $(error ...))...```

dylad
dylad reviewed Jan 30, 2020
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

SAML10, SAML11, SAML21 are happy with the new edbg version. Work as expected.
I don't have opinion about dropping EDBG_DEVICE_TYPE from the board side. Does it impact that much the build system ?

@aabadie
Copy link
Contributor

aabadie commented Jan 30, 2020

It works like a charm on arduino-zero, samr30-xpro and samr34-xpro.

@kaspar030 kaspar030 added CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 30, 2020
@kaspar030
Copy link
Contributor

Hm, the CI raspi use a precompiled edbg version. We have a chicken-and-egg problem here.

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 30, 2020
@kaspar030
Copy link
Contributor

We have a chicken-and-egg problem here.

Nice, the old options are still accepted. I've updated edbg on the CI workers, let's see.

@kaspar030
Copy link
Contributor

I've updated edbg on the CI workers, let's see.

unrelated build errors. CI wise, this is fine now.

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 30, 2020
@dylad
Copy link
Member

dylad commented Jan 31, 2020

SAME54-XPRO is also happy with the new version.
@kaspar030 Are you happy with the state of this PR ?

@kaspar030
Copy link
Contributor

@kaspar030 Are you happy with the state of this PR ?

yes, let's go. please squash!

@benpicco
tools/edbg: update to latest upstream version
baea542 
This version has changed the target names, so add a edbg-devices.inc.mk
to automatically select the right target.
@benpicco
boards: remove EDBG_DEVICE_TYPE
bd19a55 
EDBG_DEVICE_TYPE is a property of the CPU, is should not be set
by every board individually.
@dylad dylad added this to the Release 2020.04 milestone Jan 31, 2020
@dylad dylad merged commit d929087 into RIOT-OS:master Jan 31, 2020
@benpicco benpicco deleted the edbg-update branch January 31, 2020 10:48
miri64 added a commit to miri64/RIOT that referenced this pull request Jan 31, 2020
@miri64
fixup! Merge pull request RIOT-OS#13233 from benpicco/edbg-update
56f1c27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@aabadie aabadie aabadie left review comments

@dylad dylad dylad left review comments

@kaspar030 kaspar030 kaspar030 approved these changes

@keestux keestux Awaiting requested review from keestux

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
5 participants
@benpicco @aabadie @kaspar030 @dylad @miri64