Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,017 changes: 852 additions & 165 deletions Cargo.lock

Large diffs are not rendered by default.

11 changes: 5 additions & 6 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,12 @@ vsc7448-types = { git = "https://github.com/oxidecomputer/vsc7448.git" }

#
# We depend on the oxide-stable branch of Oxide's fork of probe-rs to assure
# that we can float necessary patches on probe-rs.
# that we can float necessary patches on probe-rs.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: linewrap this whole block?

#
probe-rs = { git = "https://github.com/oxidecomputer/probe-rs.git", branch = "oxide-v0.12.0" }
# As of this commit the patches are pretty minimal: an updated nusb branch
# with illumos support and some tweaks to avoid the need to pull in
# serialport. The dream would still be to run a regular probe-rs release.
probe-rs = { git = "https://github.com/oxidecomputer/probe-rs.git", branch = "oxide_v0.31.0", default-features = false, features = ["builtin-targets"] }

# Local `path`-based deps
humility = { path = "./humility-core", package = "humility-core" }
Expand Down Expand Up @@ -263,7 +266,3 @@ debug = true
inherits = "release"
debug = false
debug-assertions = true

[patch.crates-io]
libusb1-sys = { git = "https://github.com/oxidecomputer/rusb", branch = "probe-rs-0.12-libusb-v1.0.26" }
hidapi = { git = "https://github.com/oxidecomputer/hidapi-rs", branch = "oxide-stable" }
75 changes: 29 additions & 46 deletions cmd/debugmailbox/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,18 @@ use byteorder::{ByteOrder, LittleEndian, WriteBytesExt};
use clap::{CommandFactory, Parser};
use humility_cli::ExecutionContext;
use humility_cmd::Command;
use probe_rs::{
DebugProbeError, DebugProbeSelector, Probe,
architecture::arm::{ApAddress, ArmProbeInterface, DapError, DpAddress},
use probe_rs::architecture::arm::{
ArmDebugInterface, ArmError, DapError, FullyQualifiedApAddress,
sequences::DefaultArmSequence,
};
use probe_rs::probe::DebugProbeSelector;

// The debug mailbox registers
// See 51.5.5.1 of Rev 2.4 of the LPC55 manual
const CSW: u8 = 0x0;
const REQUEST: u8 = 0x4;
const RETURN: u8 = 0x8;
const IDR: u8 = 0xFC;
const CSW: u64 = 0x0;
const REQUEST: u64 = 0x4;
const RETURN: u64 = 0x8;
const IDR: u64 = 0xFC;

// Are we talking to the wrong chip?
const SP_IDR: u32 = 0x54770002;
Expand Down Expand Up @@ -104,9 +105,9 @@ enum DebugMailboxCmd {
}

fn poll_raw_ap_register(
probe: &mut Box<dyn ArmProbeInterface + '_>,
ap: &ApAddress,
addr: u8,
probe: &mut Box<dyn ArmDebugInterface + '_>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered if we could remove the Box here (in favor of a &mut dyn ArmDebugInterface), but poking at it shows that it would ripple through a bunch of places, so out of scope for this PR.

ap: &FullyQualifiedApAddress,
addr: u64,
mut f: impl FnMut(u32) -> Result<bool>,
mut timeout_ms: usize,
) -> Result<u32> {
Expand All @@ -118,30 +119,13 @@ fn poll_raw_ap_register(
// this initial delay.
std::thread::sleep(std::time::Duration::from_millis(10));

match probe.read_raw_ap_register(*ap, addr) {
match probe.read_raw_ap_register(ap, addr) {
Ok(val) => match f(val) {
Ok(true) => return Ok(val),
Ok(false) => (),
Err(e) => return Err(e),
},
Err(DebugProbeError::ArchitectureSpecific(arch_err)) => {
match arch_err.downcast::<DapError>() {
Ok(dap_err) => match *dap_err {
DapError::WaitResponse => {}
e => {
return Err(DebugProbeError::ArchitectureSpecific(
e.into(),
)
.into());
}
},
Err(e) => {
return Err(
DebugProbeError::ArchitectureSpecific(e).into()
);
}
}
}
Err(ArmError::Dap(DapError::WaitResponse)) => {}
Err(x) => return Err(x.into()),
}

Expand All @@ -155,12 +139,12 @@ fn poll_raw_ap_register(
}

fn alive<'a>(
probe: &mut Box<dyn ArmProbeInterface + 'a>,
addr: &ApAddress,
probe: &mut Box<dyn ArmDebugInterface + 'a>,
addr: &FullyQualifiedApAddress,
reset: bool,
) -> Result<()> {
if reset {
probe.write_raw_ap_register(*addr, CSW, 0x21)?;
probe.write_raw_ap_register(addr, CSW, 0x21)?;
println!("Resetting chip via SYSREQRESET!");
}

Expand All @@ -171,11 +155,11 @@ fn alive<'a>(
}

fn write_request_reg<'a>(
probe: &mut Box<dyn ArmProbeInterface + 'a>,
addr: &ApAddress,
probe: &mut Box<dyn ArmDebugInterface + 'a>,
addr: &FullyQualifiedApAddress,
val: u32,
) -> Result<()> {
probe.write_raw_ap_register(*addr, REQUEST, val)?;
probe.write_raw_ap_register(addr, REQUEST, val)?;

let _ = poll_raw_ap_register(
probe,
Expand All @@ -198,8 +182,8 @@ fn write_request_reg<'a>(
}

fn write_req<'a>(
probe: &mut Box<dyn ArmProbeInterface + 'a>,
addr: &ApAddress,
probe: &mut Box<dyn ArmDebugInterface + 'a>,
addr: &FullyQualifiedApAddress,
command: DMCommand,
args: &[u32],
) -> Result<Vec<u32>> {
Expand Down Expand Up @@ -258,8 +242,8 @@ fn write_req<'a>(
}

fn read_return<'a>(
probe: &mut Box<dyn ArmProbeInterface + 'a>,
addr: &ApAddress,
probe: &mut Box<dyn ArmDebugInterface + 'a>,
addr: &FullyQualifiedApAddress,
) -> Result<u32> {
poll_raw_ap_register(probe, addr, RETURN, |_| Ok(true), 1000)
.context("Reading debugmailbox RETURN register")
Expand All @@ -269,14 +253,15 @@ fn debugmailboxcmd(context: &mut ExecutionContext) -> Result<()> {
let subargs = DebugMailboxArgs::try_parse_from(&context.cli.cmd)?;

// Get a list of all available debug probes.
let probes = Probe::list_all();
let list = probe_rs::probe::list::Lister::new();
let probes = list.list_all();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I love the smell of fresh API design in the morning


if probes.is_empty() {
bail!("No probes found!");
}

let mut probe = match &context.cli.probe {
Some(p) => match TryInto::<DebugProbeSelector>::try_into(p.clone()) {
Some(p) => match p.parse::<DebugProbeSelector>() {
Ok(selector) => {
let vid = selector.vendor_id;
let pid = selector.product_id;
Expand All @@ -301,12 +286,10 @@ fn debugmailboxcmd(context: &mut ExecutionContext) -> Result<()> {

probe.attach_to_unspecified()?;
let mut iface = probe
.try_into_arm_interface()
.unwrap()
.initialize_unspecified()
.try_into_arm_debug_interface(DefaultArmSequence::create())
.unwrap();

let dm_port = ApAddress { dp: DpAddress::Default, ap: 2 };
let dm_port = FullyQualifiedApAddress::v1_with_default_dp(2);

// Check if this is a debug mailbox. This is based on the sequence from
// the LPC55 Debug Mailbox user manual
Expand All @@ -315,7 +298,7 @@ fn debugmailboxcmd(context: &mut ExecutionContext) -> Result<()> {
// handles this for us

// Check the IDR
let val = iface.read_raw_ap_register(dm_port, IDR)?;
let val = iface.read_raw_ap_register(&dm_port, IDR)?;

if val != DM_ID {
if val == SP_IDR {
Expand Down
2 changes: 1 addition & 1 deletion cmd/rendmp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ use std::io::Write;
use std::io::prelude::*;
use std::thread;
use std::time::{Duration, Instant};
use strum::VariantNames;
use strum::VariantNames as _;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spooky

use strum_macros::VariantNames;
use zerocopy::{FromBytes, IntoBytes};

Expand Down
1 change: 0 additions & 1 deletion humility-probes-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,3 @@ num-traits.workspace = true
parse_int.workspace = true
probe-rs.workspace = true
thiserror.workspace = true
rusb.workspace = true
78 changes: 45 additions & 33 deletions humility-probes-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use ::probe_rs::{
use ::probe_rs::Permissions;
use ::probe_rs::probe::{
Comment thread
labbott marked this conversation as resolved.
Outdated
DebugProbeError, DebugProbeInfo, DebugProbeSelector, Probe,
ProbeCreationError,
};
Expand Down Expand Up @@ -34,7 +35,8 @@ fn parse_probe(probe: &str) -> (&str, Option<usize>) {
}

fn get_usb_probe(index: Option<usize>) -> Result<DebugProbeInfo> {
let probes = Probe::list_all();
let lister = ::probe_rs::probe::list::Lister::new();
let probes = lister.list_all();

if probes.is_empty() {
return Err(ProbeError::NoProbeFound.into());
Expand All @@ -60,43 +62,31 @@ fn get_usb_probe(index: Option<usize>) -> Result<DebugProbeInfo> {
}
}

/// [`probe_rs::Probe::open`] with specialized error messages and speed
/// configuration
fn open_probe<T: Into<DebugProbeSelector> + Clone>(
selector: T,
fn open_probe_from_selector(
selector: DebugProbeSelector,
speed_khz: Option<u32>,
) -> Result<Probe> {
let probe_selector: DebugProbeSelector = selector.clone().into();
let res = Probe::open(selector);

// the following error customizations could be a match statement but until
// if let guards stabilize it would be a kludge
let lister = ::probe_rs::probe::list::Lister::new();
let res = lister.open(selector.clone());

if let Err(DebugProbeError::ProbeCouldNotBeCreated(
ProbeCreationError::NotFound,
)) = res
{
if probe_selector.serial_number.is_some() {
if selector.serial_number.is_some() {
bail!(
"Could not find probe {}.\n\
\n\
Because a serial number is present, this may be due to not \
running humility with permission to read USB device serial \
numbers; if not root already, run again as root?",
probe_selector
selector
);
} else {
bail!("Could not find probe {}.", probe_selector);
bail!("Could not find probe {}.", selector);
}
}

if let Err(DebugProbeError::Usb(Some(ref err))) = res
&& let Some(rcode) = err.downcast_ref::<rusb::Error>()
&& *rcode == rusb::Error::Busy
{
bail!("USB link in use; is OpenOCD or another debugger running?");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the Usb variant still exists, should we keep this block?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now get back a raw std::io::Error which does not have downcast_ref, only downcast (https://doc.rust-lang.org/stable/std/io/struct.Error.html) and it was faster to just pull it out when I was doing the upgrade. It wasn't clear if there was still value in keeping it. Looking again, the flow of this entire function is weird so I'm going to take a pass to see if I can make it more reasonable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some tweaks and I think I brought it back. I'm still not clear on the value but if we run into more errors that need better messages we can add more.

let mut probe = res?;

if let Some(speed) = speed_khz {
Expand All @@ -117,7 +107,7 @@ pub fn attach_to_probe(
"usb" => {
let probe_info = get_usb_probe(index)?;

let probe = open_probe(&probe_info, speed_khz)?;
let probe = probe_info.open()?;
Comment thread
labbott marked this conversation as resolved.
Outdated

crate::msg!("Opened probe {}", probe_info.identifier);
Ok(Box::new(unattached::UnattachedCore::new(
Expand All @@ -129,13 +119,13 @@ pub fn attach_to_probe(
)))
}
"auto" => attach_to_probe("usb", speed_khz),
_ => match TryInto::<DebugProbeSelector>::try_into(probe) {
_ => match probe.parse::<DebugProbeSelector>() {
Ok(selector) => {
let vidpid = probe;
let vid = selector.vendor_id;
let pid = selector.product_id;
let serial = selector.serial_number.clone();
let probe = open_probe(selector, speed_khz)?;
let probe = open_probe_from_selector(selector, speed_khz)?;
let name = probe.get_name();

crate::msg!("Opened {vidpid} via {name}");
Expand All @@ -160,20 +150,30 @@ pub fn attach_to_chip(
"usb" => {
let probe_info = get_usb_probe(index)?;

let probe = open_probe(&probe_info, speed_khz)?;
let probe = probe_info.open()?;

let name = probe.get_name();
//
// probe-rs needs us to specify a chip that it knows about -- but
// it only really uses this information for flashing the part. If
// we are attaching to the part for not pusposes of flashing, we
// specify a generic ARMv7-M (but then we also indicate that can't

// specify a generic Cortex-M3 (but then we also indicate that can't
// flash to assure that we can fail explicitly should flashing be
// attempted).
//
let (session, can_flash) = match chip {
Some(chip) => (probe.attach(chip)?, true),
None => (probe.attach("armv7m")?, false),
Some(chip) => (
probe.attach(chip, Permissions::new().allow_erase_all())?,
true,
),
None => (
probe.attach(
"Cortex-M3",
Permissions::new().allow_erase_all(),
Comment thread
labbott marked this conversation as resolved.
Outdated
)?,
false,
),
};

crate::msg!("attached via {name}");
Expand All @@ -189,24 +189,36 @@ pub fn attach_to_chip(
}
"auto" => attach_to_chip("usb", chip, speed_khz),

_ => match TryInto::<DebugProbeSelector>::try_into(probe) {
_ => match probe.parse::<DebugProbeSelector>() {
Ok(selector) => {
let vidpid = probe;

let vid = selector.vendor_id;
let pid = selector.product_id;
let serial = selector.serial_number.clone();

let probe = open_probe(selector, speed_khz)?;
let probe = open_probe_from_selector(selector, speed_khz)?;
let name = probe.get_name();

//
// See the block comment in the generic "usb" attach for
// why we use armv7m here.
// why we use Cortex-M3 here.
//
let (session, can_flash) = match chip {
Some(chip) => (probe.attach(chip)?, true),
None => (probe.attach("armv7m")?, false),
Some(chip) => (
probe.attach(
chip,
Permissions::new().allow_erase_all(),
)?,
true,
),
None => (
probe.attach(
"Cortex-M3",
Permissions::new().allow_erase_all(),
Comment thread
labbott marked this conversation as resolved.
Outdated
)?,
false,
),
};

crate::msg!("attached to {vidpid} via {name}");
Expand Down
Loading
Loading