7
31
Fork
You've already forked blackmagic
23

rtt: Add gdb monitor rtt down command #1704

Open
HrMitrev wants to merge 5 commits from HrMitrev/gdb_rtt_down into main
pull from: HrMitrev/gdb_rtt_down
merge into: blackmagic-debug:main
blackmagic-debug:main
blackmagic-debug:fix/pre-bmp-v3-cross-file-cleanup
blackmagic-debug:feature/bmda-remote-comms
blackmagic-debug:ALTracer/feature/aarch64-ident
blackmagic-debug:feature/better-meson-optimisation-handling
blackmagic-debug:feature/am335x-support
blackmagic-debug:feature/esp32-c3-support
blackmagic-debug:feature/cortex-ar-software-breakpoints
blackmagic-debug:feature/unit-testing
blackmagic-debug:feature/windows-usb-serial-interface-naming
blackmagic-debug:fix/bmp-external-spi
blackmagic-debug:ALTracer/feature/bluepillplus-platform
blackmagic-debug:ALTracer/feature/at32f43x-unrdp
blackmagic-debug:feature/const-correctness
blackmagic-debug:ALTracer/feature/fault_handlers
blackmagic-debug:ALTracer/feature/hazard3-ice40-support
blackmagic-debug:fix/ci-cleanup
blackmagic-debug:ALTracer/fix/gdb-10-12-thread
blackmagic-debug:ALTracer/feature/blackpill-f4-adc
blackmagic-debug:ALTracer/fix/cortex-desc-allocfail-report
blackmagic-debug:ALTracer/feature/spi-perf
blackmagic-debug:ALTracer/feature/calibrate_swd
blackmagic-debug:ALTracer/feature/blank-check
blackmagic-debug:feature/avr
blackmagic-debug:v2.0
blackmagic-debug:v1.9
blackmagic-debug:v1.10
blackmagic-debug:v1.8
HrMitrev commented 2023年12月18日 17:53:00 +01:00 (Migrated from github.com)
Copy link

Detailed description

Just for convenience I wanted to be able to
push strings to the rtt down buffer without
the need to change the active window from
the one where the gdb session is running

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

Closing issues

None

## Detailed description Just for convenience I wanted to be able to push strings to the rtt down buffer without the need to change the active window from the one where the gdb session is running ## 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`) * [ ] I've tested it to the best of my ability * [x] My commit messages provide a useful short description of what the commits do ## Closing issues None
HrMitrev commented 2023年12月18日 18:06:25 +01:00 (Migrated from github.com)
Copy link

Hi @koendv 🙂

Since you're the original author of the RTT functionality, could you take a quick look?

The main idea is to start the program with run & when using gdb and then I want to have .gdbinit function which sends SIGINT (interrupt) then monitor rtt down 'string' then continue &

Ideally it would be best if I there is a way to interact with the BMP probe from gdb without interrupting the debug session, but I'm not sure how and if it is even possible. Sure I can directly use ttyBmpTarg to send data to the target, but just for convenience I don't want to change the active window to the terminal where I view the log, send the string and then go back to the terminal where the gdb session is running

Hi @koendv 🙂 Since you're the original author of the RTT functionality, could you take a quick look? The main idea is to start the program with `run &` when using gdb and then I want to have .gdbinit function which sends SIGINT (`interrupt`) then `monitor rtt down 'string'` then `continue &` Ideally it would be best if I there is a way to interact with the BMP probe from gdb without interrupting the debug session, but I'm not sure how and if it is even possible. Sure I can directly use `ttyBmpTarg` to send data to the target, but just for convenience I don't want to change the active window to the terminal where I view the log, send the string and then go back to the terminal where the gdb session is running
dragonmux left a comment
Copy link

There are a couple of linguistic items we caught in review and some type issues. Please see below for more on those. Please also take care of the clang-format lints the CI picked up.

We're distinctly unsure whether to take this proposed change or not, being how easy it is to use a second console with a serial program to interact with the RTT stream. That said, we can also sort of see the sense of this command too. We'll see how bad the Flash usage hit is after the adjustments requested and have a chat with Esden.

There are a couple of linguistic items we caught in review and some type issues. Please see below for more on those. Please also take care of the clang-format lints the CI picked up. We're distinctly unsure whether to take this proposed change or not, being how easy it is to use a second console with a serial program to interact with the RTT stream. That said, we can also sort of see the sense of this command too. We'll see how bad the Flash usage hit is after the adjustments requested and have a chat with Esden.

Perhaps instead of "down" lets use "send" - "down" doesn't make much grammatical or linguistical sense to us as a native English speaker and leaves us wondering what this really does. mon rtt send <stuff> makes much more sense - you're sending to the target.

