dist/tools/openocd: add the possibility to do a reset before halt in openocd debug command by aabadie · Pull Request #8856 · RIOT-OS/RIOT

aabadie · 2018-03-30T10:30:44Z

Contribution description

This PR tries to address this comment by using an environment variable to trigger a reset before halt in the openocd debug command.

It's required for the b-l072z-lrwan1 board (at least), so the config of this board is updated accordingly.

Issues/PRs references

Addresses the same issue than #8790 but in a cleaner way.

aabadie · 2018-03-30T10:35:11Z

Maybe @gebart or @kYc0o could also help here.

kYc0o

It looks legit, but needs some adaptation IMHO.

kYc0o · 2018-04-09T13:04:00Z

boards/b-l072z-lrwan1/Makefile.include

 export STLINK_VERSION ?= 2-1

+# do a reset before halt when starting debugger
+export PRE_DEBUG_RESET_HALT ?= 1


I think the variable should be declared somewhere more general, then set it to one here. Maybe makefiles/vars.inc.mk ?

The idea is to check if the variable exists in openocd.sh. So one has just to declare it to enable the reset before halt in debug command.

Well I guess this anyways it will be refactored for all the ST boards embedding a ST-Link debugger, so it's fine for now.

this anyways it will be refactored for all the ST boards embedding a ST-Link debugger

Are you referring to #8475 ?

Yes, I think a global variable for this debugger should be available for all boards embedding it.

kYc0o · 2018-04-09T13:05:57Z

dist/tools/openocd/openocd.sh

+    if [ -n "${PRE_DEBUG_RESET_HALT}" ]; then
+        PRE_RESET_HALT='reset'
+    fi
+


I think instead of doing this you can evaluate the variable directly on the code, since it's used only there. Each command anyways gets executed separately.

I don't understand this comment. PRE_RESET_HALT is only meant for the debug command.

I mean you can do the check inline in the code below, no need to declare a variable and then do the check.

To be more clear, to reuse the variable you just exported before.

I prefer keeping it this way. The command prefix is defined before, so there's no inline bash code added when the openocd command is built. I think it's nicer the way it's done here.

I mean that you can use your first variable directly in the shell script. You might need to change the name though, something like PRE_DEBUG_COND and then add something like PRE_DEBUG_COND += reset or whatever is needed, in the Makefile.

But in this case it has to be defined globally and that's exactly what I wanted to avoid.

By the way, I changed the previous code by using a local variable. Still works.

But in this case it has to be defined globally and that's exactly what I wanted to avoid.

All the variables that are used in this script are global, they are available because they're exported since this script is out of the scope of the Makefiles. Thus, there's no real concept of "global" variable here, all of them are.

aabadie · 2018-04-12T14:07:30Z

dist/tools/openocd/openocd.sh

    test_elffile
+
+    # Configure halt command: prepend a reset if required by the board config
+    local _pre_reset_halt=''


@kYc0o, that's why I changed my code so that now it updates a local variable by checking if the global variable PRE_DEBUG_RESET_HALT is set.

By the way, this strategy was inspired by the use of PRE_FLASH_CHECK_SCRIPT. The idea is exactly the same.

By the way, this strategy was inspired by the use of PRE_FLASH_CHECK_SCRIPT. The idea is exactly the same.

That's a condition to execute a whole script and analysing its output, not just setting a variable. What I'm trying to avoid here is the declaration of variables and conditions which are IMHO not necessary, and by simply declaring the variable in makefiles/vars.inc.mk and setting it in your board Makefile it's enough. It's even more flexible since for other boards (which we might not be aware of yet) other commands might be necessary, thus you can just set this variable for your needs e.g. PRE_DEBUG_RESET_HALT += reset halt reset if you need to reset twice or whatever.

Looks like we disagree

cladmi

I would like to have a namespaced variable and homogenous in the way OPENOCD_..._CMDS work in flash.

cladmi · 2018-04-12T17:02:15Z

