dist/tools/openocd: add OPENOCD_EXTRA_INIT_RESET use for nucleo-f091rc

[fjmolinas]

Contribution description

When testing #11809 I realized that after flashing tests/ssp on nucleo-f091rc the board becomes un-flashable which caused all subsequent tests to fail. This can be avoided if connect_assert_srst is added to the reset configuration.

To avoid this also being called when debugging a new configuration parameter is introduced to only call this when reseting or flashing the board but not on debug actions. Since not all platforms support connect_assert_srst this is not used as default.

Testing procedure

Reproducing

To reproduce on master executing the following will fail on the second try:

make -C tests/ssp/ BOARD=nucleo-f091rc flash

The following also fails

make -C tests/ssp/ BOARD=nucleo-f091rc flash reset

Testing

With this PR it succeeds. Also the following succeeds:

2x make -C tests/ssp/ BOARD=nucleo-f091rc flash

make -C tests/ssp/ BOARD=nucleo-f091rc flash reset

make -C tests/xtimer_usleep/ BOARD=nucleo-f091rc flash debug

Issues/PRs references

Related to #8976, #10479

[fjmolinas]

@cladmi I would appreciate your input on this.

[fjmolinas]

Talking with @aabadie IRL, OPENOCD_EXTRA_INIT_RESET could be generalized for all stm32 platforms since this problems is present in many boards.

[cladmi]

Ohhh, interesting way of handling this.

You basically solved the issue that when using connect_assert_srst you cannot attach a debugger anymore on a running system as it would only do it for flash and reset.

Only thing with the implementation is that it breaks the boundary that the openocd low level configuration is in the .cfg file and not in the build system.
As here the reset_config arguments are really board hardware/flasher specific.

As you looked a bit more in the low level openocd configurations, do you think the same behavior could be achieved from a configuration file automatically only on flash and reset ?
Or if not, save the reset_config arguments in a variable/function and from the command line you would do -c use_reset_config_with_connect_assert_srst().
It would keep the build system to depend on an interface without the low level details leaking through.

[cladmi]

It may be easier than I expected :)
I thought reset_config always needed to have all the arguments when called but it seems you can just toggle one flag.

reset_config [none|trst_only|srst_only|trst_and_srst]                                                                                                                                                                                                                                     
          [srst_pulls_trst|trst_pulls_srst|combined|separate]                                                                                                                                                                                                                             
          [srst_gates_jtag|srst_nogate] [trst_push_pull|trst_open_drain]                                                                                                                                                                                                                  
          [srst_push_pull|srst_open_drain]                                                                                                                                                                                                                                                
          [connect_deassert_srst|connect_assert_srst]               

I removed the connect_assert_srst from the iotlab-m3 board and tested
changing only connect_assert_srst back and forth:

openocd -d0 -f boards/iotlab-m3/dist/openocd.cfg  -c 'echo "Current reset_config value"' -c reset_config -c 'reset_config connect_assert_srst' -c 'reset_config connect_deassert_srst' -c shutdown
GNU MCU Eclipse 64-bit Open On-Chip Debugger 0.10.0+dev-00462-gdd1d90111 (2019-01-18-11:37)
Licensed under GNU GPL v2
For bug reports, read
        http://openocd.org/doc/doxygen/bugs.html
debug_level: 0
adapter speed: 1000 kHz
adapter_nsrst_delay: 100
jtag_ntrst_delay: 100
none separate
cortex_m reset_config sysresetreq
trst_and_srst separate srst_nogate trst_push_pull srst_open_drain connect_deassert_srst
Current reset_config value
trst_and_srst separate srst_nogate trst_push_pull srst_open_drain connect_deassert_srst
trst_and_srst separate srst_nogate trst_push_pull srst_open_drain connect_assert_srst
trst_and_srst separate srst_nogate trst_push_pull srst_open_drain connect_deassert_srst

If reset_config is set properly in the configuration, so for your board with srst_only, OPENOCD_EXTRA_INIT_RESET could be replaced by OPENOCD_RESET_USE_CONNECT_ASSERT_SRST=1 that would add -c 'reset_config connect_assert_srst'.

