5
3
Fork
You've already forked bmputil
2

Enhance bmputil-cli target power #57

Open
P-Storm wants to merge 5 commits from P-Storm/feat/target-power into main
pull from: P-Storm/feat/target-power
merge into: blackmagic-debug:main
blackmagic-debug:main
blackmagic-debug:add-coc-no-ai
P-Storm commented 2025年07月29日 15:46:19 +02:00 (Migrated from github.com)
Copy link

Adding more information for command bmputil-cli target power:

[2025年07月29日T13:38:30Z INFO bmputil_cli] Device target power state: false
[2025年07月29日T13:38:30Z INFO bmputil_cli] Target voltage: 0V

Also made Device target power state a warning if the version is too low.

Added preparation for REMOTE_SRST and REMOTE_PWR

Adding more information for command `bmputil-cli target power`: ``` [2025年07月29日T13:38:30Z INFO bmputil_cli] Device target power state: false [2025年07月29日T13:38:30Z INFO bmputil_cli] Target voltage: 0V ``` Also made `Device target power state` a warning if the version is too low. Added preparation for `REMOTE_SRST` and `REMOTE_PWR`
semanticdiff-com[bot] commented 2025年07月29日 15:46:22 +02:00 (Migrated from github.com)
Copy link

Review changes with SemanticDiff

Changed Files
File Status
src/bin/bmputil-cli.rs 42% smaller
src/serial/remote/mod.rs 35% smaller
src/serial/remote/protocol_v4.rs 30% smaller
src/serial/bmd_rsp.rs 27% smaller
src/serial/remote/protocol_v0.rs 0% smaller
src/serial/remote/protocol_v1.rs 0% smaller
src/serial/remote/protocol_v2.rs 0% smaller
src/serial/remote/protocol_v3.rs 0% smaller
Review changes with &nbsp;<a href="https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes"><picture><source media="(prefers-color-scheme: dark)" srcset="https://app.semanticdiff.com/images/github_button.svg?theme=dark&smaller=22"><source media="(prefers-color-scheme: light)" srcset="https://app.semanticdiff.com/images/github_button.svg?theme=light&smaller=22"><img src="https://app.semanticdiff.com/images/github_button.svg?theme=light&smaller=22" align="center" alt="SemanticDiff"></picture></a> <details> <summary>Changed Files</summary> <br/> | File | Status | | :--- | :--- | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/bin/bmputil-cli.rs)&nbsp;&nbsp;[src/bin/bmputil\-cli\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/bin/bmputil-cli.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=42">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/bin/bmputil-cli.rs) &nbsp;42% smaller | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/mod.rs)&nbsp;&nbsp;[src/serial/remote/mod\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/mod.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=35">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/mod.rs) &nbsp;35% smaller | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v4.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v4\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v4.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=30">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v4.rs) &nbsp;30% smaller | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/bmd_rsp.rs)&nbsp;&nbsp;[src/serial/bmd\_rsp\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/bmd_rsp.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=27">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/bmd_rsp.rs) &nbsp;27% smaller | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v0.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v0\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v0.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=0">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v0.rs) &nbsp;0% smaller | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v1.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v1\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v1.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=0">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v1.rs) &nbsp;0% smaller | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v2.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v2\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v2.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=0">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v2.rs) &nbsp;0% smaller | | [<img src="https://app.semanticdiff.com/static/file-icons/rust.svg" width="20" height="20" align="center">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v3.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v3\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v3.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=0">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/57/changes#src/serial/remote/protocol_v3.rs) &nbsp;0% smaller | </details>
esden (Migrated from github.com) reviewed 2025年07月29日 21:28:17 +02:00
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;
esden (Migrated from github.com) commented 2025年07月29日 21:28:16 +02:00
Copy link

While this is still relatively fresh. It might be a good idea to add some docstrings /// to each of the commands and indicate which version of the protocol adds support for them. Unless we feel that it is not necessary to know as it is encoded otherwise?

