7
31
Fork
You've already forked blackmagic
23

gdb_main: move main loop into main() #1284

Merged
xobs merged 8 commits from main-loop-invert into main 2023年02月03日 16:25:19 +01:00
xobs commented 2022年11月13日 14:21:42 +01:00 (Migrated from github.com)
Copy link

Detailed description

This is a rewrite of the main loop as suggested in https://github.com/blackmagic-debug/blackmagic/pull/1281. Rather
than constantly sitting inside gdb_main(), the program's main loop does its own polling of the target. This enables
a host to do its own waiting without needing to insert hooks in various functions.

This prevents WDT triggers on ESP32 where RTT blocks the IDLE thread from running. The IDLE thread is responsible for resetting the WDT. It can also be used inside the main loop to regulate hosts such as PC_HOSTED where constant USB polling is undesirable.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do
## Detailed description This is a rewrite of the main loop as suggested in https://github.com/blackmagic-debug/blackmagic/pull/1281. Rather than constantly sitting inside `gdb_main()`, the program's main loop does its own polling of the target. This enables a host to do its own waiting without needing to insert hooks in various functions. This prevents WDT triggers on ESP32 where RTT blocks the IDLE thread from running. The IDLE thread is responsible for resetting the WDT. It can also be used inside the main loop to regulate hosts such as `PC_HOSTED` where constant USB polling is undesirable. ## Your checklist for this pull request * [x] I've read the [Code of Conduct](https://github.com/blackmagic-debug/blackmagic/blob/main/CODE_OF_CONDUCT.md) * [x] I've read the [guidelines for contributing](https://github.com/blackmagic-debug/blackmagic/blob/main/CONTRIBUTING.md) to this repository * [x] It builds for hardware native (`make PROBE_HOST=native`) * [x] It builds as BMDA (`make PROBE_HOST=hosted`) * [x] I've tested it to the best of my ability * [x] My commit messages provide a useful short description of what the commits do
xobs commented 2022年11月13日 14:24:35 +01:00 (Migrated from github.com)
Copy link

This has not been tested -- I have only confirmed that it builds, so there may be issues.

I've created this PR to get feedback, to ensure this is the approach you were describing in https://github.com/blackmagic-debug/blackmagic/pull/1281.

This has not been tested -- I have only confirmed that it builds, so there may be issues. I've created this PR to get feedback, to ensure this is the approach you were describing in https://github.com/blackmagic-debug/blackmagic/pull/1281.
koendv commented 2022年11月13日 15:45:54 +01:00 (Migrated from github.com)
Copy link

I've tried rtt on black pill v2 and hosted on arm64, seems ok. Flashing works, speed unchanged. Not bad.
How does it run on esp32? What's your opinion of the way the code looks now?

I've tried rtt on black pill v2 and hosted on arm64, seems ok. Flashing works, speed unchanged. Not bad. How does it run on esp32? What's your opinion of the way the code looks now?
xobs commented 2022年11月14日 14:45:18 +01:00 (Migrated from github.com)
Copy link

It seems to work fine. I started on a refactor of Farpatch to support multiple clients attaching at once, and nearly got something working. I backed out those changes for another day, but I can confirm that this approach allows me to insert delays as necessary. Overall, I think this approach is sound.

I am, however, concerned with the fact that RP2040 debugging appears to have broken slightly. Though if you're not seeing it on your target, then perhaps it's either something on my platform or something due to flash changes recently.

It seems to work fine. I started on a refactor of Farpatch to support multiple clients attaching at once, and nearly got something working. I backed out those changes for another day, but I can confirm that this approach allows me to insert delays as necessary. Overall, I think this approach is sound. I am, however, concerned with the fact that RP2040 debugging appears to have broken slightly. Though if you're not seeing it on your target, then perhaps it's either something on my platform or something due to flash changes recently.
koendv commented 2022年11月15日 09:01:39 +01:00 (Migrated from github.com)
Copy link

Glad to hear it works fine.

I think this also opens up the possibility of running bmp on a real-time
os.

Another possibility is running bmp and cmsis-dap on the same probe.
bmp on usb serial together with free-dap on usb hid works; the
equivalent for esp32 would be bmp on a tcp port and free-dap on
bluetooth hid. The advantage is changing between openocd and bmp
without having to change cables.

"slightly"? Could you be more specific about what you see on rp2040?

