Skip to content

Scatter-shot improvements. #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion mctp-estack/src/fragment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ impl Fragmenter {
/// Returns fragments for the MCTP payload
///
/// The same input message `payload` should be passed to each `fragment()` call.
/// In `SendOutput::Packet(buf)`, `out` is borrowed as the returned fragment, filled with packet contents.
/// In `SendOutput::Packet(buf)`, `out` is borrowed as the returned fragment, filled with
/// packet contents.
///
/// `out` must be at least as large as the specified `mtu`.
pub fn fragment<'f>(
Expand Down
11 changes: 5 additions & 6 deletions mctp-estack/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
//! ## Configuration
//!
//! `mctp-estack` uses fixed sizes to be suitable on no-alloc platforms.
//! These can be configured at build time, see [`config`]
//! These can be configured at build time, see [`config`].

#![cfg_attr(not(feature = "std"), no_std)]
// Temporarily relaxed for zerocopy_channel vendored code.
Expand Down Expand Up @@ -176,9 +176,9 @@ pub struct Stack {
// Unused reassemblers set Reassembler::is_unused()
reassemblers: [(Reassembler, Vec<u8, MAX_PAYLOAD>); NUM_RECEIVE],

/// monotonic time and counter.
/// Monotonic time and counter.
now: EventStamp,
/// cached next expiry time from update()
/// Cached next expiry time from update().
next_timeout: u64,

// Arbitrary counter to make tag allocation more variable.
Expand Down Expand Up @@ -668,8 +668,8 @@ pub struct MctpMessage<'a> {
pub ic: MsgIC,
pub payload: &'a [u8],

/// Cookie is set for response messages when the request had `cookie` set in the [`Stack::start_send`] call.
/// "Response" message refers having `TO` bit unset.
/// Cookie is set for response messages when the request had `cookie` set in the
/// [`Stack::start_send`] call. "Response" message refers having `TO` bit unset.
reassembler: &'a mut Reassembler,

// By default when a MctpMessage is dropped the reassembler will be
Expand Down Expand Up @@ -773,7 +773,6 @@ pub(crate) mod fmt {

#[cfg(test)]
mod tests {

// TODO:
// back to back fragmenter/reassembler

Expand Down
6 changes: 1 addition & 5 deletions mctp-estack/src/serial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,8 @@ impl MctpSerialHandler {
// Complete frame
let packet = &self.rxbuf[2..][..self.rxcount];
return Some(packet);
} else {
warn!(
"Bad checksum got {:04x} calc {:04x}",
cs, cs_calc
);
}
warn!("Bad checksum got {:04x} calc {:04x}", cs, cs_calc);
} else {
// restart
self.rxpos = Pos::SerialRevision;
Expand Down
8 changes: 2 additions & 6 deletions mctp-usb-embassy/src/mctpusb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,7 @@ impl<'d, D: Driver<'d>> Receiver<'d, D> {
// Refill
let l = match self.ep.read(&mut self.buf).await {
Ok(l) => l,
Err(_e) => {
return None;
}
Err(_e) => return None,
};
self.remaining = Range { start: 0, end: l };
}
Expand All @@ -158,9 +156,7 @@ impl<'d, D: Driver<'d>> Receiver<'d, D> {
let rem = &self.buf[self.remaining.clone()];
let (pkt, rem) = match MctpUsbHandler::decode(rem) {
Ok(a) => a,
Err(e) => {
return Some(Err(e));
}
Err(e) => return Some(Err(e)),
};
self.remaining.start = self.remaining.end - rem.len();
Some(Ok(pkt))
Expand Down
2 changes: 1 addition & 1 deletion pldm-file/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ where
let resp_data_len = read_resp.len as usize;

if rest.len() != resp_data_len + 4 {
return Err(proto_error!("invalid resonse data length"));
return Err(proto_error!("invalid response data length"));
}

let (resp_data, resp_cs) = rest.split_at(resp_data_len);
Expand Down
14 changes: 5 additions & 9 deletions pldm-file/src/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ impl<const N: usize> Responder<N> {
&mut self,
mut comm: R,
req: &PldmRequest<'_>,
host: &mut impl Host,
Copy link
Member

Choose a reason for hiding this comment

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

So we currently have this under the assumption that anything more complex than the current (ie., very basic) file host implementation would need interactions with the host at least for DfOpen operations.

I am happy to remove it now, but we'd likely need it back shortly - which would be an API-breaking change. That said, that change would also modify the Host trait, so we'd have breakages there anyway.

Removing the host arg from the cmd_ handlers is fine though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't removed it, precisely because it's public. Changing from host to _host is just needed to prevent the compiler complaining about the unused arg.

Copy link
Member

Choose a reason for hiding this comment

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

hah, I had been focusing all on the - side of the series. All good then!

_host: &mut impl Host,
) -> pldm::Result<()> {
if req.typ != PLDM_TYPE_FILE_TRANSFER {
trace!("pldm-fw non-pldm-fw request {req:?}");
Expand All @@ -125,9 +125,9 @@ impl<const N: usize> Responder<N> {
};

let r = match cmd {
Cmd::DfProperties => self.cmd_dfproperties(req, host),
Cmd::DfOpen => self.cmd_dfopen(req, host),
Cmd::DfClose => self.cmd_dfclose(req, host),
Cmd::DfProperties => self.cmd_dfproperties(req),
Cmd::DfOpen => self.cmd_dfopen(req),
Cmd::DfClose => self.cmd_dfclose(req),
_ => {
trace!("unhandled command {cmd:?}");
Err(CCode::ERROR_UNSUPPORTED_PLDM_CMD.into())
Expand Down Expand Up @@ -207,7 +207,6 @@ impl<const N: usize> Responder<N> {
fn cmd_dfproperties<'a>(
&mut self,
req: &'a PldmRequest<'a>,
_host: &mut impl Host,
) -> Result<PldmResponse<'a>> {
let (rest, dfp) = DfPropertiesReq::from_bytes((&req.data, 0))?;

Expand All @@ -234,7 +233,6 @@ impl<const N: usize> Responder<N> {
fn cmd_dfopen<'a>(
&mut self,
req: &'a PldmRequest<'a>,
_host: &mut impl Host,
) -> Result<PldmResponse<'a>> {
let (rest, dfo) = DfOpenReq::from_bytes((&req.data, 0))?;

Expand All @@ -254,8 +252,7 @@ impl<const N: usize> Responder<N> {
let id = self
.files
.iter()
.enumerate()
.find_map(|(n, e)| if e.is_none() { Some(n) } else { None })
.position(|e| e.is_none())
.ok_or(file_ccode::MAX_NUM_FDS_EXCEEDED)?;

self.files[id].replace(file_ctx);
Expand All @@ -273,7 +270,6 @@ impl<const N: usize> Responder<N> {
fn cmd_dfclose<'a>(
&mut self,
req: &'a PldmRequest<'a>,
_host: &mut impl Host,
) -> Result<PldmResponse<'a>> {
let (rest, dfc) = DfCloseReq::from_bytes((&req.data, 0))?;

Expand Down
19 changes: 7 additions & 12 deletions pldm-fw-cli/src/bin/pldm-fw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,13 @@ use anyhow::{bail, Context};
use argh::FromArgs;
use enumset::{EnumSet, EnumSetType};
use mctp_linux::{MctpAddr, MctpLinuxListener};
use std::fmt::Write as _;
use std::io::Write;

fn comma_separated<T: EnumSetType + std::fmt::Debug>(e: EnumSet<T>) -> String {
let mut s = String::new();
let mut first = true;
for i in e.iter() {
write!(s, "{}{:?}", if first { "" } else { "," }, i).unwrap();
first = false;
}
s
e.iter()
.map(|t| format!("{t:?}"))
.collect::<Vec<_>>()
.join(",")
}

fn print_device_info(
Expand Down Expand Up @@ -112,10 +108,9 @@ fn extract_component(
pkg: &pldm_fw::pkg::Package,
idx: usize,
) -> anyhow::Result<()> {
if idx >= pkg.components.len() {
bail!("no component with index {}", idx);
}
let comp = &pkg.components[idx];
let Some(comp) = pkg.components.get(idx) else {
bail!("no component with index {}", idx)
};

let fname = format!("component-{}.{:04x}.bin", idx, comp.identifier);
let mut f = std::fs::File::create(&fname)
Expand Down
27 changes: 16 additions & 11 deletions pldm-fw/src/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,17 +167,21 @@ impl<R: RespChannel> Responder<R> {
// Check for consistent EID
match cmd {
// informational commands or Cancel always allowed
| Cmd::QueryDeviceIdentifiers
Cmd::QueryDeviceIdentifiers
| Cmd::GetFirmwareParameters
| Cmd::GetStatus
| Cmd::CancelUpdate
// RequestUpdate will check itself
| Cmd::RequestUpdate
=> (),
| Cmd::RequestUpdate => (),
_ => {
if self.ua_eid != Some(eid) {
debug!("Ignoring {cmd:?} from mismatching EID {eid}, expected {:?}", self.ua_eid);
self.reply_error(req, &mut comm,
debug!(
"Ignoring {cmd:?} from mismatching EID {eid}, expected {:?}",
self.ua_eid
);
self.reply_error(
req,
&mut comm,
CCode::ERROR_NOT_READY as u8,
);
}
Expand Down Expand Up @@ -243,7 +247,7 @@ impl<R: RespChannel> Responder<R> {
req_comm,
req_pending,
..
} => req_pending.then(|| req_comm),
} => req_pending.then_some(req_comm),
_ => None,
};
trace!("pending reply {}", r.is_some());
Expand All @@ -268,14 +272,15 @@ impl<R: RespChannel> Responder<R> {

match Cmd::from_u8(rsp.cmd) {
Some(Cmd::RequestFirmwareData) => self.download_response(rsp, d),
| Some(Cmd::TransferComplete)
| Some(Cmd::VerifyComplete)
| Some(Cmd::ApplyComplete)

// Ignore replies to these requests.
// We may have already moved on to a later state
// and don't have any useful retry for them.
=> Ok(()),
_ => Err(proto_error!("Unsupported PLDM response"))
Some(Cmd::TransferComplete)
| Some(Cmd::VerifyComplete)
| Some(Cmd::ApplyComplete) => Ok(()),

_ => Err(proto_error!("Unsupported PLDM response")),
}
}

Expand Down
1 change: 0 additions & 1 deletion pldm-fw/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1342,7 +1342,6 @@ pub struct UpdateTransferProgress {

#[cfg(test)]
mod tests {

use crate::*;

#[test]
Expand Down
24 changes: 7 additions & 17 deletions pldm-fw/src/pkg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,25 +70,15 @@ impl<'a> ComponentBitmap {
}

pub fn as_index_str(&self) -> String {
let mut s = String::new();
let mut first = true;
for i in 0usize..self.n_bits {
if self.bit(i) {
s.push_str(&format!("{}{}", if first { "" } else { ", " }, i));
first = false;
}
}
s
(0usize..self.n_bits)
.filter(|&i| self.bit(i))
.map(|i| i.to_string())
.collect::<Vec<_>>()
.join(", ")
}

pub fn as_index_vec(&self) -> Vec<usize> {
let mut v = Vec::new();
for i in 0usize..self.n_bits {
if self.bit(i) {
v.push(i)
}
}
v
(0usize..self.n_bits).filter(|&i| self.bit(i)).collect()
}
}

Expand Down Expand Up @@ -232,7 +222,7 @@ impl Package {
.finish()
.map_err(|_| PldmPackageError::new_format("can't parse devices"))?;

/* this is the first divegence in package format versions; the
/* this is the first divergence in package format versions; the
* downstream device identification area is only present in 1.1.x
*/
let r = match identifier {
Expand Down
4 changes: 2 additions & 2 deletions pldm/src/control/responder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ impl TypeData {
}
}

// number of peers that we track negotiated parameters against.
// Number of peers that we track negotiated parameters against.
const N_PEERS: usize = 8;

// per-peer data on Negotiate Transfer Parameters results.
// Per-peer data on Negotiate Transfer Parameters results.
//
// The current implementation is spec-volatingly basic: we don't handle per-type
// data, but just update our negotiated size across all types.
Expand Down
1 change: 0 additions & 1 deletion pldm/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@ impl<S> NoneNoSpace<S> for Option<S> {

#[cfg(test)]
mod tests {

use crate::*;

#[test]
Expand Down
3 changes: 1 addition & 2 deletions standalone/examples/echo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ use log::LevelFilter;
use mctp::{Eid, Listener, MsgType, RespChannel};
use mctp_standalone::MctpSerialListener;

/** mctp serial echo
*/
/// mctp serial echo
#[derive(argh::FromArgs)]
struct Args {
#[argh(switch, short = 'v')]
Expand Down
3 changes: 1 addition & 2 deletions standalone/examples/req.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ use log::LevelFilter;
use mctp::{Eid, MsgType, ReqChannel};
use mctp_standalone::MctpSerialReq;

/** mctp serial echo
*/
/// mctp serial echo
#[derive(argh::FromArgs)]
struct Args {
#[argh(switch, short = 'v')]
Expand Down
1 change: 0 additions & 1 deletion standalone/src/serial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use log::{debug, error, info, trace, warn};
use core::time::Duration;
use std::time::Instant;

// use embedded_io::Write;
use embedded_io_async::{Read, Write};
use smol::future::FutureExt;
use smol::Timer;
Expand Down