Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: qStartNoAckmode #1361

Merged
merged 6 commits into from
Jul 28, 2023

Conversation

perigoso
Copy link
Contributor

@perigoso perigoso commented Jan 10, 2023

Detailed description

This is the start of no acknowledge mode (QStartNoAckMode) support.

The BMP uses a USB connection with the CDC class (Bulk transfers) for GDB data transfer, this means the connection should be reliable, with integrated integrity checks and data delivery guarantees (provided there are no bugs in the class handling side of things in the firmware), this means that acknowledgment and checksum verification is redundant and should be safe to ignore, hopefully providing a performance benefit, GDB provides support for this with QStartNoAckMode.

See GDB: Packet-Acknowledgment for context.

The intent is to test if this brings improvements in performance, particularly in the flashing routines.

As of right now, this consists only of refactoring of the gdb_getpacket to make use of a state machine, in preparation of the actual implementation, reviews are not necessary but feedback is always welcome, shallow testing showed this to work, so testing is also welcome

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

@perigoso
Copy link
Contributor Author

QStartNoAckmode now implemented, testing didn't show any noticeable difference in speeds, but further testing is needed and welcome

@dragonmux dragonmux added Enhancement General project improvement GDB Issue/PR related to GDB labels Jan 18, 2023
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick check over reveals just one style mistake and something we need to consider between this branch and another that we're contemplating for v1.10. Generally though this looks great and improves the GDB packet handler handily

src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
@perigoso
Copy link
Contributor Author

perigoso commented Jul 21, 2023

@dragonmux rebased over your organization commit, theres still one issue to solve and adding a buildsystem option to enable this feature, but its in a ok state to star reviewing

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, nice job integrating our refactor of the remote packet handling!

There are a few items we've spotted, mostly commented out dead code.

src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
src/gdb_packet.c Outdated Show resolved Hide resolved
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're happy to take this feature when you're happy with the implementation. Update the PR title and mark it as ready for review when you're ready.

We've only seen the one remaining item having re-checked through the cleanups and fixes you've performed and it is only a suggestion not a requested change.

src/gdb_packet.c Outdated Show resolved Hide resolved
In cases where the transport mechanism is itself reliable (on our case USB is), the ‘+’/‘-’ acknowledgments are redundant.
This disables them with the GDB NoAckMode mechanism
This disables only the advertising of NoAckMode to GDB and the
the feature is still compiled in and can still be manually enabled in GDB
@perigoso perigoso changed the title WIP: Feature qStartNoAckmode Feature: qStartNoAckmode Jul 28, 2023
@perigoso perigoso marked this pull request as ready for review July 28, 2023 03:08
@perigoso perigoso requested a review from dragonmux July 28, 2023 03:09
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, We'll run a couple of tests locally just to be sure things work again GDB 13, and then we'll get this merged ASAP. Excellent work and an awesome contribution, thank you!

@dragonmux dragonmux merged commit 95e768b into blackmagic-debug:main Jul 28, 2023
6 checks passed
@perigoso perigoso deleted the feature/qstartnoackmode branch July 28, 2023 10:02
@dragonmux dragonmux mentioned this pull request Aug 1, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement General project improvement GDB Issue/PR related to GDB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants