-
-
Notifications
You must be signed in to change notification settings - Fork 770
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
Conversation
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 |
sorry, i missed the mail yesteray when i was wfh-ing. it might be a day or two. |
4367460
to
970c52f
Compare
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? |
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 git switch main
git pull upstream main
git switch pull-ti-msp432e4
git rebase main
git push -f |
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? |
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. |
8d6042e
to
1bd5d08
Compare
1bd5d08
to
1f65f3f
Compare
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. |
…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.
…bout and reduce Flash usage
…ing the pre-existing erase_range and erase_mass implementations
…tra comments and simplification
…age of the target Flash layer
1f65f3f
to
0f043ca
Compare
…age of the target Flash layer
There was a problem hiding this 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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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
make PROBE_HOST=native
)make PROBE_HOST=hosted
)Closing issues