[riot-notifications] [RIOT-OS/RIOT] kinetis/fcfield-check*: merge into single file (#11589)

Gaëtan Harter notifications at github.com
Mon May 27 17:00:35 CEST 2019


cladmi requested changes on this pull request.



> +fi
+
+if [ $# -ne 1 ]; then
+    echo "Usage: $0 FLASHFILE"
+    echo "Checks the flash configuration field protection bits to avoid flashing a locked image to a Kinetis MCU (protection against accidental bricking)."
+    exit 2
+fi
+
+FLASHFILE="$1"
+
+FCFIELD_START='0x400'
+FCFIELD_END='0x410'
+FCFIELD_AWK_REGEX='^ 0400 '
+
+if [ ${FLASHFILE##*.} = bin ]; then
+    OBJDUMP=$(arm-none-eabi-objdump --start-address=${FCFIELD_START} --stop-address=${FCFIELD_END} -bbinary -marm ${FLASHFILE} -s)


Better not use OBJDUMP as variable name here. In the build system OBJDUMP is the name of the tool, and I have a pull request to use OBJDUMP from the build system in this file :D (I will do it after this one).

I personally tend to not put commands output into a variable when possible (from this source, in French https://linuxfr.org/news/revue-des-techniques-de-programmation-en-shell). So would more refactor this in a function and use the output with get_fc_field "${FLASHFILE}" | awk.
The fact that you need to do printf %s ${OBJDUMP} somehow shows the pattern issue.

So at least rename it, and if you want put it in a function


> +if [ $# -ne 1 ]; then
+    echo "Usage: $0 FLASHFILE"
+    echo "Checks the flash configuration field protection bits to avoid flashing a locked image to a Kinetis MCU (protection against accidental bricking)."
+    exit 2
+fi
+
+FLASHFILE="$1"
+
+FCFIELD_START='0x400'
+FCFIELD_END='0x410'
+FCFIELD_AWK_REGEX='^ 0400 '
+
+if [ ${FLASHFILE##*.} = bin ]; then
+    OBJDUMP=$(arm-none-eabi-objdump --start-address=${FCFIELD_START} --stop-address=${FCFIELD_END} -bbinary -marm ${FLASHFILE} -s)
+elif [ ${FLASHFILE##*.} = elf ]; then
+    OBJDUMP=$(arm-none-eabi-objdump -j.fcfield -s "${ELFFILE}")

Here should be `FLASHFILE`.

> +    echo "Checks the flash configuration field protection bits to avoid flashing a locked image to a Kinetis MCU (protection against accidental bricking)."
+    exit 2
+fi
+
+FLASHFILE="$1"
+
+FCFIELD_START='0x400'
+FCFIELD_END='0x410'
+FCFIELD_AWK_REGEX='^ 0400 '
+
+if [ ${FLASHFILE##*.} = bin ]; then
+    OBJDUMP=$(arm-none-eabi-objdump --start-address=${FCFIELD_START} --stop-address=${FCFIELD_END} -bbinary -marm ${FLASHFILE} -s)
+elif [ ${FLASHFILE##*.} = elf ]; then
+    OBJDUMP=$(arm-none-eabi-objdump -j.fcfield -s "${ELFFILE}")
+elif [ ${FLASHFILE##*.} = hex ]; then
+    OBJDUMP=$(arm-none-eabi-objdump --start-address=${FCFIELD_START} --stop-address=${FCFIELD_END} ${HEXFILE} -s)

Here should be `FLASHFILE` too.

> @@ -0,0 +1,54 @@
+#!/bin/sh
+#
+# Flash configuration field check script for Freescale Kinetis MCUs.
+#
+# This script is supposed to be called from RIOTs
+# unified OpenOCD script (dist/tools/openocd/openocd.sh).
+#
+# Syntax: check-fcfield.sh $FLASHFILE
+#
+# @author       Jonas Remmert <j.remmert at phytec.de>
+# @author       Johann Fischer <j.fischer at phytec.de>
+# @author       Joakim Nohlgård <joakim.nohlgard at eistec.se>
+# @author       Francisco Molina <francisco.molina at inria.fr>
+
+if [ $(printf '%d' "${IMAGE_OFFSET}") -gt 1040 ] ; then

If you put this after defining `FCFIELD_END`, you can do a compare with the variable converted to int like for `IMAGE_OFFSET`. Otherwise the value is defined two times.

> +    exit 0
+fi
+
+if [ $# -ne 1 ]; then
+    echo "Usage: $0 FLASHFILE"
+    echo "Checks the flash configuration field protection bits to avoid flashing a locked image to a Kinetis MCU (protection against accidental bricking)."
+    exit 2
+fi
+
+FLASHFILE="$1"
+
+FCFIELD_START='0x400'
+FCFIELD_END='0x410'
+FCFIELD_AWK_REGEX='^ 0400 '
+
+if [ ${FLASHFILE##*.} = bin ]; then

I would prefer adding the `.bin` and `IMAGE_OFFSET` in a second commit, to show "this was the old behavior moved in the same file`, `this is adding `.bin` support.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11589#pullrequestreview-242315257
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190527/73b5e1ef/attachment-0001.html>


More information about the notifications mailing list