I will do more extensive tests tomorrow.

[MrKevinWeiss]

Yup this is a common issue. Do we make it recover from a unflashable state or allow debug connect on running system. There has already been some connect_assert_srst in some boards. I though it would be good to have a flag like @cladmi suggested so you can opt-in to resets.

[fjmolinas]

If reset_config is set properly in the configuration, so for your board with srst_only, OPENOCD_EXTRA_INIT_RESET could be replaced by OPENOCD_RESET_USE_CONNECT_ASSERT_SRST=1 that would add -c 'reset_config connect_assert_srst'.

So you would do something like this?

if [ -n "${OPENOCD_RESET_USE_CONNECT_ASSERT_SRST}" ]; then
   OPENOCD_EXTRA_INIT+="-c 'reset_config connect_assert_srst'"
fi

I'm not sure I understand correctly the implementation change you are looking for. @cladmi

[cladmi]

If reset_config is set properly in the configuration, so for your board with srst_only, OPENOCD_EXTRA_INIT_RESET could be replaced by OPENOCD_RESET_USE_CONNECT_ASSERT_SRST=1 that would add -c 'reset_config connect_assert_srst'.

So you would do something like this?

if [ -n "${OPENOCD_RESET_USE_CONNECT_ASSERT_SRST}" ]; then
   OPENOCD_EXTRA_INIT+="-c 'reset_config connect_assert_srst'"
fi

I'm not sure I understand correctly the implementation change you are looking for. @cladmi

Exactly, that is the behavior I am thinking about :)

On the implementation maybe checking for 1 instead of not empty, and not sure if it does not need an extra space but that's details.

[fjmolinas]

Exactly, that is the behavior I am thinking about :)

This would have to be duplicated onder do_flash and do_reset in openocd.sh though.

[cladmi]

Exactly, that is the behavior I am thinking about :)

This would have to be duplicated onder do_flash and do_reset in openocd.sh though.

Or use an intermediate global OPENOCD_EXTRA_INIT_RESET intermediate variable, but indeed.

What I did not like was having the srst_only in the Makefile.
We could keep the reset_config connect_assert_srst in the Makefile but I would be worried it would end up being used to pass any random other things there, instead of adding a dedicated handling.

[fjmolinas]

Or use an intermediate global OPENOCD_EXTRA_INIT_RESET intermediate variable, but indeed.

Ok this seems better, to me.

[cladmi]

I did not manage to test yet on other boards but it is still in my plans.

[cladmi]

I tried with my pba-d-01-kw2x as a replacement of "boards/pba-d-01-kw2x: fix flashing from invalid state #11549" and without the srst_only I get a

Error: BUG: can't assert SRST

With the srst_only, I can ignore the srst_nogate that I had there as it is also enabled at the same time.

I will try with my other boards from #10870

[cladmi]

I also checked, with srst_only it does not flash without the connect_assert_srst…
EDIT: it can flash when it could flash in riot with only reset_config srst_only so this could be in the RIOT configuration. (I was trying when it was in the 'locked' state).

[MrKevinWeiss]

Did you try reset_config none separate?

[cladmi]

reset_config none separate is the default for the pba-d-01-kw2x in RIOT as shown by openocd when flashing:

adapter speed: 1000 kHz
none separate
cortex_m reset_config sysresetreq

And if you flash tests/driver_adt7310 2 times, you cannot flash anymore #11549

However when testing I did a mistake and tried to flash with srst_only when the broken firmware was in place… that's why it failed, not because of srst_only missing connect_assert_srst. (will update my comment)

I can correctly flash hello-world multiple times when I have reset_config srst_only.

[cladmi]

Talking with @aabadie IRL, OPENOCD_EXTRA_INIT_RESET could be generalized for all stm32 platforms since this problems is present in many boards.

This would give me the same behaviour as what I was doing in #11550 for the stm32f3discovery in a different way, so +1 for a generalized solution.

For the iotlab-m3 it needs another configuration in openocd as it uses reset_config trst_and_srst but it would work to put the connect_assert_srst in the makefile. I do not have any failing examples to indeed make it in a non flashable state though.