dist/tools/openocd/openocd.sh

+
+    # Configure halt command: prepend a reset if required by the board config
+    local _pre_reset_halt=''
+    if [ -n "${PRE_DEBUG_RESET_HALT}" ]; then


For me, the PRE here is wrong, because it changes the command from halt to reset halt it's not a previous command. Also, introducing a new really specific variable only for this specific way of handling the case seems bad to me.

I would prefer the following at the beginning of openocd.sh

# A meaningful comment on what this is :p : ${OPENOCD_EXTRA_INIT:=-c 'halt'}

And in the given board set

export OPENOCD_DEBUG_HALT_CMD = -c 'reset halt'

aabadie · 2018-04-13T05:35:47Z

@cladmi, I re-read the openocd commands documentation.
You are right thathalt and reset halt are 2 different commands: the former just halt the target (which doesn't work with stm32l0 when used with some reset_options) while the latter performs a reset and then a halt (this is why I used PRE prefix).
Now I'm wondering if halt should just be replaced by reset halt like I did initially since reset halt seems more robust.

aabadie · 2018-04-13T06:18:17Z

Now I'm wondering if halt should just be replaced by reset halt like I did initially since reset halt seems more robust.

I tested on an Arduino Zero board and the current debug target doesn't work for me with just halt, reset halt works. samr21-xpro might have the same problem.
I'm using openocd 0.10 (master version). Can someone confirm ?

aabadie · 2018-04-13T06:56:52Z

@cladmi @kYc0o I updated the code following your suggestions. Let's move forward.

cladmi · 2018-04-18T12:40:18Z

dist/tools/openocd/openocd.sh

+# Initial target state when using debug, by default a 'halt' request is sent to
+# the target when starting a debug session. 'reset halt' can also be used
+# depending on the type of target.
+: ${OPENOCD_EXTRA_DEBUG_INIT_CMD:=-c 'halt'}


Some naming remarks, in that case it is not an EXTRA command like the other one as it's mandatory, so this EXTRA part should be removed.
To match your comment, maybe put more START_CMD as the name INIT means something with openocd (reset init, init)

Also I would prefer it defined along the DBG variables, but that's personal taste.

cladmi · 2018-04-18T12:41:58Z

boards/b-l072z-lrwan1/Makefile.include

 export DEBUG_ADAPTER ?= stlink
 export STLINK_VERSION ?= 2-1

+# do a reset before halt when starting debugger


Same here, it is not a reset before halt but a reset halt openocd command.
http://openocd.org/doc/html/General-Commands.html#resetcommand

Indeed "The optional parameter specifies what should happen after the reset" so there's a reset before halt (halt is an optional parameter) :)

aabadie · 2018-04-18T14:51:21Z

Renamed and moved the variable as suggested by @cladmi

cladmi

ACK

I checked the output difference between iotlab-m3 and b-l072z-lrwan1 by doing sh -xc in the debug function.

For the fact that it's working I trust you.

Please squash.

cladmi · 2018-04-18T15:32:16Z

@aabadie Reviewing this PR made me find something you will like :) I will do a PR

aabadie · 2018-04-18T15:34:26Z

For the fact that it's working I trust you.

It does work :)

Please squash.

Done

cladmi · 2018-04-20T09:22:20Z

@kYc0o do you ACK ?

kYc0o

Excellent! ACK.

kYc0o · 2018-04-20T09:30:36Z

I'd like to change the title though, it doesn't really reflect what it was done here.

aabadie · 2018-04-24T08:58:00Z

I'd like to change the title though, it doesn't really reflect what it was done here.

Again, it does, see this comment.

kYc0o · 2018-04-24T10:43:53Z

IMHO you're not adding the possibility to make a reset before halt, but rather you add a different command called reset halt. So maybe something like "openocd: replace halt command by OPENOCD_DBG_START_CMD", or something like that. Also, I'd say you should squash your commits into one, to allow a proper bisect if needed.

aabadie · 2018-04-24T11:05:25Z