koen

Glad to hear it works fine. I think this also opens up the possibility of running bmp on a real-time os. Another possibility is running bmp and cmsis-dap on the same probe. bmp on usb serial together with free-dap on usb hid works; the equivalent for esp32 would be bmp on a tcp port and free-dap on bluetooth hid. The advantage is changing between openocd and bmp without having to change cables. "slightly"? Could you be more specific about what you see on rp2040? koen
xobs commented 2022年11月15日 09:53:03 +01:00 (Migrated from github.com)
Copy link

I just checked, and it seems that RP2040 support is just weird even on main. For example:

[4:47:17 pm] E:/Code/Farpatch/farpatch> & 'E:\Software\gcc6円 2017-q2-update\bin\arm-none-eabi-gdb.exe' mdi-gateway.elf
GNU gdb (GNU Tools for ARM Embedded Processors 6-2017-q2-update) 7.12.1.20170417-git
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "--host=i686-w64-mingw32 --target=arm-none-eabi".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from mdi-gateway.elf...done.
(gdb) tar ext \\.\COM54
Remote debugging using \\.\COM54
(gdb) mon s
Target voltage: 3.3V
Available Targets:
No. Att Driver
 1 Raspberry RP2040 M0+
 2 Raspberry RP2040 M0+
 3 Raspberry RP2040 Rescue (Attach to reset!)
(gdb) att 1
Attaching to program: E:\Code\Ion\mdi-gateway.elf, Remote target
0x10002340 in tud_suspended () at /opt/Pico/pico-sdk/lib/tinyusb/src/device/usbd.c:372
372 /opt/Pico/pico-sdk/lib/tinyusb/src/device/usbd.c: No such file or directory.
(gdb) load
Loading section .boot2, size 0x100 lma 0x10000000
Loading section .text, size 0x61e8 lma 0x10000100
Loading section .rodata, size 0x8b8 lma 0x100062e8
Loading section .binary_info, size 0x24 lma 0x10006ba0
Loading section .data, size 0x2a8 lma 0x10006bc4
Start address 0x100001e8, load size 28268
Transfer rate: 47 KB/sec, 883 bytes/write.
(gdb) c
Continuing.
Program received signal SIGINT, Interrupt.
0x102f29d0 in ?? ()
(gdb) run
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: mdi-gateway.elf
Program received signal SIGINT, Interrupt.
0x000020f0 in ?? ()
(gdb) bt
#0 0x000020f0 in ?? ()
#1 0x000002b2 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) run
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: mdi-gateway.elf
Program received signal SIGINT, Interrupt.
0x000020f0 in ?? ()
(gdb) bt
#0 0x000020f0 in ?? ()
#1 0x000002b2 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) b main
Breakpoint 1 at 0x100004b0: file main.c, line 59.
(gdb) run
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: mdi-gateway.elf
Note: automatically using hardware breakpoints for read-only addresses.
Program received signal SIGINT, Interrupt.
0x000020f0 in ?? ()
(gdb) x 0x10002000
0x10002000 <_hw_endpoint_buffer_control_update32+56>: 0x00000000
(gdb) disassemble main
Dump of assembler code for function main:
 0x100004b0 <+0>: movs r0, r0
 0x100004b2 <+2>: movs r0, r0
 0x100004b4 <+4>: movs r0, r0
 0x100004b6 <+6>: movs r0, r0
 0x100004b8 <+8>: movs r0, r0
 0x100004ba <+10>: movs r0, r0
 0x100004bc <+12>: movs r0, r0
 0x100004be <+14>: movs r0, r0
 0x100004c0 <+16>: movs r0, r0
 0x100004c2 <+18>: movs r0, r0
 0x100004c4 <+20>: movs r0, r0
 0x100004c6 <+22>: movs r0, r0
 0x100004c8 <+24>: movs r0, r0
 0x100004ca <+26>: movs r0, r0
 0x100004cc <+28>: movs r0, r0
 0x100004ce <+30>: movs r0, r0
 0x100004d0 <+32>: movs r0, r0
 0x100004d2 <+34>: movs r0, r0
 0x100004d4 <+36>: movs r0, r0
 0x100004d6 <+38>: movs r0, r0
 0x100004d8 <+40>: movs r0, r0
 0x100004da <+42>: movs r0, r0
 0x100004dc <+44>: movs r0, r0
 0x100004de <+46>: movs r0, r0
 0x100004e0 <+48>: movs r0, r0
 0x100004e2 <+50>: movs r0, r0
 0x100004e4 <+52>: movs r0, r0
 0x100004e6 <+54>: movs r0, r0
 0x100004e8 <+56>: movs r0, r0
 0x100004ea <+58>: movs r0, r0
 0x100004ec <+60>: movs r0, r0
 0x100004ee <+62>: movs r0, r0
 0x100004f0 <+64>: movs r0, r0
 0x100004f2 <+66>: movs r0, r0
 0x100004f4 <+68>: movs r0, r0
 0x100004f6 <+70>: movs r0, r0
 0x100004f8 <+72>: movs r0, r0
 0x100004fa <+74>: movs r0, r0
 0x100004fc <+76>: movs r0, r0
 0x100004fe <+78>: movs r0, r0
 0x10000500 <+80>: movs r0, r0
 0x10000502 <+82>: movs r0, r0
 0x10000504 <+84>: movs r0, r0
 0x10000506 <+86>: movs r0, r0
 0x10000508 <+88>: movs r0, r0
 0x1000050a <+90>: movs r0, r0
 0x1000050c <+92>: movs r0, r0
 0x1000050e <+94>: movs r0, r0
 0x10000510 <+96>: movs r0, r0
 0x10000512 <+98>: movs r0, r0