While this is still relatively fresh. It might be a good idea to add some docstrings `///` to each of the commands and indicate which version of the protocol adds support for them. Unless we feel that it is not necessary to know as it is encoded otherwise?
P-Storm (Migrated from github.com) reviewed 2025年07月29日 21:31:02 +02:00
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;
P-Storm (Migrated from github.com) commented 2025年07月29日 21:31:02 +02:00
Copy link

I would say the version is found with 'find usings', so I fould forgo it. The commands I have to look into what exactly they are, because I still find it somewhat hard to describe them correctly. But agree there with docstring :)

I would say the version is found with 'find usings', so I fould forgo it. The commands I have to look into what exactly they are, because I still find it somewhat hard to describe them correctly. But agree there with docstring :)
esden (Migrated from github.com) reviewed 2025年07月29日 21:39:39 +02:00
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;
esden (Migrated from github.com) commented 2025年07月29日 21:39:39 +02:00
Copy link

Besides this, it might be a good idea to restrict how public the whole struct and constants are. It might be a good idea to make them only pub(self) so that a user is not compelled to use those constants outside of the module. Short of encoding the availability of those commands using some clever way in the typesystem, we should retain it's use to within this module where we contain the "knowledge" about the protocol history.

Besides this, it might be a good idea to restrict how public the whole struct and constants are. It might be a good idea to make them only `pub(self)` so that a user is not compelled to use those constants outside of the module. Short of encoding the availability of those commands using some clever way in the typesystem, we should retain it's use to within this module where we contain the "knowledge" about the protocol history.
dragonmux left a comment
Copy link

Done an initial review, though not particularly thorough - hopefully these notes help direct the evolution of this PR though as it would be great to get this in, but we do need to make sure it's done right.

Done an initial review, though not particularly thorough - hopefully these notes help direct the evolution of this PR though as it would be great to get this in, but we do need to make sure it's done right.
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;

nit: "if power is being supplied to the target"

nit: "if power is being supplied to the target"

Duplicates the first constant, and the SET variant should be named the same way - ie SET_TARGET_POWER_STATE.

Duplicates the first constant, and the SET variant should be named the same way - ie `SET_TARGET_POWER_STATE`.

This one should be dropped or made to agree in encoding with github.com/P-Storm/bmputil@66efc79859/src/serial/bmd_rsp.rs (L26) as this is where that get used.

This one should be dropped or made to agree in encoding with https://github.com/P-Storm/bmputil/blob/66efc79859dedbda73c835a433cc1e673a45ab37/src/serial/bmd_rsp.rs#L26 as this is where that get used.
@ -38,0 +36,4 @@
/// This command asks the probe what it's protocol version is
pubconstHL_CHECK: &str ="!HC#";
/// This command asks the probe to initialise JTAG comms to any connected targets
pubconstJTAG_INIT: &str ="!JS#";

NB that the lifetimes of all the strings here should be implicitly static (so you should be able to say just &str) - if that is not the case then the typing is fine, but we should leave the lifetime implicit if possible.

NB that the lifetimes of all the strings here should be implicitly static (so you should be able to say just `&str`) - if that is not the case then the typing is fine, but we should leave the lifetime implicit if possible.

This needs to be nrst, otherwise this is just plain confusing as srst means Software Reset in a pile of places, but this refers to the nRST pin state (ARM trying to make it mean System Reset does not help things.. it just overloads the initialism)

