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: HC32L110 support #1241

Merged
merged 9 commits into from
Jul 17, 2023
Merged

Feature: HC32L110 support #1241

merged 9 commits into from
Jul 17, 2023

Conversation

vesim987
Copy link
Contributor

Detailed description

In this PR I've implemented support for detecting and flashing HC32L110B(A variant should work too, but i don't have it to test). And also introduce flash_operation in flash.prepare and flash.done

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

@vesim987 vesim987 changed the title Hc32l110 Feature: HC32L110 support Sep 15, 2022
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.

couple functions look unformatted, make sure to run clang-format (this might be the github view at fault)

src/target/target_internal.h Outdated Show resolved Hide resolved
src/target/target_internal.h Outdated Show resolved Hide resolved
@vesim987 vesim987 force-pushed the hc32l110 branch 7 times, most recently from c3ee7a7 to 422c24c Compare September 16, 2022 00:45
@vesim987 vesim987 marked this pull request as ready for review September 16, 2022 00:45
src/target/hc32l110.c Outdated Show resolved Hide resolved
src/target/hc32l110.c Outdated Show resolved Hide resolved
src/target/hc32l110.c Outdated Show resolved Hide resolved
{
if (t->flash_mode)
if (t->flash_mode) {
t->current_flash_operation = op;

This comment was marked as outdated.

@dragonmux dragonmux added the New Target New debug target label Sep 19, 2022
@dragonmux dragonmux added this to the v1.10 milestone Sep 19, 2022
@dragonmux
Copy link
Member

Please can you rebase this against main, or let us know if you're happy for us to do so for you, so we can review and merge this?

@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, addressing Perigoso's review points. Getting this sorted out and merged for v1.10 would be really good.

@dragonmux dragonmux force-pushed the hc32l110 branch 7 times, most recently from 5fe5454 to 805513a Compare July 13, 2023 12:38
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.

One of the issues I noted in a previous review was not yet addressed (to be fair I didn't really dive into the problem and provide context), that is, the new flash operation status is implemented on the wrong scope, it's kept on the target scope when it should belong to the flash structs.

Instead of thoroughly explaining what needed to change in review I found it easier to just re-write the affected code, you can find the changes in perigoso/fix/hc32l110, 8c353a4 and 08c5786 replace a9ae7c4 and there's an additional fix-up because of function signature changes 8d213d9

src/target/target_flash.c Outdated Show resolved Hide resolved
src/target/hc32l110.c Show resolved Hide resolved
src/target/hc32l110.c Outdated Show resolved Hide resolved
src/target/hc32l110.c Outdated Show resolved Hide resolved
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 c4b988b into blackmagic-debug:main Jul 17, 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