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

Restructure programmers by removing hardcoded build.openocdscript #636

Merged
merged 1 commit into from
Oct 20, 2021
Merged

Restructure programmers by removing hardcoded build.openocdscript #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions platform.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,22 +198,22 @@ tools.openocd.cmd.windows=bin/openocd.exe

tools.openocd.upload.params.verbose=-d2
tools.openocd.upload.params.quiet=-d0
tools.openocd.upload.pattern="{path}/{cmd}" {upload.verbose} -s "{path}/share/openocd/scripts/" -f "{runtime.platform.path}/variants/{build.variant}/{build.openocdscript}" -c "telnet_port disabled; program {{build.path}/{build.project_name}.bin} verify reset 0x2000; shutdown"
tools.openocd.upload.pattern="{path}/{cmd}" {upload.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/at91samdXX.cfg" -c "telnet_port disabled; program {{build.path}/{build.project_name}.bin} verify reset 0x2000; shutdown"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it weird that this hardcodes the target configuration? Sounds like that should be board-configurable? Or is the samd core specific enough that all samd/samd21 boards it supports use the same openocd target?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, it may sound strange but all samd{11,21,51} are supported by the same configuration 🙂 . It doesn't apply to SAMC and SAML though (I didn't check if derived cores targeting these mcus exist). Anyway, moving to a property in boards.txt wouldn't hurt but could in fact break backwards compatibility with existing derived cores (due to the missing entry)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't you define a default platform-wide and have that (optionally) overridden by boards.txt? Not entirely sure if that's possible, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll test it and report back 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked and unfortunately the boards.txt entry doesn't override the default one 😞
Example patch (should fail but it doesn't)

diff --git a/boards.txt b/boards.txt
index 91adede0..2ebf09a9 100644
--- a/boards.txt
+++ b/boards.txt
@@ -111,6 +111,7 @@ mkr1000.build.vid=0x2341
mkr1000.build.pid=0x804e
mkr1000.bootloader.tool=openocd
mkr1000.bootloader.file=mkr1000/samd21_sam_ba_arduino_mkr1000.bin
+mkr1000.target=at91samdXXX.cfg

# Arduino MKRZero
# ---------------
diff --git a/platform.txt b/platform.txt
index 3df9b1ab..2bc409a4 100644
--- a/platform.txt
+++ b/platform.txt
@@ -192,20 +192,23 @@ tools.bossacI.upload.network_pattern="{network_cmd}" -address {serial.port} -por
# OpenOCD sketch upload
#

+# Can be overridden by boards.txt
+tools.openocd.target=at91samdXX.cfg
+
tools.openocd.path={runtime.tools.openocd-0.10.0-arduino7.path}
tools.openocd.cmd=bin/openocd
tools.openocd.cmd.windows=bin/openocd.exe

tools.openocd.upload.params.verbose=-d2
tools.openocd.upload.params.quiet=-d0
-tools.openocd.upload.pattern="{path}/{cmd}" {upload.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/at91samdXX.cfg" -c "telnet_port disabled; program {{build.path}/{build.project_name}.bin} verify reset 0x2000; shutdown"
+tools.openocd.upload.pattern="{path}/{cmd}" {upload.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/{target}" -c "telnet_port disabled; program {{build.path}/{build.project_name}.bin} verify reset 0x2000; shutdown"

tools.openocd.network_cmd={runtime.tools.arduinoOTA.path}/bin/arduinoOTA
tools.openocd.upload.network_pattern={network_cmd} -address {serial.port} -port 65280 -username arduino -password "{network.password}" -sketch "{build.path}/{build.project_name}.bin" -upload /sketch -b

tools.openocd.program.params.verbose=-d2
tools.openocd.program.params.quiet=-d0
-tools.openocd.program.pattern="{path}/{cmd}" {program.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/at91samdXX.cfg" -c "telnet_port disabled; program {{build.path}/{build.project_name}.hex} verify reset; shutdown"
+tools.openocd.program.pattern="{path}/{cmd}" {program.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/{target}" -c "telnet_port disabled; program {{build.path}/{build.project_name}.hex} verify reset; shutdown"

tools.openocd.erase.params.verbose=-d3
tools.openocd.erase.params.quiet=-d0
@@ -213,7 +216,7 @@ tools.openocd.erase.pattern=

tools.openocd.bootloader.params.verbose=-d2
tools.openocd.bootloader.params.quiet=-d0
-tools.openocd.bootloader.pattern="{path}/{cmd}" {bootloader.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/at91samdXX.cfg" -c "telnet_port disabled; init; halt; at91samd bootloader 0; program {{runtime.platform.path}/bootloaders/{bootloader.file}} verify reset; shutdown"
+tools.openocd.bootloader.pattern="{path}/{cmd}" {bootloader.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/{target}" -c "telnet_port disabled; init; halt; at91samd bootloader 0; program {{runtime.platform.path}/bootloaders/{bootloader.file}} verify reset; shutdown"

#
# OpenOCD sketch upload - version with configurable bootloader size


tools.openocd.network_cmd={runtime.tools.arduinoOTA.path}/bin/arduinoOTA
tools.openocd.upload.network_pattern={network_cmd} -address {serial.port} -port 65280 -username arduino -password "{network.password}" -sketch "{build.path}/{build.project_name}.bin" -upload /sketch -b

tools.openocd.program.params.verbose=-d2
tools.openocd.program.params.quiet=-d0
tools.openocd.program.pattern="{path}/{cmd}" {program.verbose} -s "{path}/share/openocd/scripts/" -f "{runtime.platform.path}/variants/{build.variant}/{build.openocdscript}" -c "telnet_port disabled; program {{build.path}/{build.project_name}.hex} verify reset; shutdown"
tools.openocd.program.pattern="{path}/{cmd}" {program.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/at91samdXX.cfg" -c "telnet_port disabled; program {{build.path}/{build.project_name}.hex} verify reset; shutdown"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the deal with telnet_port disabled and set telnet_port 0 here? Seems duplicate?

Copy link
Member Author

Choose a reason for hiding this comment

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

set telnet_port 0 was hidden in the variant configuration; it was necessary alongside telnet_port disabled due to this issue arduino/Arduino#4330 , so I'd leave it there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I'm a bit confused about that issue you linked. Sounds like that issue is really only about gdb_port not accepting disabled (but that was since fixed in openocd upstream, not sure if the Arduino version already updated to include that fix)? And it seems that disabled is the right value for all openocd versions (using 0 would, AFAICS, in older version be used as disabled, but in newer versions cause a random port to be used).

So are you sure that set telnet_port 0 is needed? I would think it never was really required, even?

Also, I can imagine that tcl_port disabled and (if openocd is sufficiently new) gdb_port disabled would make sense here as well? Or are we actually using the gdb_port over TCP now (as opposed to using it over stdin/stdout)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure it's still needed, the best way to check this would be trying to reproduce the issue but it's a bit impractical. About gdb_port disabled, I need to check how the debugging bridge is implemented in IDE 2.0 ; is it's over stdio we can safely add it.


tools.openocd.erase.params.verbose=-d3
tools.openocd.erase.params.quiet=-d0
tools.openocd.erase.pattern=

tools.openocd.bootloader.params.verbose=-d2
tools.openocd.bootloader.params.quiet=-d0
tools.openocd.bootloader.pattern="{path}/{cmd}" {bootloader.verbose} -s "{path}/share/openocd/scripts/" -f "{runtime.platform.path}/variants/{build.variant}/{build.openocdscript}" -c "telnet_port disabled; init; halt; at91samd bootloader 0; program {{runtime.platform.path}/bootloaders/{bootloader.file}} verify reset; shutdown"
tools.openocd.bootloader.pattern="{path}/{cmd}" {bootloader.verbose} -s "{path}/share/openocd/scripts/" -f "interface/{protocol}" -c "set telnet_port 0" {extra_params} -f "target/at91samdXX.cfg" -c "telnet_port disabled; init; halt; at91samd bootloader 0; program {{runtime.platform.path}/bootloaders/{bootloader.file}} verify reset; shutdown"

#
# OpenOCD sketch upload - version with configurable bootloader size
Expand Down
21 changes: 12 additions & 9 deletions programmers.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,24 @@

edbg.name=Atmel EDBG
edbg.communication=USB
edbg.protocol=
edbg.program.protocol=
edbg.protocol=cmsis-dap.cfg
edbg.program.tool=openocd
edbg.program.extra_params=
edbg.extra_params=

atmel_ice.name=Atmel-ICE
atmel_ice.communication=USB
atmel_ice.protocol=
atmel_ice.program.protocol=
atmel_ice.protocol=cmsis-dap.cfg
atmel_ice.program.tool=openocd
atmel_ice.program.extra_params=
atmel_ice.extra_params=

sam_ice.name=Atmel SAM-ICE
sam_ice.communication=USB
sam_ice.protocol=
sam_ice.program.protocol=
sam_ice.protocol=cmsis-dap.cfg
sam_ice.program.tool=openocd
sam_ice.program.extra_params=
sam_ice.extra_params=

jlink.name=Segger J-Link
jlink.communication=USB
jlink.protocol=jlink.cfg
jlink.program.tool=openocd
jlink.extra_params=-c "transport select swd"