openocd: Refactor openocd.sh script #7685

jnohlgard · 2017-10-06T13:04:12Z

Merge flash and flash-elf commands since they were identical except for the file name of the image
Split GDB command from DBG environment variable to allow more easily configure front-ends for GDB via environment variables.
Remove verbose tests of empty variables and replace by : ${VAR:=default}
Remove passed command line arguments to sub-functions, they were unused in the functions anyway.
Remove TUI variable, use export DBG_EXTRA_FLAGS=-tui to get the same result.
Remove intermediate reset halt between write_image and verify commands, fixes an issue with the watchdog interfering with the checksum computation on Kinetis K devices.

Tested with samr21-xpro and frdm-k22f.

jnohlgard · 2017-10-06T13:07:14Z

In particular, the default image file is changed from .hex to .elf. I don't know of any boards that do any special magic translations of the elf file when creating the hex. Flashing should work correctly for any board which have fully working GDB debugging through OpenOCD, because that means that the symbols are loading properly from the ELF file.

kYc0o · 2017-10-06T13:18:57Z

I like this PR! It comes in the moment I'm actually working on a solution to force all (openocd-flashed) boards to flash bin (easier to distribute for updates). My current solution is:

do_flash_bin() {
    test_config
    test_binfile
    if [ -n "${PRE_FLASH_CHECK_SCRIPT}" ]; then
        sh -c "${PRE_FLASH_CHECK_SCRIPT} '${HEXFILE}'"
        RETVAL=$?
        if [ $RETVAL -ne 0 ]; then
            echo "pre-flash checks failed, status=$RETVAL"
            exit $RETVAL
        fi
    fi
    # flash device
    sh -c "${OPENOCD} -f '${OPENOCD_CONFIG}' \
            ${OPENOCD_EXTRA_INIT} \
            -c 'tcl_port 0' \
            -c 'telnet_port 0' \
            -c 'gdb_port 0' \
            -c 'init' \
            -c 'targets' \
            -c 'reset halt' \
            ${OPENOCD_PRE_FLASH_CMDS} \
            -c 'program \"${BINFILE}\" \"${FLASH_ADDR}\"' \
            -c 'reset halt' \
            ${OPENOCD_PRE_VERIFY_CMDS} \
            -c 'verify_image \"${BINFILE}\" \"${FLASH_ADDR}\" bin' \
            -c 'reset run' \
            -c 'shutdown'" &&
    echo 'Done flashing'
}

Which can be also merged to the HEX or ELF solutions, except for the command, which is program instead of flash.

So, in summary, I like the work but I don't see a strong reason to flash always ELF, however I do agree that we don't need to flash HEX for openocd flashing.

jnohlgard · 2017-10-06T13:27:14Z

It should be easy to refactor this in a follow up to support setting the base address that you need for bin flashing.

My main motivation for this was to make it possible to run ddd automatically and to prepare for an upcoming separation of board configuration from debug interface adapter configuration, which will be useful to reduce code duplication in the repo, as well as being able to use any freestanding jtag/swd adapter for programming any board (especially useful for custom boards without integrated debug adapters)

jnohlgard · 2017-10-06T13:31:53Z

@kYc0o btw, you should not need to switch to the program command in openocd, it should be enough to put your flash base address as an extra argument after the file name to flash write_image.

kYc0o · 2017-10-06T15:34:35Z

@kYc0o btw, you should not need to switch to the program command in openocd, it should be enough to put your flash base address as an extra argument after the file name to flash write_image.

Ok I see, but then I need to provide also the type, which is not harmful... thanks for the hint!

So, In short, can we use this PR for the default debugging interface but maybe we use bin for the flash command? Are these two necessarily together? I think flashing bin and debugging with ELF is possible and would ease other procedures like bin file distribution.

kYc0o · 2017-10-06T15:57:16Z

dist/tools/openocd/openocd.sh

+# Default TCL port, set to 0 to disable
+: ${TCL_PORT:=6333}
+# Default path to OpenOCD configuration file
+: ${OPENOCD_CONFIG:=${RIOTBOARD}/${BOARD}/dist/openocd.cfg}


So here it shouldn't point to a generic configuration depending on the debugger?

Oops, sorry, I think this change would come in #7686 (still I don't see it there either)