For

[cladmi]

I also tested my change from 900f6b8 and the difference from the reset configuration with master is only connect_assert_srst (as shown by openocd) so this OPENOCD_RESET_USE_CONNECT_ASSERT_SRST would also work for me there.

frdm-k64f  master srst_only separate srst_nogate srst_open_drain connect_deassert_srst
frdm-k64f  commit srst_only separate srst_nogate srst_open_drain connect_assert_srst
frdm-kw41z master srst_only separate srst_nogate srst_open_drain connect_deassert_srst
frdm-kw41z commit srst_only separate srst_nogate srst_open_drain connect_assert_srst

I did not test USE_OLD_OPENOCD case as the boards do not support it.

[cladmi]

For completeness, I tried on nrf52dk, and connect_assert_srst would just be ignored by openocd. reset_config would stay at none separate.
So it would not even hurt to have it there, even if doing nothing.

[fjmolinas]

@cladmi rebased to fix conflict issues. I also added your suggested changes. This will need squashing to split adding OPENOCD_RESET_USE_CONNECT_ASSERT_SRST and OPENOCD_EXTRA_INIT_RESET into separate commits.

[MrKevinWeiss]

Retested the nucleo-f091rc on the CI rack and everything seems good, it locks up in master and is fixed with this PR. If you want you can squash? I am a bit curious so know if the OPENOCD_RESET_USE_CONNECT_ASSERT_SRST can be overwritten?

[cladmi]

The OPENOCD_EXTRA_INIT_RESET must also be used when probing the flash to get the address in _flash_list_raw() (so for the first call).
(I noticed I did not use OPENOCD_EXTRA_INIT there, but only pba-d-01-kw2x uses it, I will refactor it to use OPENOCD_ADAPTER_INIT).

I think you can already squash the PR as it changed a lot. It will be easier to review anyway.

I can test it again this afternoon.

[cladmi]

Retested the nucleo-f091rc on the CI rack and everything seems good, it locks up in master and is fixed with this PR. If you want you can squash? I am a bit curious so know if the OPENOCD_RESET_USE_CONNECT_ASSERT_SRST can be overwritten?

It could be overwritten by doing make reset flash OPENOCD_RESET_USE_CONNECT_ASSERT_SRST=0 but currently not from the environment. It would indeed be better to allow setting it from environment.

[smlng]

Just to let you know: we tested on our HIL, this definitely fixes the flashing problem. So would be nice to get this in quickly, however I don't have any opinion regarding the implementation/integration into our build system and tools.

I trust @cladmi on this.

[fjmolinas]

@MrKevinWeiss @cladmi Squashed, OPENOCD_RESET_USE_CONNECT_ASSERT_SRST is now overwridable from the environment.

OPENOCD_RESET_USE_CONNECT_ASSERT_SRST=0 make -C tests/ssp/ BOARD=nucleo-f091rc reset

make: Entering directory '/home/francisco/workspace/RIOT/tests/ssp'
/home/francisco/workspace/RIOT/dist/tools/openocd/openocd.sh reset
### Resetting Target ###
Open On-Chip Debugger 0.10.0+dev-00703-g92bb76a4-dirty (2019-07-19-14:27)
Licensed under GNU GPL v2
For bug reports, read
        http://openocd.org/doc/doxygen/bugs.html
WARNING: interface/stlink-v2-1.cfg is deprecated, please switch to interface/stlink.cfg
hla_swd
Info : The selected transport took over low-level target control. The results might differ compared to plain JTAG/SWD
adapter speed: 1000 kHz
adapter_nsrst_delay: 100
none separate
srst_only separate srst_nogate srst_open_drain connect_deassert_srst
Info : clock speed 1000 kHz
Info : STLINK V2J23M9 (API v2) VID:PID 0483:374B
Info : Target voltage: 3.260407
Error: init mode failed (unable to connect to the target)

OPENOCD_RESET_USE_CONNECT_ASSERT_SRST=1 make -C tests/ssp/ BOARD=nucleo-f091rc reset

