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

Support for TI MSP432E4 parts #1287

Merged
merged 12 commits into from
Jul 13, 2023
Merged

Conversation

neutered
Copy link
Contributor

@neutered neutered commented Nov 16, 2022

Detailed description

add the ti msp432e4 variant. they're both arm cortex-m4f cores but the rest of the internals are pretty different so it was suggested (dragonmux on discord) that a new target was probably the best way to go.

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

@dragonmux
Copy link
Member

dragonmux commented Nov 16, 2022

Thank you for the contribution! We've quickly edited the description to remove the spaces that were preventing the checkboxes being parsed correctly.

One thing of note before we do a code review: please correct the commit messages for all 3 commits to provide what target each modifies - for example msp432p4: Shuffle the MSP432 code about and msp432e4: Implement support for the MSP432E4 variant. The reason for asking for this is so, later, when we're trying to review commit history for building release notes or if we need to bisect, blame, or otherwise inspect the commit log, we get a clear view of what each commit touches and a fair idea of how

@dragonmux dragonmux added this to the v1.10 milestone Nov 16, 2022
@dragonmux dragonmux added the New Target New debug target label Nov 16, 2022
@dragonmux dragonmux changed the title Pull ti msp432e4 Support for TI MSP432E4 parts Nov 16, 2022
@neutered
Copy link
Contributor Author

sorry, i missed the mail yesteray when i was wfh-ing. it might be a day or two.

@neutered
Copy link
Contributor Author

i think i did the subject-line change, but force pushed since no one else was on this branch. github seems ok w/ it, but how this interacts w/ the pull request proper remains to be seen.

please take a look?

@dragonmux
Copy link
Member

Looks fine - the only pre-merge issue that stands now is that you need to update your branch against main - but we'd hold off on this till fix/naming-conformity has merged otherwise you'll keep having to re-do work. If push comes to shove, with your permission, we are able to update the branch for you to resolve the merge conflict.

The quick and easy way to update against main, assuming you have the main repo as a remote named usptream is to run this sequence:

git switch main
git pull upstream main
git switch pull-ti-msp432e4
git rebase main
git push -f

@dragonmux
Copy link
Member

Please can you rebase this on main, or let us know if you're ok with us doing so, so we can get this merged?

@dragonmux
Copy link
Member

Given the time between asking for this to be rebased and now, we're going to get on and do so. Getting this sorted out and merged for v1.10 would be really good.

@dragonmux dragonmux force-pushed the pull-ti-msp432e4 branch 9 times, most recently from 8d6042e to 1bd5d08 Compare July 13, 2023 08:20
@dragonmux dragonmux requested a review from esden July 13, 2023 08:20
@dragonmux
Copy link
Member

We have now adjusted the attribution as the new support had an incomplete copyright notice. We have also performed some refinement on the new support to reduce added Flash usage and integrate it with the existing framework and naming schemes better.

We're happy for this to be merged, so it's over to Esden for a review with a view to merge.

src/target/msp432e4.c Outdated Show resolved Hide resolved
src/target/msp432e4.c Outdated Show resolved Hide resolved
src/target/msp432e4.c Outdated Show resolved Hide resolved
src/target/msp432e4.c Outdated Show resolved Hide resolved
src/target/msp432e4.c Outdated Show resolved Hide resolved
neutered and others added 7 commits July 13, 2023 12:51
…for other parts in the family

it's really supporting the msp432p4 variant which does device id
through a descriptor table. renaming to make it slightly less
confusing, the msp432e4 has a completely different layout.
tested against the MSP432E401YTPDTR that we're using at work.
testing w/ attach/detach, manually erasing flash all/sectors,
and loading elf files via the gdb 'load' command.
…ing the pre-existing erase_range and erase_mass implementations
Copy link
Contributor

@perigoso perigoso left a comment

Choose a reason for hiding this comment

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

The flash routines look great now :) they are at the simplicity level I would like to see on all targets! (It's on my todo to optimize some)

…e ID dissection suffered, bumping its reporting to DEBUG_TARGET() from DEBUG_INFO()
Copy link
Member

@esden esden left a comment

Choose a reason for hiding this comment

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

LGTM!

@esden esden merged commit dc44228 into blackmagic-debug:main Jul 13, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Target New debug target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants