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.
| 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)
| 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