End of assembler dump.
(gdb) p main
1ドル = {int (void)} 0x100004b0 <main>
(gdb) x main
0x100004b0 <main>: 0x00000000
(gdb) load
Loading section .boot2, size 0x100 lma 0x10000000
Loading section .text, size 0x61e8 lma 0x10000100
Loading section .rodata, size 0x8b8 lma 0x100062e8
Loading section .binary_info, size 0x24 lma 0x10006ba0
Loading section .data, size 0x2a8 lma 0x10006bc4
Start address 0x100001e8, load size 28268
Transfer rate: 106 KB/sec, 883 bytes/write.
(gdb) p main
2ドル = {int (void)} 0x100004b0 <main>
(gdb) x 0x100004b0
0x100004b0 <main>: 0x00000000
(gdb) mon erase_mass
Erasing device Flash: done
(gdb) mon ver
Black Magic Probe v1.8.0-709-g5092cd14, Hardware Version 6
Copyright (C) 2022 Black Magic Debug Project
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
(gdb)

From this point, the board has stopped responding. Examining memory returns all zeroes. Doing an erase_mass returns immediately rather than performing a mass erase. The board doesn't actually work unless I hard power-cycle it.

It's just coincidence that it's the board I was testing my patches with. Perhaps it's damaged. At any rate, the STM32G49 test board I've got works just fine under both JTAG and SWD, so I'd consider the rpi weirdness a separate issue.