/home/francisco/workspace/RIOT/dist/tools/openocd/openocd.sh reset
### Resetting Target ###
Open On-Chip Debugger 0.10.0+dev-00703-g92bb76a4-dirty (2019-07-19-14:27)
Licensed under GNU GPL v2
For bug reports, read
        http://openocd.org/doc/doxygen/bugs.html
WARNING: interface/stlink-v2-1.cfg is deprecated, please switch to interface/stlink.cfg
hla_swd
Info : The selected transport took over low-level target control. The results might differ compared to plain JTAG/SWD
adapter speed: 1000 kHz
adapter_nsrst_delay: 100
none separate
srst_only separate srst_nogate srst_open_drain connect_deassert_srst
srst_only separate srst_nogate srst_open_drain connect_assert_srst
Info : clock speed 1000 kHz
Info : STLINK V2J23M9 (API v2) VID:PID 0483:374B
Info : Target voltage: 3.263559
Info : stm32f0x.cpu: hardware has 4 breakpoints, 2 watchpoints
Info : Listening on port 38441 for gdb connections
Info : Unable to match requested speed 1000 kHz, using 950 kHz
Info : Unable to match requested speed 1000 kHz, using 950 kHz
adapter speed: 950 kHz
shutdown command invoked

[fjmolinas]

@cladmi comments addressed.

[cladmi]

Good for me, I re-tested my pba pr rebased on the last version and works like a charm :)

[fjmolinas]

@MrKevinWeiss @cladmi All green, do you have anymore comments?

[cladmi]

It's good for me (but I am not allowed to ACK).

[MrKevinWeiss]

I tested before and if @cladmi tested the overwrite then I will ACK.

[cladmi]

The overwrite is only about defining the variable with ?= in the board makefile.

[cladmi]

@fjmolinas Thank you for finding the right way of handling this connect_assert_srst that does not change debug.
I am updating all my PRs that rely on this.

• TODO: update all your boards that were overwriting reset_config and had a special handling for this.
  • iotlab-m3 boards/iotlab: rely on OPENOCD_RESET_USE_CONNECT_ASSERT_SRST #12153

git grep OPENOCD_DBG_START_CMD '**' ':!dist'
boards/b-l072z-lrwan1/Makefile.include:export OPENOCD_DBG_START_CMD = -c 'reset halt'
boards/common/iotlab/Makefile.include:export OPENOCD_DBG_START_CMD = -c 'reset halt'
boards/common/stm32f103c8/Makefile.include:  export OPENOCD_DBG_START_CMD = -c 'reset halt'
boards/i-nucleo-lrwan1/Makefile.include:export OPENOCD_DBG_START_CMD = -c 'reset halt'
boards/lsn50/Makefile.include:export OPENOCD_DBG_START_CMD = -c 'reset halt'

[fjmolinas]

@MrKevinWeiss @smlng @cladmi Thanks for the review and testing!

[cladmi]

Is this not funny when you think about it?

A PR open for almost one month, no reviewer reviewing it.

Me, contributor, as I like this change, spent several hours in the manual, testing the implementation, doing local tests of openocd, challenging with the other boards that had similar fix, test my own PRs that addressed the same issue in the old way based on this change, rebase my PRs on it, give feedback on the implementation, the limitation, the documentation.

Then, as it is discovered today that it is needed for a board in the CI, it "would be nice to be in quickly", so gets merged based on maintainers testing only the fixed board, not the whole feature and concept, and on "I trust this contributor".
Not even taking the responsibility of the review, the required effort for doing it, or even acknowledging the previous feedback.

"I want it all, and I want it now"

If it was that urgent, adding at least a "thank you, you saved our ass" would have been welcomed.

[aabadie]

A PR open for almost one month, no reviewer reviewing it.

Well, I have to say that I was interested by this PR and with @fjmolinas we discussed it IRL as he mentioned at the beginning of the discussion. It's just that vacations came in between.