Again, it does, see this comment.

Have you read the documentation ? I can put what I wrote there (again):

"The optional parameter specifies what should happen after the reset"

so there's a reset before halt since halt is the optional parameter when using "reset halt".

Also, I'd say you should squash your commits into one, to allow a proper bisect if needed.

I don't understand the purpose of your comment. The first commit updates the openocd script by introducing a parametrizable variable. The second commit set this variable to what works with the b-l072z-lrwan1 board. The 2 commits touch 2 different things (tools and boards) and this is very common practice into RIOT.

kYc0o · 2018-04-24T12:05:54Z

Well, I still don't agree about the title (I had problems to recognise what actually PRs are doing by the title, even merging something that was already PRed before just because the title was misleading), but let's go. About the commits is ok because they are in the good order, I just want to enforce working commits because I also recently ran in a big hassle doing bisect.

dist/tools/openocd: add the possibility to do a reset before halt in openocd debug command

aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools Area: boards Area: Board ports labels Mar 30, 2018

aabadie requested a review from miri64 March 30, 2018 10:30

aabadie mentioned this pull request Mar 30, 2018

boards/b-l072z-lrwan1: fix openocd configuration and 'make debug' #8790

Merged

kYc0o requested changes Apr 9, 2018

View reviewed changes

aabadie force-pushed the pr/openocd_debug_reset branch from c91540a to a60fc8c Compare April 11, 2018 13:28

aabadie commented Apr 12, 2018

View reviewed changes

cladmi suggested changes Apr 12, 2018

View reviewed changes

miri64 removed their request for review April 12, 2018 18:00

aabadie force-pushed the pr/openocd_debug_reset branch from 5c33b4c to 47ddc1c Compare April 13, 2018 06:34

cladmi reviewed Apr 18, 2018

View reviewed changes

aabadie force-pushed the pr/openocd_debug_reset branch from ada64e1 to e87aa52 Compare April 18, 2018 14:52

cladmi approved these changes Apr 18, 2018

View reviewed changes

aabadie added 2 commits April 18, 2018 17:33

dist/tools/openocd: make reset before halt option in debug

2172074

boards/b-l072z-lrwan1: enable reset before halt in debug

ea255f5

aabadie force-pushed the pr/openocd_debug_reset branch from e87aa52 to ea255f5 Compare April 18, 2018 15:33

This was referenced Apr 18, 2018

dist/tools/openocd: cannot debug some stlink based boards #8975

Closed

dist/tools/openocd: use reset halt command for board using stlink #8976

Closed

kYc0o approved these changes Apr 20, 2018

View reviewed changes

kYc0o merged commit 4e872e6 into RIOT-OS:master Apr 24, 2018

aabadie mentioned this pull request Apr 25, 2018

boards/iotlab-m3|a8: fix openocd configuration #8977

Merged

maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request May 8, 2018

Merge pull request RIOT-OS#8856 from aabadie/pr/openocd_debug_reset

5efa90a

dist/tools/openocd: add the possibility to do a reset before halt in openocd debug command

aabadie deleted the pr/openocd_debug_reset branch June 1, 2018 18:58

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

aabadie commented Mar 30, 2018

Contribution description

Issues/PRs references

Uh oh!

aabadie commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kYc0o left a comment

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 Apr 9, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

aabadie Apr 10, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Apr 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi Apr 12, 2018 • 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!

aabadie commented Apr 13, 2018

Uh oh!

aabadie commented Apr 13, 2018

Uh oh!

aabadie commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Apr 18, 2018 • 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!

aabadie commented Apr 18, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

aabadie commented Mar 30, 2018 •

edited

Loading

kYc0o Apr 9, 2018 •

edited

Loading

aabadie Apr 10, 2018 •

edited

Loading

aabadie Apr 12, 2018 •

edited

Loading

cladmi Apr 12, 2018 •

edited

Loading

aabadie Apr 18, 2018 •

edited

Loading