I just checked, and it seems that RP2040 support is just weird even on `main`. For example: ``` [4:47:17 pm] E:/Code/Farpatch/farpatch> & 'E:\Software\gcc6円 2017-q2-update\bin\arm-none-eabi-gdb.exe' mdi-gateway.elf GNU gdb (GNU Tools for ARM Embedded Processors 6-2017-q2-update) 7.12.1.20170417-git Copyright (C) 2017 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "--host=i686-w64-mingw32 --target=arm-none-eabi". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from mdi-gateway.elf...done. (gdb) tar ext \\.\COM54 Remote debugging using \\.\COM54 (gdb) mon s Target voltage: 3.3V Available Targets: No. Att Driver 1 Raspberry RP2040 M0+ 2 Raspberry RP2040 M0+ 3 Raspberry RP2040 Rescue (Attach to reset!) (gdb) att 1 Attaching to program: E:\Code\Ion\mdi-gateway.elf, Remote target 0x10002340 in tud_suspended () at /opt/Pico/pico-sdk/lib/tinyusb/src/device/usbd.c:372 372 /opt/Pico/pico-sdk/lib/tinyusb/src/device/usbd.c: No such file or directory. (gdb) load Loading section .boot2, size 0x100 lma 0x10000000 Loading section .text, size 0x61e8 lma 0x10000100 Loading section .rodata, size 0x8b8 lma 0x100062e8 Loading section .binary_info, size 0x24 lma 0x10006ba0 Loading section .data, size 0x2a8 lma 0x10006bc4 Start address 0x100001e8, load size 28268 Transfer rate: 47 KB/sec, 883 bytes/write. (gdb) c Continuing. Program received signal SIGINT, Interrupt. 0x102f29d0 in ?? () (gdb) run The program being debugged has been started already. Start it from the beginning? (y or n) y Starting program: mdi-gateway.elf Program received signal SIGINT, Interrupt. 0x000020f0 in ?? () (gdb) bt #0 0x000020f0 in ?? () #1 0x000002b2 in ?? () Backtrace stopped: previous frame identical to this frame (corrupt stack?) (gdb) run The program being debugged has been started already. Start it from the beginning? (y or n) y Starting program: mdi-gateway.elf Program received signal SIGINT, Interrupt. 0x000020f0 in ?? () (gdb) bt #0 0x000020f0 in ?? () #1 0x000002b2 in ?? () Backtrace stopped: previous frame identical to this frame (corrupt stack?) (gdb) b main Breakpoint 1 at 0x100004b0: file main.c, line 59. (gdb) run The program being debugged has been started already. Start it from the beginning? (y or n) y Starting program: mdi-gateway.elf Note: automatically using hardware breakpoints for read-only addresses. Program received signal SIGINT, Interrupt. 0x000020f0 in ?? () (gdb) x 0x10002000 0x10002000 <_hw_endpoint_buffer_control_update32+56>: 0x00000000 (gdb) disassemble main Dump of assembler code for function main: 0x100004b0 <+0>: movs r0, r0 0x100004b2 <+2>: movs r0, r0 0x100004b4 <+4>: movs r0, r0 0x100004b6 <+6>: movs r0, r0 0x100004b8 <+8>: movs r0, r0 0x100004ba <+10>: movs r0, r0 0x100004bc <+12>: movs r0, r0 0x100004be <+14>: movs r0, r0 0x100004c0 <+16>: movs r0, r0 0x100004c2 <+18>: movs r0, r0 0x100004c4 <+20>: movs r0, r0 0x100004c6 <+22>: movs r0, r0 0x100004c8 <+24>: movs r0, r0 0x100004ca <+26>: movs r0, r0 0x100004cc <+28>: movs r0, r0 0x100004ce <+30>: movs r0, r0 0x100004d0 <+32>: movs r0, r0 0x100004d2 <+34>: movs r0, r0 0x100004d4 <+36>: movs r0, r0 0x100004d6 <+38>: movs r0, r0 0x100004d8 <+40>: movs r0, r0 0x100004da <+42>: movs r0, r0 0x100004dc <+44>: movs r0, r0 0x100004de <+46>: movs r0, r0 0x100004e0 <+48>: movs r0, r0 0x100004e2 <+50>: movs r0, r0 0x100004e4 <+52>: movs r0, r0 0x100004e6 <+54>: movs r0, r0 0x100004e8 <+56>: movs r0, r0 0x100004ea <+58>: movs r0, r0 0x100004ec <+60>: movs r0, r0 0x100004ee <+62>: movs r0, r0 0x100004f0 <+64>: movs r0, r0 0x100004f2 <+66>: movs r0, r0 0x100004f4 <+68>: movs r0, r0 0x100004f6 <+70>: movs r0, r0 0x100004f8 <+72>: movs r0, r0 0x100004fa <+74>: movs r0, r0 0x100004fc <+76>: movs r0, r0 0x100004fe <+78>: movs r0, r0 0x10000500 <+80>: movs r0, r0 0x10000502 <+82>: movs r0, r0 0x10000504 <+84>: movs r0, r0 0x10000506 <+86>: movs r0, r0 0x10000508 <+88>: movs r0, r0 0x1000050a <+90>: movs r0, r0 0x1000050c <+92>: movs r0, r0 0x1000050e <+94>: movs r0, r0 0x10000510 <+96>: movs r0, r0 0x10000512 <+98>: movs r0, r0 End of assembler dump. (gdb) p main 1ドル = {int (void)} 0x100004b0 <main> (gdb) x main 0x100004b0 <main>: 0x00000000 (gdb) load Loading section .boot2, size 0x100 lma 0x10000000 Loading section .text, size 0x61e8 lma 0x10000100 Loading section .rodata, size 0x8b8 lma 0x100062e8 Loading section .binary_info, size 0x24 lma 0x10006ba0 Loading section .data, size 0x2a8 lma 0x10006bc4 Start address 0x100001e8, load size 28268 Transfer rate: 106 KB/sec, 883 bytes/write. (gdb) p main 2ドル = {int (void)} 0x100004b0 <main> (gdb) x 0x100004b0 0x100004b0 <main>: 0x00000000 (gdb) mon erase_mass Erasing device Flash: done (gdb) mon ver Black Magic Probe v1.8.0-709-g5092cd14, Hardware Version 6 Copyright (C) 2022 Black Magic Debug Project License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> (gdb) ``` From this point, the board has stopped responding. Examining memory returns all zeroes. Doing an `erase_mass` returns immediately rather than performing a mass erase. The board doesn't actually work unless I hard power-cycle it. It's just coincidence that it's the board I was testing my patches with. Perhaps it's damaged. At any rate, the STM32G49 test board I've got works just fine under both JTAG and SWD, so I'd consider the rpi weirdness a separate issue.