But what I find funny with your comment, is that you (@cladmi) consider yourself as a simple contributor just because you decided (I still don't understand why honestly) to resign from the maintainer status. But we all know here that you were maintainer not so long ago and already did (a lot of) good reviews and merged quite some stuff. And that's why people trust a not so simple contributor like you.
So, do you want to become a maintainer again ?

[smlng]

@cladmi you got that a bit wrong.

1. Nope not funny, rather that is how things sometimes work out. In this case: we were working on extending our test setup with more boards before the summit. More or less by chance we picked the Nucleo-l091rc and encountered flashing problems in a few cases (not all the time). We also found this PR ran it through our setup, it helped with the Nucleo and didn't break any other board or something else. So we reported our findings here, with the intention that some additional tests results might be nice too (also in light of the recent discussion of "untested ACK").

2. I didn't have time nor the expertise in the build/make system of RIOT to do any meaningful in-depth/conceptual review right now. Hence, I trusted you and the discussion you already had with @fjmolinas to bring this into (mergeable) shape ... but I still wanted to contribute the test results we had.

3. My "would be nice to get this in quickly" was not meant to imply any urgency (or "save-my-a**" situation). I looked over this PR and saw it adds only a few lines, also there was a discussion between you and @fjmolinas sorting things out and several tests showed positiv results. Hence, with +/- 600 open PRs, this looked like an easy one to get of the stack.
   Anyway, next time I'll try to watch my wording to avoid such misunderstandings.

4. Again to be clear: this PR did not "save us" (or our test setup). Actually such things are exactly why we are building the test infrastructure: to find bugs and issues - not only with RIOT but also with its build and tool system around.

5. Regarding the timing: @aabadie already made the point, that this was opened during summer vacation time which certainly had its share in delaying progress on many PRs - not just this one.

Btw. I don't remember ever getting a thank you for a PR I made; for reviews and testing -- yes, many times. Generally I don't deem it necessary, but appreciated it is.

TL;DR: thanks @fjmolinas and @cladmi

[MrKevinWeiss]

Thanks @cladmi 😄
You always know they right thing to say 😉

[cladmi]

3\. I looked over this PR and saw it adds only a few lines, also there was a discussion between you and @fjmolinas sorting things out and several tests showed positiv results. Hence, with +/- 600 open PRs, this looked like an easy one to get of the stack.

That is exactly why I complain about not getting an explicit review.

This is not a simple change! Francisco managed to be so right and precise with this fix.

Maybe it end up in only a few makefile/shell lines, but the important part is not the integration but the openocd part, what it solves, how, why it works, which impact it will have. What were we doing before and why were we not doing this?
What impact does it have on #8976 and #10479 ?

Does it deprecate the 5 other handling in RIOT ?
#11976 (comment)

I personally rebased 3 of my PRs on this one, open one to use this and added a cleanup/bug fix.

Btw. I don't remember ever getting a thank you for a PR I made; for reviews and testing -- yes, many times. Generally I don't deem it necessary, but appreciated it is.

It was not a thing when I started, but at least all the last ones with you have :) https://github.com/RIOT-OS/RIOT/pulls?q=is%3Apr+reviewed-by%3Asmlng+author%3Acladmi+is%3Amerged

A PR open for almost one month, no reviewer reviewing it.

Well, I have to say that I was interested by this PR and with @fjmolinas we discussed it IRL as he mentioned at the beginning of the discussion. It's just that vacations came in between.

It was to put it in contrast with the really quick merge with no maintainer explicit review. I know you were both in vacation.

But what I find funny with your comment, is that you (@cladmi) consider yourself as a simple contributor just because you decided (I still don't understand why honestly) to resign from the maintainer status. But we all know here that you were maintainer not so long ago and already did (a lot of) good reviews and merged quite some stuff. And that's why people trust a not so simple contributor like you.

Trust is unfortunately not a review. I do not trust what I do.
I trust that I do mistakes all the time and try to compensate by giving enough noise around so they can be found.

And yes, I am a contributor and should be considered as such.

As long as a review can be done without following https://github.com/RIOT-OS/RIOT/blob/master/MAINTAINING.md I am better as a contributor.

(btw, if I am in a state where a review is not necessary for my contributions, I have many PRs waiting :p)