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.

Sign up for GitHub

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

Jump to bottom

J-Link cleanup and Target Power implementation #1548

Merged
merged 19 commits into from
Jul 28, 2023
Merged

J-Link cleanup and Target Power implementation #1548

merged 19 commits into from
Jul 28, 2023

Conversation

perigoso
Copy link
Contributor

@perigoso perigoso commented Jul 20, 2023 •
edited
Loading

Detailed description

Branch is a fix, but this is actually a feature, all things considered

This is a general cleanup of J-Link code, with the final goal of adding target power functionality

It also includes the change from #1545:

J-Link version string was being shown on BMDA startup with verbosity, but not when calling the version command, this addresses that

Tested with a J-Link V8 and OB J-Link (Silabs Dev board)

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

replaces #1545

@perigoso perigoso force-pushed the fix/tpwr branch 2 times, most recently from 973acc6 to 83c0443 Compare July 20, 2023 01:18
@dragonmux dragonmux added this to the v1.10 milestone Jul 20, 2023
@dragonmux dragonmux added Bug Confirmed bug BMD App Black Magic Debug App (aka. PC hosted) (not firmware) Enhancement General project improvement labels Jul 20, 2023
dragonmux
dragonmux requested changes Jul 20, 2023
View reviewed changes
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 PR does a lot to the J-Link code so we've started small with a couple of items we've spotted that have large knock-on effects. We'll review further once these items have been addressed

src/include/platform_support.h Show resolved Hide resolved
src/platforms/hosted/jlink.c Outdated Show resolved Hide resolved
@perigoso perigoso force-pushed the fix/tpwr branch 4 times, most recently from c373436 to 4ab7220 Compare July 20, 2023 12:42
@perigoso perigoso requested a review from dragonmux July 20, 2023 12:43
@perigoso perigoso force-pushed the fix/tpwr branch 4 times, most recently from 88e43b9 to eea537c Compare July 27, 2023 15:01
perigoso added 4 commits July 27, 2023 16:01
@perigoso @rg-silva
platforms: refactor platform_target_set_power return value, error and…
aeb0449 
… debug handling
@perigoso @rg-silva
hosted/jlink: move unused VID/PID macros to platform header
5b7e23b
@perigoso @rg-silva
hosted/jlink: return bool on jlink protocol functions
b2bd0ab 
The return value of these functions is only used for success/fail check,
returning a bool gives us more readable code at no cost to current functionality
@perigoso @rg-silva
bmp_libusb: add timeout argument for usb transfers
4c6178c 
The J-Link USB Protocol RM defines a USB communication timeout, use it
perigoso added 2 commits July 27, 2023 19:30
@perigoso @rg-silva
hosted/jlink_protocol: transcribe jlink usb protocol from reference man
f30ed4e 
Include all publicly known commands from RM08001 J-Link USB protocol Reference manual

The commands have been reordered and renamed in the context of Black Magic Debug in an
effort to make them more intuitive, consistent and easier to use

Mapping tables from the BMDA version of the commands to the ones on the RM are provided
to make referencing the documentation easier
@perigoso @rg-silva
hosted/jlink: cleanup SIGNAL_STATE based commands
e612aae 
Update constant suffixes and use macros in place of magic numbers
@perigoso
Copy link
Contributor Author

Pushed a couple fixups, typos and extra const keywords

dragonmux
dragonmux requested changes Jul 27, 2023
View reviewed changes
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.

Thank you for your excellent work on this. It's looking much better as a result - there are some times we've caught in review, but they shouldn't be complicated to sort. We look forward to merging this.

src/platforms/hosted/jlink.h Outdated Show resolved Hide resolved
src/platforms/hosted/jlink_protocol.h Outdated Show resolved Hide resolved
src/platforms/hosted/jlink.c Outdated Show resolved Hide resolved
src/platforms/hosted/jlink.c Show resolved Hide resolved
src/platforms/hosted/jlink.c Outdated Show resolved Hide resolved
src/platforms/hosted/jlink.c Outdated Show resolved Hide resolved
src/platforms/hosted/jlink.c Outdated Show resolved Hide resolved
src/platforms/hosted/jlink.c Show resolved Hide resolved
src/platforms/hosted/jlink.c Outdated Show resolved Hide resolved
src/platforms/hosted/jlink.c Outdated Show resolved Hide resolved
dragonmux
dragonmux approved these changes Jul 28, 2023
View reviewed changes
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, thank you for the contribution! Merging.

@dragonmux dragonmux merged commit ad36690 into blackmagic-debug:main Jul 28, 2023
@perigoso perigoso deleted the fix/tpwr branch July 28, 2023 01:46
@ALTracer ALTracer mentioned this pull request Sep 13, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dragonmux dragonmux dragonmux approved these changes

Assignees
No one assigned
Labels
BMD App Black Magic Debug App (aka. PC hosted) (not firmware) Bug Confirmed bug Enhancement General project improvement
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.
2 participants
@perigoso @dragonmux