5
3
Fork
You've already forked bmputil
2

CLI Feature: target power #52

Merged
P-Storm merged 6 commits from feat/target-power into main 2025年07月27日 16:25:13 +02:00
P-Storm commented 2025年07月17日 22:08:57 +02:00 (Migrated from github.com)
Copy link

As per #46 , my attempt to take a stab on this.

  • As discussed, this command should only attempt a call on v2 and higher.
  • As discussed, makes only sense to target 1 BMP

Open question:

  • println!("Device target power state: {}", power);, do we want to return println! or info! here?
As per #46 , my attempt to take a stab on this. - As discussed, this command should only attempt a call on v2 and higher. - As discussed, makes only sense to target 1 BMP Open question: - `println!("Device target power state: {}", power);`, do we want to return `println!` or `info!` here?
semanticdiff-com[bot] commented 2025年07月17日 22:09:00 +02:00 (Migrated from github.com)
Copy link

Review changes with SemanticDiff

Changed Files
File Status
src/metadata/firmware_download.rs 100% smaller
src/bmp_matcher.rs 99% smaller
src/bin/bmputil-cli.rs 11% smaller
src/serial/remote/protocol_v2.rs 1% smaller
src/serial/remote/mod.rs 0% smaller
src/serial/remote/protocol_v0.rs 0% smaller
src/serial/remote/protocol_v1.rs 0% smaller
src/serial/remote/protocol_v3.rs 0% smaller
src/serial/remote/protocol_v4.rs 0% smaller
Review changes with &nbsp;<a href="https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes"><picture><source media="(prefers-color-scheme: dark)" srcset="https://app.semanticdiff.com/images/github_button.svg?theme=dark&smaller=31"><source media="(prefers-color-scheme: light)" srcset="https://app.semanticdiff.com/images/github_button.svg?theme=light&smaller=31"><img src="https://app.semanticdiff.com/images/github_button.svg?theme=light&smaller=31" 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/52/changes#src/metadata/firmware_download.rs)&nbsp;&nbsp;[src/metadata/firmware\_download\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/metadata/firmware_download.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=100">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/metadata/firmware_download.rs) &nbsp;100% 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/52/changes#src/bmp_matcher.rs)&nbsp;&nbsp;[src/bmp\_matcher\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/bmp_matcher.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=99">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/bmp_matcher.rs) &nbsp;99% 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/52/changes#src/bin/bmputil-cli.rs)&nbsp;&nbsp;[src/bin/bmputil\-cli\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/bin/bmputil-cli.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=11">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/bin/bmputil-cli.rs) &nbsp;11% 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/52/changes#src/serial/remote/protocol_v2.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v2\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/serial/remote/protocol_v2.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=1">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/serial/remote/protocol_v2.rs) &nbsp;1% 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/52/changes#src/serial/remote/mod.rs)&nbsp;&nbsp;[src/serial/remote/mod\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/serial/remote/mod.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=0">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/serial/remote/mod.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/52/changes#src/serial/remote/protocol_v0.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v0\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/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/52/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/52/changes#src/serial/remote/protocol_v1.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v1\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/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/52/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/52/changes#src/serial/remote/protocol_v3.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v3\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/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/52/changes#src/serial/remote/protocol_v3.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/52/changes#src/serial/remote/protocol_v4.rs)&nbsp;&nbsp;[src/serial/remote/protocol\_v4\.rs](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/serial/remote/protocol_v4.rs) | [<img src="https://app.semanticdiff.com/images/progressbar.svg?progress=0">](https://app.semanticdiff.com/gh/blackmagic-debug/bmputil/pull/52/changes#src/serial/remote/protocol_v4.rs) &nbsp;0% smaller | </details>
dragonmux left a comment
Copy link

Done an initial review - there are a couple of tweaks, but this shouldn't be anything too bad.

We note it appears you got a little confused about which remote command you were encoding - see the final review item.

Done an initial review - there are a couple of tweaks, but this shouldn't be anything too bad. We note it appears you got a little confused about which remote command you were encoding - see the final review item.
@ -2,6 +2,7 @@
// SPDX-FileCopyrightText: 2022-2025 1BitSquared <info@1bitsquared.com>
// SPDX-FileContributor: Written by Mikaela Szekely <mikaela.szekely@qyriad.me>
// SPDX-FileContributor: Modified by Rachel Mant <git@dragonmux.network>

Please make this an addition only - this file was modified by us and you.

Please make this an addition only - this file was modified by us *and* you.

target commands should not be doing anything that can cause problems like this - so please drop this option.

Enums can reference other enums fwiw, so for now we would suggest that Target in ToplevelCommands hold a TargetCommands rather than this struct given removing this option means the command has no option flags, only subcommands.

`target` commands should not be doing anything that can cause problems like this - so please drop this option. Enums can reference other enums fwiw, so for now we would suggest that Target in ToplevelCommands hold a TargetCommands rather than this struct given removing this option means the command has no option flags, only subcommands.

Slight reword suggestion - replace powered Command with power control state

Slight reword suggestion - replace `powered Command` with `power control state`

If you do change the above as suggested, this will become simpler at just

