gdb_main: move main loop into main()
#1284
Conversation
|
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 #1281. |
|
I've tried rtt on black pill v2 and hosted on arm64, seems ok. Flashing works, speed unchanged. Not bad. |
|
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. |
|
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
|
|
I just checked, and it seems that RP2040 support is just weird even on From this point, the board has stopped responding. Examining memory returns all zeroes. Doing an 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 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 |
|
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 flashHowever, 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 |
|
I opened #1295 which is at least rather reproducible |
xobs
force-pushed
the
main-loop-invert
branch
from
c316b9f to
ec10885
Compare
|
Rebased to latest |
dragonmux
left a comment
dragonmux
left a comment
There was a problem hiding this comment.
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
xobs
force-pushed
the
main-loop-invert
branch
3 times, most recently
from
448422a to
37498c8
Compare
|
Rebased to latest |
xobs
force-pushed
the
main-loop-invert
branch
from
37498c8 to
5d4bdb0
Compare
xobs
force-pushed
the
main-loop-invert
branch
from
5d4bdb0 to
ddffdfd
Compare
|
Rebased to merge latest changes on |
xobs
force-pushed
the
main-loop-invert
branch
from
ddffdfd to
c4fab70
Compare
xobs
force-pushed
the
main-loop-invert
branch
from
c4fab70 to
72dfe68
Compare
|
Rebased to fix conflicts with latest |
|
Rebased again to fix conflicts with latest |
xobs
force-pushed
the
main-loop-invert
branch
2 times, most recently
from
aa85558 to
94e77c3
Compare
|
We will try to get this reviewed today now the v1.10 merge window is open |
dragonmux
left a comment
dragonmux
left a comment
There was a problem hiding this comment.
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.
src/gdb_main.c
Outdated
| default: | ||
| gdb_putpacket_f("T%02X", GDB_SIGTRAP); | ||
| } | ||
| if (cur_target) |
There was a problem hiding this comment.
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
src/main.c
Outdated
| TRY_CATCH (e, EXCEPTION_ALL) { | ||
| gdb_main(); | ||
| SET_IDLE_STATE(false); | ||
| while (gdb_target_running && cur_target) { |
There was a problem hiding this comment.
Is it possible to flatten this out a bit by making this while loop the outer while loop?
xobs
force-pushed
the
main-loop-invert
branch
from
eb80a0a to
ad0b905
Compare
|
I've made the requested changes, except I'm not sure what you mean by flattening out that bit. |
dragonmux
left a comment
dragonmux
left a comment
There was a problem hiding this comment.
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?
|
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. |
There was a problem hiding this comment.
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 |
This is an attempt to invert the main loop. Rather than spending the majority of time executing within `gdb_main()`, turn it into a state machine that gets polled. This will improve behaviour on PC_HOSTED platforms as well as platforms such as ESP32 where the main loop needs to pause occasionally. Signed-off-by: Sean Cross <[email protected]>
These macros were previously empty, which made them unusable in if()
statements without braces. Add an empty do{}while(0) statement that
allows these statements to be used in such locations.
Signed-off-by: Sean Cross <[email protected]>
Add a parameter to `pbuf_size`, since `sizeof(pbuf)` now always returns the size of a pointer. Signed-off-by: Sean Cross <[email protected]>
The function was indented bceause it used to have a loop around it. Deindent the entire function now that the `while()` is gone. Signed-off-by: Sean Cross <[email protected]>
Use newer C standard variable initialization. Signed-off-by: Sean Cross <[email protected]>
Remove extra braces and an extra, now-unnecessary cast. Signed-off-by: Sean Cross <[email protected]>
In order to prevent super indentation, move the main poll loop into its own function. Signed-off-by: Sean Cross <[email protected]>
This typedef exists, so we might as well use it. Signed-off-by: Sean Cross <[email protected]>
dragonmux
force-pushed
the
main-loop-invert
branch
from
6e5d32d to
f532e55
Compare
…o a hidden UB declaration of gdb_main_loop()
…o a hidden UB declaration of gdb_main_loop()
3 participants
Detailed description
This is a rewrite of the main loop as suggested in #1281. Rather
than constantly sitting inside
gdb_main(), the program's main loop does its own polling of the target. This enablesa 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_HOSTEDwhere constant USB polling is undesirable.Your checklist for this pull request
make PROBE_HOST=native)make PROBE_HOST=hosted)