Enhance bmputil-cli target power
#57
Conversation
|
Review changes with Changed Files
|
dragonmux
left a comment
dragonmux
left a comment
There was a problem hiding this comment.
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.
src/serial/remote/mod.rs
Outdated
| fn supported_families(&self) -> Result<Option<TargetFamily>>; | ||
| fn get_target_power_state(&self) -> Result<bool>; | ||
| fn get_target_voltage(&self) -> Result<f32>; | ||
| fn get_srst_val(&self) -> Result<bool>; |
There was a problem hiding this comment.
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)
src/serial/remote/mod.rs
Outdated
| pub const JTAG_INIT: &'static str = "!JS#"; | ||
| pub const NRST_GET: &'static str = "!Gz#"; | ||
| pub const NRST_SET: &'static str = "!GZ#"; | ||
| pub const PWR_GET: &'static str = "!Gp#"; |
There was a problem hiding this comment.
Duplicates the first constant, and the SET variant should be named the same way - ie SET_TARGET_POWER_STATE.
There was a problem hiding this comment.
Ah, good catch, removed the duplicate and renamed :)
dragonmux
left a comment
dragonmux
left a comment
There was a problem hiding this comment.
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.
| /// This is the max possible size of a remote protocol packet which a hard limitation of the | ||
| /// firmware on the probe - 1KiB is all the buffer that could be spared. | ||
| pub const MAX_MSG_SIZE: usize = 1024; |
There was a problem hiding this comment.
NB: this constant is not specific to the response - it sets the request max length too.
| pub const HL_CHECK: &str = "!HC#"; | ||
| /// This command asks the probe to initialise JTAG comms to any connected targets | ||
| pub const JTAG_INIT: &str = "!JS#"; | ||
| pub const NRST_TARGET_VOLTAGE: &str = "!GV#"; |
There was a problem hiding this comment.
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.
| /// Probe found an error with a request parameter | ||
| pub const RESP_PARERR: u8 = b'P'; | ||
| /// Start of message marker for the protocol | ||
| pub const SOM: u8 = b'!'; |
There was a problem hiding this comment.
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.
3 participants
Adding more information for command
bmputil-cli target power:Also made
Device target power statea warning if the version is too low.Added preparation for
REMOTE_SRSTandREMOTE_PWR