That behaviour indicates issues with getting the RP2040 to run its exit-XIP and enter-XIP sequences (rp_flash_prepare, rp_flash_resume). It's possible this is a bug in your RP2040's ROM routines, or it could indicate an issue with BMP's own implementations of those same routines..

There are two #if 0 blocks in the RP2040 support routines guarding rp_flash_* routines, if you remove the preprocessor magic, and replace the prepare and resume routines' calls to the ROM table with:

rp_flash_connect_internal(t);
rp_flash_exit_xip(t);

and

rp_flash_flush_cache(t);
rp_flash_enter_xip(t);

for prepare and resume respectively.. please let us know how that goes in a follow-up issue report

That behaviour indicates issues with getting the RP2040 to run its exit-XIP and enter-XIP sequences (rp_flash_prepare, rp_flash_resume). It's possible this is a bug in your RP2040's ROM routines, or it could indicate an issue with BMP's own implementations of those same routines.. There are two `#if 0` blocks in the RP2040 support routines guarding `rp_flash_*` routines, if you remove the preprocessor magic, and replace the prepare and resume routines' calls to the ROM table with: ```c rp_flash_connect_internal(t); rp_flash_exit_xip(t); ``` and ```c rp_flash_flush_cache(t); rp_flash_enter_xip(t); ``` for prepare and resume respectively.. please let us know how that goes in a follow-up issue report
xobs commented 2022年11月18日 05:36:48 +01:00 (Migrated from github.com)
Copy link

I think this is what you were looking for:

diff --git a/src/target/rp.c b/src/target/rp.c
index c27a0a58..06b8ec6e 100644
--- a/src/target/rp.c
+++ b/src/target/rp.c
@@ -207,7 +207,7 @@ static bool rp_mass_erase(target *t);
 // Our own implementation of bootloader functions for handling flash chip
 static void rp_flash_exit_xip(target *const t);
 static void rp_flash_enter_xip(target *const t);
-#if 0
+#if 1
 static void rp_flash_connect_internal(target *const t);
 static void rp_flash_flush_cache(target *const t);
 #endif
@@ -225,6 +225,7 @@ static void rp_add_flash(target *t)
 return;
 }
+ rp_flash_connect_internal(t);
 rp_flash_exit_xip(t);
 spi_parameters_s spi_parameters;
@@ -236,6 +237,7 @@ static void rp_add_flash(target *t)
 spi_parameters.sector_erase_opcode = SPI_FLASH_CMD_SECTOR_ERASE;
 }
+ rp_flash_flush_cache(t);
 rp_flash_enter_xip(t);
 DEBUG_INFO("Flash size: %uMiB\n", spi_parameters.capacity / (1024U * 1024U));
@@ -612,7 +614,7 @@ static void rp_spi_read(
 target_mem_write32(t, RP_SSI_ENABLE, ssi_enabled);
 }
-#if 0
+#if 1
 // Connect the XIP controller to the flash pads
 static void rp_flash_connect_internal(target *const t)
 {
@@ -769,7 +771,7 @@ static void rp_flash_exit_xip(target *const t)
 target_mem_write32(t, RP_GPIO_QSPI_CS_CTRL, 0);
 }
-#if 0
+#if 1
 // This is a hook for steps to be taken in between programming the flash and
 // doing cached XIP reads from the flash. Called by the bootrom before
 // entering flash second stage, and called by the debugger after flash