Perhaps instead of "down" lets use "send" - "down" doesn't make much grammatical or linguistical sense to us as a native English speaker and leaves us wondering what this really does. `mon rtt send <stuff>` makes much more sense - you're sending <stuff> to the target.

Either include the check, or entirely eliminate it please. Likewise, proper capitalisation - "Here" in the above comment, "The" in the gdb_out() below. Please drop the braces on the else, they're unnecessary.

Either include the check, or entirely eliminate it please. Likewise, proper capitalisation - "Here" in the above comment, "The" in the gdb_out() below. Please drop the braces on the else, they're unnecessary.

strlen() returns a size_t, please do not truncate it to a uint16_t.

`strlen()` returns a size_t, please do not truncate it to a uint16_t.
@ -60,5 +60,6 @@ typedef struct rtt_channel {
extern rtt_channel_s rtt_channel[MAX_RTT_CHAN];

Likewise to the strlen() comment, size_t please.

Likewise to the `strlen()` comment, `size_t` please.
@ -58,9 +58,20 @@ inline static bool recv_set_nak()
return recv_bytes_free() < 2U * CDCACM_PACKET_SIZE;

This isn't actually your fault, but the use of raw int introduces a signed-unsigned conversion and a silent error here. Please switch this to use size_t or at least uint16_t to match the len argument.

Similarly with the proper capitalisation of comments. We know that one's also not your fault here, this code has just not yet been cleaned up and the original author was distinctly lazy about it.

This isn't actually your fault, but the use of raw `int` introduces a signed-unsigned conversion and a silent error here. Please switch this to use `size_t` or at least `uint16_t` to match the `len` argument. Similarly with the proper capitalisation of comments. We know that one's also not your fault here, this code has just not yet been cleaned up and the original author was distinctly lazy about it.
@ -78,14 +89,7 @@ void rtt_serial_receive_callback(usbd_device *dev, uint8_t ep)
return;

Please insert a new line above this comment, and remove the one below it.

Please insert a new line above this comment, and remove the one below it.
HrMitrev (Migrated from github.com) reviewed 2023年12月18日 21:35:27 +01:00
HrMitrev (Migrated from github.com) commented 2023年12月18日 21:35:27 +01:00
Copy link

If I include the check, the error message adds to the .text segment, so I just print something generic like "error"? I think it is good to have this in place just in case and also it is OK to bypass, bacause it is not at all critical. So if someone is browsing the code can re-enable it.

If I include the check, the error message adds to the .text segment, so I just print something generic like "error"? I think it is good to have this in place just in case and also it is OK to bypass, bacause it is not at all critical. So if someone is browsing the code can re-enable it.

Given what the message is trying to express, perhaps gdb_print("rtt not found\n");? It expresses the same information without using quite so much Flash - we'd really prefer this and the check to be enabled, or just entirely eliminate the check. The only reason the || true is not causing additional Flash usage is because of the Dead-Code Elimination pass and we do not want to be relying on that, it's implementation-defined behaviour.

Given what the message is trying to express, perhaps `gdb_print("rtt not found\n");`? It expresses the same information without using quite so much Flash - we'd really prefer this and the check to be enabled, or just entirely eliminate the check. The only reason the `|| true` is not causing additional Flash usage is because of the Dead-Code Elimination pass and we do not want to be relying on that, it's implementation-defined behaviour.
koendv commented 2023年12月19日 07:44:09 +01:00 (Migrated from github.com)
Copy link

On 2023年12月18日 09:06:36 -0800
Hristo Mitrev @.***> wrote:

Hi @koendv 🙂

Since you're the original author of the RTT functionality, could you
take a quick look?

Hi Hristo.

This also works:
^C
Program received signal SIGINT, Interrupt.
0x08000528 in ?? ()
(gdb) python rtt = open("/dev/ttyBmpTarg", "w")
(gdb) python rtt.write("hello, world")
(gdb) python rtt.flush()
(gdb) c
Continuing.

I'm wondering:
If you send data, maybe you'll want to wait for some kind of reply.
Like the linux "expect" command, or python pexpect.

koen

On 2023年12月18日 09:06:36 -0800 Hristo Mitrev ***@***.***> wrote: > Hi @koendv 🙂 > > Since you're the original author of the RTT functionality, could you > take a quick look? > Hi Hristo. This also works: ^C Program received signal SIGINT, Interrupt. 0x08000528 in ?? () (gdb) python rtt = open("/dev/ttyBmpTarg", "w") (gdb) python rtt.write("hello, world") (gdb) python rtt.flush() (gdb) c Continuing. I'm wondering: If you send data, maybe you'll want to wait for some kind of reply. Like the linux "expect" command, or python pexpect. koen
HrMitrev commented 2023年12月30日 00:09:09 +01:00 (Migrated from github.com)
Copy link

Hi Hristo. This also works: ^C Program received signal SIGINT, Interrupt. 0x08000528 in ?? () (gdb) python rtt = open("/dev/ttyBmpTarg", "w") (gdb) python rtt.write("hello, world") (gdb) python rtt.flush() (gdb) c Continuing. I'm wondering: If you send data, maybe you'll want to wait for some kind of reply. Like the linux "expect" command, or python pexpect. koen

Hi @koendv,

This is a great suggestion, but I am afraid that my use-case is a bit different.
I always have /dev/ttyBmpTarg open in another console because I need to save the logs to a file and also look at them in the meantime. My main idea was to skip the need of switching from gdb to the log console in order to send commands. Since I have to have /dev/ttyBmpTarg open in another window I thought that this gdb mon command will be helpful.

PS: it took me a while to realize that there are 2 separate rtt_if.c files for stm32 and launchpad-icdi. I think I fixed the problem and it seems that all the tests now pass. I also rebased to the latest commit in main

> Hi Hristo. This also works: ^C Program received signal SIGINT, Interrupt. 0x08000528 in ?? () (gdb) python rtt = open("/dev/ttyBmpTarg", "w") (gdb) python rtt.write("hello, world") (gdb) python rtt.flush() (gdb) c Continuing. I'm wondering: If you send data, maybe you'll want to wait for some kind of reply. Like the linux "expect" command, or python pexpect. koen Hi @koendv, This is a great suggestion, but I am afraid that my use-case is a bit different. I always have /dev/ttyBmpTarg open in another console because I need to save the logs to a file and also look at them in the meantime. My main idea was to skip the need of switching from gdb to the log console in order to send commands. Since I have to have /dev/ttyBmpTarg open in another window I thought that this gdb mon command will be helpful. PS: it took me a while to realize that there are 2 separate rtt_if.c files for stm32 and launchpad-icdi. I think I fixed the problem and it seems that all the tests now pass. I also rebased to the latest commit in main
koendv (Migrated from github.com) reviewed 2023年12月30日 13:41:49 +01:00
koendv commented 2023年12月30日 14:25:29 +01:00 (Migrated from github.com)
Copy link

I think an rtt "send" command might be useful as a first step towards making testbenches in gdb/python.

I think an rtt "send" command might be useful as a first step towards making testbenches in gdb/python.
ALTracer (Migrated from github.com) reviewed 2023年12月30日 14:52:41 +01:00
ALTracer (Migrated from github.com) commented 2023年12月30日 14:52:41 +01:00
Copy link
		"poll [MAXMS MINMS MAXERR]]"},

