-
Notifications
You must be signed in to change notification settings - Fork 2k
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
boards: remove exports of jlink related variables #13567
boards: remove exports of jlink related variables #13567
Conversation
c6117da
to
3bffd4b
Compare
3bffd4b
to
6cfe1e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. I tested on the following boards, and everything works like in current master:
- sensebox
- ruuvitag (tested RTT terminal)
- nrf52dk
- slstk3402a
Maybe it would be nice to have further testing before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good cleanup, there is an unrelated change in here though. @leandrolanzieri provided testing, I think there is no need for testing all the boards and the variables are still exported for all debugger related targets, IMO this is good to go. Please remove the unrelated changes and squash.
makefiles/tools/jlink.inc.mk
Outdated
@@ -12,6 +12,24 @@ RESET_FLAGS ?= reset | |||
|
|||
JLINK_SERIAL ?= $(DEBUG_ADAPTER_ID) | |||
|
|||
JLINK_IF ?= SWD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this default value? wasn't the case before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default in jlink.sh
:
RIOT/dist/tools/jlink/jlink.sh
Line 70 in 741b9d3
_JLINK_IF=SWD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although why set it again then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it was simpler this way but I'll change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
6cfe1e9
to
1b0b2c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested:
- hifiv1b
- nrf52dk
- nrf52840dk
- slstk3402a
While all connected on the same machine.
ACK! Please squash @aabadie.
This variable is already export in vars.inc.mk
8aceb6e
to
ad5b9d7
Compare
Done (this one was not easy!) |
Contribution description
This PR removes all uses of
export
with JLink related variables and move the export injlink.inc.mk
. The variables are also now only exported with jlink related targets.Static tests are also added to prevent future unwanted exports of theses variables.
Testing procedure
flash
,reset
,debug
still works on affected boards.nrf52840dk
hifive1b
make term
still works with board usingstdio_rtt
(could not test this)Issues/PRs references
Related to #10850