TopLevelCommands::Target(target_command)=>matchtarget_command{
If you do change the above as suggested, this will become simpler at just ```rs TopLevelCommands::Target(target_command) => match target_command { ```

We're surprised at this formatting change.. because it fails to follow the line length rules. Was this formatted with cargo +nightly fmt? File formatting must be done with rustfmt nightly.

We're surprised at this formatting change.. because it fails to follow the line length rules. Was this formatted with `cargo +nightly fmt`? File formatting **must** be done with rustfmt nightly.
@ -20,6 +20,10 @@ pub struct RemoteV2(RemoteV1);
pubstruct RemoteV2JTAG(RemoteV0JTAG);
constREMOTE_JTAG_INIT: &str ="!JS#";

It's not so much if power is used, but rather what voltage is present on the Vtgt pin (given you encoded for REMOTE_VOLTAGE here, not REMOTE_PWR_GET - the latter is "!Gp#").

Please update the name of this string accordingly which is REMOTE_TARGET_VOLTAGE to give it a more proper name than BMD itself does.

The command matching the get_target_power_state() impl below is (again to give it a more proper name) REMOTE_GET_TARGET_POWER_STATE ("!Gp#").

Both are useful, one checks whether the probe is routing power out to Vtgt, the other checks the voltage applied on Vtgt.

It's not so much if power is used, but rather what voltage is present on the Vtgt pin (given you encoded for `REMOTE_VOLTAGE` here, not `REMOTE_PWR_GET` - the latter is "!Gp#"). Please update the name of this string accordingly which is `REMOTE_TARGET_VOLTAGE` to give it a more proper name than BMD itself does. The command matching the `get_target_power_state()` impl below is (again to give it a more proper name) `REMOTE_GET_TARGET_POWER_STATE` ("!Gp#"). Both are useful, one checks whether the probe is routing power out to Vtgt, the other checks the voltage applied on Vtgt.
P-Storm (Migrated from github.com) reviewed 2025年07月22日 12:53:01 +02:00
P-Storm (Migrated from github.com) commented 2025年07月22日 12:53:01 +02:00
Copy link

Currently it can't be changed because of the following structure:

  • #[derive(Subcommand)] ToplevelCommmands
  • #[derive(Args)] TargetArguments
  • #[derive(Subcommand)] TargetCommmands
Currently it can't be changed because of the following structure: - #[derive(Subcommand)] ToplevelCommmands - #[derive(Args)] TargetArguments - #[derive(Subcommand)] TargetCommmands
P-Storm (Migrated from github.com) reviewed 2025年07月22日 12:53:12 +02:00
P-Storm (Migrated from github.com) commented 2025年07月22日 12:53:12 +02:00
Copy link

Done

Done
P-Storm (Migrated from github.com) reviewed 2025年07月22日 12:53:29 +02:00
P-Storm (Migrated from github.com) commented 2025年07月22日 12:53:29 +02:00
Copy link

See above comment

See above comment
P-Storm (Migrated from github.com) reviewed 2025年07月22日 12:53:51 +02:00
P-Storm (Migrated from github.com) commented 2025年07月22日 12:53:51 +02:00
Copy link

It had more changes, with the current commit more has come through.

It had more changes, with the current commit more has come through.
P-Storm (Migrated from github.com) reviewed 2025年07月22日 12:54:17 +02:00
@ -20,6 +20,10 @@ pub struct RemoteV2(RemoteV1);
pubstruct RemoteV2JTAG(RemoteV0JTAG);
constREMOTE_JTAG_INIT: &str ="!JS#";
P-Storm (Migrated from github.com) commented 2025年07月22日 12:54:17 +02:00
Copy link

I think I did the right thing here, but not sure, so please check it out again :)

I think I did the right thing here, but not sure, so please check it out again :)

An enum item in a Subcommand can hold another Subcommand, that is allowed. the Args structure TargetArguments can be eliminated here without breaking things.

An enum item in a Subcommand can hold another Subcommand, that is allowed. the Args structure TargetArguments can be eliminated here without breaking things.
@ -20,6 +20,10 @@ pub struct RemoteV2(RemoteV1);
pubstruct RemoteV2JTAG(RemoteV0JTAG);
constREMOTE_JTAG_INIT: &str ="!JS#";

Very close to - there are two different commands, one which gets what voltage is on the pin (if known) and the other which gets if we're sending power out through it.

Those are, respectively:

constREMOTE_TARGET_VOLTAGE: &str ="!GV#";constREMOTE_GET_TARGET_POWER_STATE: &str ="!Gp#";

You want the latter of these for this particular command (you would be welcome to define both and mark REMOTE_TARGET_VOLTAGE with an allow-unsused directive).

Very close to - there are two different commands, one which gets what voltage is on the pin (if known) and the other which gets if we're sending power out through it. Those are, respectively: ```rs const REMOTE_TARGET_VOLTAGE: &str = "!GV#"; const REMOTE_GET_TARGET_POWER_STATE: &str = "!Gp#"; ``` You want the latter of these for this particular command (you would be welcome to define both and mark REMOTE_TARGET_VOLTAGE with an allow-unsused directive).
dragonmux left a comment
Copy link

This LGTM now - good stuff solving the issues found in review.

We haven't yet tested this locally but the code looks correct and like a reasonable port of the BMDA code for this command, so we have no reason to suspect non-function - thus, merging.

Thank you for the contribution!

This LGTM now - good stuff solving the issues found in review. We haven't yet tested this locally but the code looks correct and like a reasonable port of the BMDA code for this command, so we have no reason to suspect non-function - thus, merging. Thank you for the contribution!
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!52
Reference in a new issue
blackmagic-debug/bmputil
No description provided.
Delete branch "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?