However, it's still behaving Oddly. Since this is orthogonal to this patch, I can move this into another issue and we can discuss it there.

Is there anything else that needs to happen in order to merge this change?

I think this is what you were looking for: ```patch diff --git a/src/target/rp.c b/src/target/rp.c index c27a0a58..06b8ec6e 100644 --- a/src/target/rp.c +++ b/src/target/rp.c @@ -207,7 +207,7 @@ static bool rp_mass_erase(target *t); // Our own implementation of bootloader functions for handling flash chip static void rp_flash_exit_xip(target *const t); static void rp_flash_enter_xip(target *const t); -#if 0 +#if 1 static void rp_flash_connect_internal(target *const t); static void rp_flash_flush_cache(target *const t); #endif @@ -225,6 +225,7 @@ static void rp_add_flash(target *t) return; } + rp_flash_connect_internal(t); rp_flash_exit_xip(t); spi_parameters_s spi_parameters; @@ -236,6 +237,7 @@ static void rp_add_flash(target *t) spi_parameters.sector_erase_opcode = SPI_FLASH_CMD_SECTOR_ERASE; } + rp_flash_flush_cache(t); rp_flash_enter_xip(t); DEBUG_INFO("Flash size: %uMiB\n", spi_parameters.capacity / (1024U * 1024U)); @@ -612,7 +614,7 @@ static void rp_spi_read( target_mem_write32(t, RP_SSI_ENABLE, ssi_enabled); } -#if 0 +#if 1 // Connect the XIP controller to the flash pads static void rp_flash_connect_internal(target *const t) { @@ -769,7 +771,7 @@ static void rp_flash_exit_xip(target *const t) target_mem_write32(t, RP_GPIO_QSPI_CS_CTRL, 0); } -#if 0 +#if 1 // This is a hook for steps to be taken in between programming the flash and // doing cached XIP reads from the flash. Called by the bootrom before // entering flash second stage, and called by the debugger after flash ``` However, it's still behaving Oddly. Since this is orthogonal to this patch, I can move this into another issue and we can discuss it there. Is there anything else that needs to happen in order to merge this change?

That is indeed, please do move that into a new issue ticket. A trace of what you're seeing would be fantastic to help debug but otherwise we mostly need to recreate your situation to see what's going on

That is indeed, please do move that into a new issue ticket. A trace of what you're seeing would be fantastic to help debug but otherwise we mostly need to recreate your situation to see what's going on
xobs commented 2022年11月18日 13:12:03 +01:00 (Migrated from github.com)
Copy link

I opened https://github.com/blackmagic-debug/blackmagic/issues/1295 which is at least rather reproducible

I opened https://github.com/blackmagic-debug/blackmagic/issues/1295 which is at least rather reproducible
xobs commented 2022年11月18日 13:38:03 +01:00 (Migrated from github.com)
Copy link

Rebased to latest main to keep it mergable.

Rebased to latest `main` to keep it mergable.
dragonmux left a comment
Copy link

There are a couple of times we found on a cursory initial code review. We'll dive deeper once the cleanup work is all merged

There are a couple of times we found on a cursory initial code review. We'll dive deeper once the cleanup work is all merged

This file is missing a newline at the end, which is what the "no entry" sign represents, please add the missing newline

This file is missing a newline at the end, which is what the "no entry" sign represents, please add the missing newline
@ -21,6 +21,10 @@
#ifndef INCLUDE_GDB_MAIN_H

Please use size_t, not int here. -Wconversion would show up that you're converting between the two in the call in main() and with what size represents, size_t is the more correct type.

Please use size_t, not int here. `-Wconversion` would show up that you're converting between the two in the call in main() and with what size represents, `size_t` is the more correct type.
@ -28,6 +28,40 @@
#include "gdb_packet.h"
#include "morse.h"

Please remove the braces here. Likewise in the if statement below

Please remove the braces here. Likewise in the if statement below

The inner parentheses here suppress a GCC warning that we want enabled here, please remove them as well as the braces around the if body

The inner parentheses here suppress a GCC warning that we want enabled here, please remove them as well as the braces around the if body
xobs (Migrated from github.com) reviewed 2022年11月19日 04:16:16 +01:00
@ -28,6 +28,40 @@
#include "gdb_packet.h"
#include "morse.h"
xobs (Migrated from github.com) commented 2022年11月19日 04:16:16 +01:00
Copy link

