pace_poll: allow other platforms to delay

[xobs]

Detailed description

The RTT implementation normally spends most of its time in a tight loop. This can cause problems on some platforms where tight loops can starve other threads.

Add support for defining PLATFORM_HAS_PACE_POLL in platform.h in order to supply a platform_pace_poll() function that can implement this.

This prevents WDT triggers on ESP32 where RTT blocks the IDLE thread from running. The IDLE thread is responsible for resetting the WDT.

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

[dragonmux]

LGTM, and seems like a reasonable patch for the problem. @koendv does this seem reasonable to you too?

We'll merge this if it is

[koendv]

On Thu, 10 Nov 2022 23:08:33 -0800 Rachel Mant ***@***.***> wrote: @dragonmux approved this pull request. LGTM, and seems like a reasonable patch for the problem. @koendv does this seem reasonable to you too? We'll merge this if it is

OK. Good for me. But I have one question. How tight is this loop we are talking about? If I look at rtt.c, function poll_rtt(), I see: static uint32_t poll_ms; static uint32_t last_poll_ms; ... if (last_poll_ms + poll_ms <= now || now < last_poll_ms) { ... /* rtt polling frequency goes up and down with rtt activity */ ... After running poll_rtt(), you can wait poll_ms milliseconds before calling poll_rtt() again, no problem. So perhaps break out poll_ms, so poll_ms is no longer static and can be used for pacing rtt? koen

[dragonmux]

We'll hold off on merging till xobs has been able to answer that excellent question

[xobs]

I'm not sure I understand what you mean by "how tight" the loop is, but my understanding is that it's a busy loop.

The code in gdb_main.c that calls this is here:

blackmagic/src/gdb_main.c

Lines 221 to 230 in 02986d6

	while(!(reason = target_halt_poll(cur_target, &watch))) {
	char c = (char)gdb_if_getchar_to(0);
	if(c == '\x03' || c == '\x04')
	target_halt_request(cur_target);
	platform_pace_poll();
	#ifdef ENABLE_RTT
	if (rtt_enabled)
	poll_rtt(cur_target);
	#endif
	}

This will:

1. Poll the target to see if it needs to halt
2. Poll the input buffer to see if there's any data
3. Try to poll RTT

None of these have any delays, so it will busy-wait polling those three things. The RTT code you quoted seems to decide whether the call to poll_rtt() actually does something, but it doesn't prevent that from being repeatedly called and having no work done.

I think it was incorrect of me to call out rtt as being the culprit here. I believe it's just exposing an existing issue that is causing instability on my host. The ultimate solution is still to insert a delay every once in a while, similar to how it's done with PC_HOSTED.

[koendv]

Yes, it seems a generic issue, not limited to rtt.

I attach a small patch that breaks out the variable rtt_poll_ms.
With default settings, rtt_poll_ms is between 8ms (lots of things to print) and 256ms (nothing to print, just checking).
As far as rtt is concerned, you can put a delay of rtt_poll_ms milliseconds in gdb_main_loop(), no problem.

rtt_poll_patch.txt

[xobs]

I've updated the PR title and commit message to reflect the fact that it's not RTT that's causing the issue, and ultimately what my target needs is a way to occasionally call yield() similar to how 868e76b adds slow polling as the default setting for hosted mode.

I can apply your rtt patch if you like, but I don't immediately have a need for examining rtt_poll_ms.

[koendv]

On Fri, 11 Nov 2022 00:50:09 -0800 Sean Cross ***@***.***> wrote: This is bmp on esp32, right? I would suggest: take gdb_main_loop(), and rewrite it as a task: - no arguments - execute gdb remote command stored in 'pbuf' - return immediately - no busy waiting - rewrite as state machine: - replace while(1) loop by switch(state) {} - convert every busy wait to a state of the state machine Opinion? koen

[xobs]

This is BMP on an ESP32, yes. I'm creating a task that wraps gdb_main() and I've rewritten the gdb_if_* commands to call versions that map to wifi equivalents. The GDB main wrapper is at https://github.com/farpatch/farpatch/blob/main/main/gdb_if.c#L88-L131

By taking this approach I'm able to reuse almost the entirety of BMP. I just have to omit the JTAG/SWD drivers, USB code, and main loop. Everything else is used wholesale. I've taken this approach to try and be light-fingered and avoid introducing too many architectural changes in BMP.

If you'd prefer me to rewrite gdb_main() in gdb_main.c so that it's state-based, I can do that and submit a PR. Overall there are just two states -- halted and not-halted. My concern is that it essentially turns the loop inside-out, and I'd want to make sure the approach is what you had in mind prior to starting that.

[koendv]

On Fri, 11 Nov 2022 05:41:29 -0800 Sean Cross ***@***.***> wrote: This is BMP on an ESP32, yes. I'm creating a task that wraps `gdb_main()` and I've rewritten the `gdb_if_*` commands to call versions that map to wifi equivalents. The GDB main wrapper is at https://github.com/farpatch/farpatch/blob/main/main/gdb_if.c#L88-L131 By taking this approach I'm able to reuse almost the entirety of BMP. I just have to omit the JTAG/SWD drivers, USB code, and main loop. Everything else is used wholesale. I've taken this approach to try and be light-fingered and avoid introducing too many architectural changes in BMP. If you'd prefer me to rewrite `gdb_main()` in `gdb_main.c` so that it's state-based, I can do that and submit a PR. Overall there are just two states -- halted and not-halted. My concern is that it essentially turns the loop inside-out, and I'd want to make sure the approach is what you had in mind prior to starting that.

in gdb_main_loop() there is /* Wait for target halt */ while(!(reason = target_halt_poll(cur_target, &watch))) { That while() translates to a state "waiting for target halt". and semi-hosting will probably also need a state, "waiting for semihosting response from gdb". Opinion? koen

[xobs]

I'm still not entirely sure what architecture you're looking to move to.

Right now it looks like this:

1. Platform setup calls gdb_main(), which never returns
2. gdb_main() calls gdb_main_loop() with a struct target_controller argument that is non-public
3. gdb_main_loop() spends most of its time in gdb_getpacket() or the target_halt_poll() tight loop

What approach are you looking for in the new design?

Would you make struct target_controller gdb_controller public? Would gdb_main_loop() return now between calls? Would gdb_getpacket() get hoisted into gdb_main()?

Some of these changes would be nice, because it would be great to be able to put a mutex around the main loop and share it among different network sockets. But making these changes requires removing static from a lots of structs that I've tried to not touch so far. Is that your intended approach?

[koendv]

On Fri, 11 Nov 2022 06:42:43 -0800 Sean Cross ***@***.***> wrote: I'm still not entirely sure what architecture you're looking to move to.

You could look here: https://gitee.com/koendv/blackmagic/raw/devel/patch/gdb_main_c.patch But, in general, I would like you to do better than I have done. koen

[xobs]

I'll close this issue in favour of #1284