This needs to be `nrst`, otherwise this is just plain confusing as `srst` means Software Reset in a pile of places, but this refers to the nRST pin state (ARM trying to make it mean System Reset does not help things.. it just overloads the initialism)
@ -169,6 +170,44 @@ impl BmdRemoteProtocol for RemoteV0
{
Err(eyre!("Not supported"))

This duplicates get_target_power_state which is also the better name for what this is and does.

This duplicates `get_target_power_state` which is also the better name for what this is and does.
P-Storm (Migrated from github.com) reviewed 2025年07月30日 13:54:39 +02:00
@ -38,0 +36,4 @@
/// This command asks the probe what it's protocol version is
pubconstHL_CHECK: &str ="!HC#";
/// This command asks the probe to initialise JTAG comms to any connected targets
pubconstJTAG_INIT: &str ="!JS#";
P-Storm (Migrated from github.com) commented 2025年07月30日 13:54:38 +02:00
Copy link

I tried to make it implicit, but my IDE was yelling to me (RustRover RUST-11520).

It does work without the static, so I can remove it if you want.

I tried to make it implicit, but my IDE was yelling to me (RustRover [RUST-11520](https://youtrack.jetbrains.com/issue/RUST-11520/False-positive-E0106-missing-lifetime-specifier-on-const-str-in-struct.)). It does work without the static, so I can remove it if you want.
P-Storm (Migrated from github.com) reviewed 2025年07月30日 13:55:45 +02:00
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;
P-Storm (Migrated from github.com) commented 2025年07月30日 13:55:45 +02:00
Copy link

Ah, good catch, removed the duplicate and renamed :)

Ah, good catch, removed the duplicate and renamed :)
P-Storm (Migrated from github.com) reviewed 2025年07月30日 14:00:02 +02:00
@ -169,6 +170,44 @@ impl BmdRemoteProtocol for RemoteV0
{
Err(eyre!("Not supported"))
P-Storm (Migrated from github.com) commented 2025年07月30日 14:00:02 +02:00
Copy link

Removed :)

Removed :)
P-Storm (Migrated from github.com) reviewed 2025年07月30日 14:00:43 +02:00
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;
P-Storm (Migrated from github.com) commented 2025年07月30日 14:00:42 +02:00
Copy link

Agreed, and changed.

Agreed, and changed.
P-Storm (Migrated from github.com) reviewed 2025年07月30日 14:04:24 +02:00
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;
P-Storm (Migrated from github.com) commented 2025年07月30日 14:04:24 +02:00
Copy link

Added to RemoteCommands. Only thing is, the + in the START command, should that be also there static?

Added to RemoteCommands. Only thing is, the `+` in the START command, should that be also there static?
dragonmux left a comment
Copy link

Started to try and re-review however hit some things that make further review a little pointless for the moment - we'll let you address these three items and then get back to you with another round once they're addressed so we can more usefully review things for you.

Started to try and re-review however hit some things that make further review a little pointless for the moment - we'll let you address these three items and then get back to you with another round once they're addressed so we can more usefully review things for you.
@ -44,3 +49,1 @@
pubconstREMOTE_RESP_ERR: u8 =b'E';
/// Probe does not support the request made
pubconstREMOTE_RESP_NOTSUP: u8 =b'N';
pubstruct RemoteResponse;

Unsure what happened here, but this one isn't documented now and the constant is misnamed for what it does - this retrieves the target voltage.

Unsure what happened here, but this one isn't documented now and the constant is misnamed for what it does - this retrieves the target voltage.
@ -47,3 +70,4 @@
}
pubtype TargetAddr32=u32;
pubtype TargetAddr64=u64;

NB: this constant is not specific to the response - it sets the request max length too.

NB: this constant is not specific to the response - it sets the request max length too.

This one is part of the request not response - same for EOM above. It's the RESP marker that indicates the start of a response.

This one is part of the request not response - same for EOM above. It's the RESP marker that indicates the start of a response.
@ -25,25 +25,49 @@ mod protocol_v3;
mod protocol_v4;

As it functions as an ack to the GDB engine to complete any outstanding commands before entering remote protocol mode, yes, it needs to be present.

As it functions as an ack to the GDB engine to complete any outstanding commands before entering remote protocol mode, yes, it needs to be present.
This pull request has changes conflicting with the target branch.
  • src/bin/bmputil-cli.rs
  • src/serial/bmd_rsp.rs
  • src/serial/remote/mod.rs
  • src/serial/remote/protocol_v0.rs
  • src/serial/remote/protocol_v1.rs
  • src/serial/remote/protocol_v2.rs
  • src/serial/remote/protocol_v3.rs
  • src/serial/remote/protocol_v4.rs
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin P-Storm/feat/target-power:P-Storm/feat/target-power
git switch P-Storm/feat/target-power
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
blackmagic-debug/bmputil!57
Reference in a new issue
blackmagic-debug/bmputil
No description provided.
Delete branch "P-Storm/feat/target-power"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?