The braces appear necessary here, because on hosted SET_IDLE_STATE(x) is defined as an empty macro.

If you like, I can redefine SET_IDLE_STATE(x) to be do {} while(0) on hosted, which would get around that error...

The braces appear necessary here, because on hosted `SET_IDLE_STATE(x)` is defined as an empty macro. If you like, I can redefine `SET_IDLE_STATE(x)` to be `do {} while(0)` on hosted, which would get around that error...
xobs (Migrated from github.com) reviewed 2022年11月19日 04:19:29 +01:00
@ -28,6 +28,40 @@
#include "gdb_packet.h"
#include "morse.h"
xobs (Migrated from github.com) commented 2022年11月19日 04:19:29 +01:00
Copy link

I went ahead and added that in c86e7964304845fc062749b94b3d3e9bc3ba0467 which fixes this on hosted.

I went ahead and added that in c86e7964304845fc062749b94b3d3e9bc3ba0467 which fixes this on `hosted`.
@ -28,6 +28,40 @@
#include "gdb_packet.h"
#include "morse.h"

ah, right.. that mess - yes, good call to turn that into a do {} while(0)

ah, right.. that mess - yes, good call to turn that into a `do {} while(0)`
xobs commented 2022年11月20日 03:53:37 +01:00 (Migrated from github.com)
Copy link

Rebased to latest main to keep it mergable.

Rebased to latest `main` to keep it mergable.
xobs commented 2022年11月29日 16:49:58 +01:00 (Migrated from github.com)
Copy link

Rebased to merge latest changes on main

Rebased to merge latest changes on `main`
xobs commented 2022年12月29日 02:47:11 +01:00 (Migrated from github.com)
Copy link

Rebased to fix conflicts with latest main

Rebased to fix conflicts with latest `main`
xobs commented 2023年01月01日 17:16:36 +01:00 (Migrated from github.com)
Copy link

Rebased again to fix conflicts with latest main

Rebased again to fix conflicts with latest `main`

We will try to get this reviewed today now the v1.10 merge window is open

We will try to get this reviewed today now the v1.10 merge window is open
dragonmux left a comment
Copy link

There are a couple of stylistic/formatting things to resolve, and we'd like an explanation on one or two things the change set is doing so we can better understand it, but this is shaping up nicely. With the stylistic/formatting items resolved this looks ready to merge.

There are a couple of stylistic/formatting things to resolve, and we'd like an explanation on one or two things the change set is doing so we can better understand it, but this is shaping up nicely. With the stylistic/formatting items resolved this looks ready to merge.

In the interest of making sure someone changing cur_target, last_target, etc breaks the build rather than causing UB - it would make more sense to keep this include, no?

In the interest of making sure someone changing cur_target, last_target, etc breaks the build rather than causing UB - it would make more sense to keep this include, no?

With the removal of the loop, we'd remove the comment as it's inaccurate and de-indent the entire block