In #7686, The config file is the configuration for the board, it should contain the CPU definitions and anything related to getting the board into a state ready to receive a new image. So basically everything except the jtag interface adapter definition, which is handled by the new $OPENOCD_IFACE_INIT (in #7686, not here)

jnohlgard · 2017-10-06T16:05:46Z

I don't like the idea of using bin as the default. Bin is just a raw memory dump with no extra information. Also since it's a raw dump it doesn't support memory holes. It is way easier to mess up a flash unintentionally when you don't have any header information for the image.
I don't mind providing bin as an option though.
How about this:

Rename ELFFILE -> IMAGE_FILE, in flashing only
Introduce IMAGE_OFFSET and IMAGE_TYPE, both optional, default is 0 for offset and empty string for type (means openocd will figure out from the file automatically)
Keep ELFFILE for debug target
Default value for IMAGE_FILE is $ELFFILE
Add file existence check for IMAGE_FILE in flashing only.

In a future PR:

Search the boards tree for HEXFILE definitions and remove them when they do not add any value to the board. I noticed that some boards seem to specify various special image builds even though they use the regular openocd script for flashing, I don't know why this was done so or if it's just a remnant from some ancient Makefile before the openocd tool usage was consolidated.

kYc0o · 2017-10-06T16:11:18Z

How about this:

Rename ELFFILE -> IMAGE_FILE, in flashing only
Introduce IMAGE_OFFSET and IMAGE_TYPE, both optional, default is 0 for offset and empty string for type (means openocd will figure out from the file automatically)
Keep ELFFILE for debug target
Default value for IMAGE_FILE is $ELFFILE
Add file existence check for IMAGE_FILE in flashing only.

It seems perfect to me, I didn't considered having a mess in the resulting binary since the goal is to generate it exactly in the same way as the HEX are generated (using objcopy), so for me it seemed legit. However I prefer you solution since it avoids any mistake if a different binary is loaded.

In a future PR:

Search the boards tree for HEXFILE definitions and remove them when they do not add any value to the board. I noticed that some boards seem to specify various special image builds even though they use the regular openocd script for flashing, I don't know why this was done so or if it's just a remnant from some ancient Makefile before the openocd tool usage was consolidated.

I'd like to do that job.

jnohlgard · 2017-10-13T08:07:56Z

@kYc0o
Added IMAGE_TYPE, IMAGE_FILE, IMAGE_OFFSET
Updated mulle to use the check elf-file script instead of hex. The other Kinetis boards already used the elf version.

kYc0o · 2017-10-13T10:13:52Z

Perfect, will test very soon.

kYc0o · 2017-10-16T15:32:17Z

dist/tools/openocd/openocd.sh

+# Image file used for flashing. Must be in a format that OpenOCD can handle (ELF,
+# Intel hex, S19, or raw binary)
+# Default is to use $ELFFILE
+: ${IMAGE_FILE:=${ELFFILE}}


:= means optional? Can it be overridden?

The construct : ${varname:=default_value} is a way to specify defaults for missing values. The colon operator is a no-op command in sh, and the := means that the variable will be updated with the value given if it is empty.
Another option is to write it as
varname = ${varname:default_value}

OK, I'm not familiar with sh, so thanks a lot from the explanation! My ACK remains.

kYc0o

ACK. Tested on several boards with several debuggers (OpenSDA, CMSIS-DAP, FT2232, ST-Link, JLink).

kYc0o · 2017-10-17T14:51:36Z

Please squash, I think this is ready.

jnohlgard · 2017-10-17T19:01:49Z

Will rebase tomorrow

jnohlgard · 2017-10-18T12:31:28Z

squashed

- Merge flash and flash-elf commands since they were identical except for the file name of the image - Split GDB command from DBG environment variable to allow more easily configure front-ends for GDB via environment variables. - Remove verbose tests of empty variables and replace by `: ${VAR:=default}` - Remove passed command line arguments to sub-functions, they were unused in the functions anyway. - Remove TUI variable, use `export DBG_EXTRA_FLAGS=-tui` to get the same result.

Fixes problems with the watchdog interfering on Kinetis K devices when USE_OLD_OPENOCD=0.

jnohlgard · 2017-10-18T12:35:21Z

Amended a small documentation error (ELFFILE vs. IMAGE_FILE in the docs for the flash command)

kYc0o · 2017-10-18T12:38:10Z

dist/tools/openocd/openocd.sh