Is this with the pre-commit hook installed and clang-format working?
upd: Nevermind, sorry, was looking at an older commit.

```suggestion "poll [MAXMS MINMS MAXERR]]"}, ``` Is this with the `pre-commit` hook installed and clang-format working? upd: Nevermind, sorry, was looking at an older commit.
HrMitrev (Migrated from github.com) reviewed 2023年12月30日 17:31:02 +01:00
HrMitrev commented 2024年02月09日 12:45:05 +01:00 (Migrated from github.com)
Copy link

Hi Again!
If I remember correctly, the overhead of this feature was 72 more bytes of ROM.

Since I proposed this enhancement I also realized that it will be helpful when I implement a make-shift password that will be used to lock some debug features of the target. If I implement such password it will be tedious to enter it every time I flash my target. Having this feature will help to automate this. (I thought of this password because I give debug versions of my firmware to testing colleagues who provide me with logs, but the problem is that I also implement "scanf actions" that can alter the internal state of the program and lead to false positive bug reports)

One more proposal from my side will be to add ifdef guards and make this optional if we think that it will add too much overhead.

Cheers!

Hi Again! If I remember correctly, the overhead of this feature was 72 more bytes of ROM. Since I proposed this enhancement I also realized that it will be helpful when I implement a make-shift password that will be used to lock some debug features of the target. If I implement such password it will be tedious to enter it every time I flash my target. Having this feature will help to automate this. (I thought of this password because I give debug versions of my firmware to testing colleagues who provide me with logs, but the problem is that I also implement "scanf actions" that can alter the internal state of the program and lead to false positive bug reports) One more proposal from my side will be to add `ifdef` guards and make this optional if we think that it will add too much overhead. Cheers!
This pull request has changes conflicting with the target branch.
  • src/command.c
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin HrMitrev/gdb_rtt_down:HrMitrev/gdb_rtt_down
git switch HrMitrev/gdb_rtt_down
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!1704
Reference in a new issue
blackmagic-debug/blackmagic
No description provided.
Delete branch "HrMitrev/gdb_rtt_down"

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?