With the removal of the loop, we'd remove the comment as it's inaccurate and de-indent the entire block
@ -184,0 +201,4 @@
single_step = false;
/* fall through */
case '?': { /* '?': Request reason for target halt */
/*

Does this not need to call the new gdb_poll_target() before sending back a W00 or no-response? If not, please add documentation as to why this otherwise disjointed-seeming approach works and what the new logic is

Does this not need to call the new gdb_poll_target() before sending back a `W00` or no-response? If not, please add documentation as to why this otherwise disjointed-seeming approach works and what the new logic is

Please don't use C89 style declarations - having the if statement first, then declaring watch and declare-initialising reason helps the compiler and removes a bug possibility.

Please don't use C89 style declarations - having the if statement first, then declaring watch and declare-initialising reason helps the compiler and removes a bug possibility.

tautological statement - please remove per clang-tidy lints.

tautological statement - please remove per clang-tidy lints.
@ -28,6 +28,40 @@
#include "gdb_packet.h"
#include "morse.h"

These braces ({, }) are still redundant, please remove them.

These braces (`{`, `}`) are still redundant, please remove them.
@ -31,0 +45,4 @@
// alter these variables.
if (!gdb_target_running || !cur_target)
break;
char c = gdb_if_getchar_to(0);

Is it possible to flatten this out a bit by making this while loop the outer while loop?

Is it possible to flatten this out a bit by making this while loop the outer while loop?

This cast should be removed - it should do nothing now and was a bug that it was ever required.

This cast should be removed - it should do nothing now and was a bug that it was ever required.
xobs commented 2023年01月31日 06:43:32 +01:00 (Migrated from github.com)
Copy link

I've made the requested changes, except I'm not sure what you mean by flattening out that bit.

I've made the requested changes, except I'm not sure what you mean by flattening out that bit.
dragonmux left a comment
Copy link

Think this should be the last round of changes. Next review should be approve-to-merge.

To explain what we meant about flattening the main loop out a bit: currently there's a while loop inside which there's a "try-catch", inside which another while loop - this is a lot of levels of indentation, and especially as the outer most is while true.

What we were thinking is if it's possible to push the inner while condition out to the outer while loop, making the inner while body a part of the try-catch scope instead, removing a layer indentation. If not, it might be worth extracting the inner while loop into its own function to reduce the indentation level and improve readability instead?

Think this should be the last round of changes. Next review should be approve-to-merge. To explain what we meant about flattening the main loop out a bit: currently there's a while loop inside which there's a "try-catch", inside which another while loop - this is a lot of levels of indentation, and especially as the outer most is `while true`. What we were thinking is if it's possible to push the inner while condition out to the outer while loop, making the inner while body a part of the try-catch scope instead, removing a layer indentation. If not, it might be worth extracting the inner while loop into its own function to reduce the indentation level and improve readability instead?

target_halt_reason_e please - the enum is typedef'd for this reason

`target_halt_reason_e` please - the enum is typedef'd for this reason
@ -31,0 +45,4 @@
// alter these variables.
if (!gdb_target_running || !cur_target)
break;
char c = gdb_if_getchar_to(0);

pbuf[0] is a char, so this should compare '\x04' for type consistency please

`pbuf[0]` is a char, so this should compare `'\x04'` for type consistency please
xobs commented 2023年02月03日 13:03:40 +01:00 (Migrated from github.com)
Copy link

Alright, both of those requested changes have been made. I can't figure out how to de-indent those without adding a goto, so I opted to pull it into its own function instead.

Alright, both of those requested changes have been made. I can't figure out how to de-indent those without adding a goto, so I opted to pull it into its own function instead.
dragonmux left a comment
Copy link
No description provided.
LGTM, we'll give this a test locally just to make sure nothing broke on native, then merge. Many thanks xobs! While we do, could we get you to rebase the PR so the gdb_main.c conflict is resolved? We'll aim to merge this today so this should be the last rebase

Testing says this works well on both RP2040 and Tiva-C, so this is ready for merge once the branch conflict has been sorted

Testing says this works well on both RP2040 and Tiva-C, so this is ready for merge once the branch conflict has been sorted
Sign in to join this conversation.
No reviewers
Labels
Clear labels
BMD App
Black Magic Debug App (aka. PC hosted) (not firmware)
BMP Firmware
Black Magic Probe Firmware (not PC hosted software)
Bug
Confirmed bug
Build system
Build system
Can't reproduce
Maintainers can't reproduce this problem
CI
Continuous Integration System
Contribution wanted
User contributions welcome
Documentation
Project documentation
Draft
Work in progress draft
Duplicate
This issue or pull request already exists
Enhancement
General project improvement
Feedback wanted
Requires additional submitter feedback
Foreign Host Board
Non Native hardware to runing Black Magic firmware on
GDB
Issue/PR related to GDB
Good first issue
Good for newcommers
HwIssue Mitigation
Solving or mitigating a Hardware issue in Software
Information Needed
Maintainers need more information
NativeHardware
Official Black Magic Debug Hardware
New Host Board
New hardware to run Black Magic firmware on
New Target
New debug target
Off Topic
Something that does not involve the project in any way
Potential Bug
A potential, unconfirmed or very special circumstance bug
Regression
Bug caused by a regression
User Interest Needed
More user interest required before consideration
User Testing Needed
Looking for user testing reports
Won't fix
Outside of the project scope or works as intended
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
blackmagic-debug/blackmagic!1284
Reference in a new issue
blackmagic-debug/blackmagic
No description provided.
Delete branch "main-loop-invert"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?