@@ -147,40 +161,9 @@ do_flash() {
            -c 'targets' \
            -c 'reset halt' \
            ${OPENOCD_PRE_FLASH_CMDS} \
-            -c 'flash write_image erase \"${HEXFILE}\"' \
-            -c 'reset halt' \
+            -c 'flash write_image erase \"${IMAGE_FILE}\" ${IMAGE_OFFSET} ${IMAGE_TYPE}' \


A last nitpick... to avoid errors, shouldn't ${IMAGE_TYPE} be deduced from the ${IMAGE_FILE} value? because then this can allow to set ELFFILE bin. Though I think is not a blocker for merging as is.

Since we default to having IMAGE_TYPE empty, we defer image type deduction to OpenOCD by default. If a user does export IMAGE_TYPE=bin and IMAGE_FILE=myfile.elf, this is obviously going to cause a bricked board (sounds worse than it is, simply re-flashing with an empty IMAGE_TYPE will unbrick it).
In my opinion we can't protect the user from every possible configuration mistake they do, we should only make sure that the default configuration always works and that it is easy use the tool.
Can we leave any kind of image type deduction as a possible follow up and let others propose changes?
I don't know how we would do it in a good way other than using tools like file or readelf..

Yes I agree that's maybe too much overhead to avoid this kind of errors.

kYc0o · 2017-10-18T15:23:29Z

Let's go for #7686

jnohlgard · 2017-10-18T16:27:52Z

Thanks @kYc0o

jnohlgard added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tools Area: Supplementary tools labels Oct 6, 2017

jnohlgard added this to the Release 2017.10 milestone Oct 6, 2017

jnohlgard assigned haukepetersen Oct 6, 2017

jnohlgard requested a review from haukepetersen October 6, 2017 13:04

jnohlgard mentioned this pull request Oct 6, 2017

boards: board<->debugger separation #7686

Merged

kYc0o requested changes Oct 6, 2017

View reviewed changes

jnohlgard force-pushed the pr/openocd-refactor branch 4 times, most recently from f21b890 to f9eb678 Compare October 13, 2017 08:06

jnohlgard force-pushed the pr/openocd-refactor branch from f9eb678 to de2e59f Compare October 13, 2017 11:49

kYc0o reviewed Oct 16, 2017

View reviewed changes

kYc0o approved these changes Oct 16, 2017

View reviewed changes

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 16, 2017

jnohlgard force-pushed the pr/openocd-refactor branch from de2e59f to 365918a Compare October 18, 2017 12:28

Joakim Nohlgård added 2 commits October 18, 2017 14:34

makefiles: Split GDB settings from toolchain

0ad0a39

Joakim Nohlgård added 2 commits October 18, 2017 14:34

openocd: Run verify after write without intermediate reset

dd95f59

Fixes problems with the watchdog interfering on Kinetis K devices when USE_OLD_OPENOCD=0.

mulle: Update flash verify script to use ELF version

887ddce

jnohlgard force-pushed the pr/openocd-refactor branch from 365918a to 887ddce Compare October 18, 2017 12:34

kYc0o reviewed Oct 18, 2017

View reviewed changes

kYc0o merged commit d1f128e into RIOT-OS:master Oct 18, 2017

jnohlgard deleted the pr/openocd-refactor branch November 28, 2017 10:44

openocd: Refactor openocd.sh script #7685

openocd: Refactor openocd.sh script #7685

Uh oh!

Conversation

jnohlgard commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard commented Oct 6, 2017

Uh oh!

kYc0o commented Oct 6, 2017

Uh oh!

jnohlgard commented Oct 6, 2017

Uh oh!

jnohlgard commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kYc0o commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Oct 6, 2017

Uh oh!

kYc0o commented Oct 6, 2017

Uh oh!

jnohlgard commented Oct 13, 2017

Uh oh!

kYc0o commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Oct 17, 2017

Uh oh!

jnohlgard commented Oct 17, 2017

Uh oh!

jnohlgard commented Oct 18, 2017

Uh oh!

jnohlgard commented Oct 18, 2017

Uh oh!

kYc0o Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Oct 18, 2017

Uh oh!

jnohlgard commented Oct 18, 2017

Uh oh!

Uh oh!

jnohlgard commented Oct 6, 2017 •

edited

Loading

jnohlgard commented Oct 6, 2017 •

edited

Loading

kYc0o Oct 18, 2017 •

edited

Loading