Skip to content

Commit

Permalink
hiffy: stm32g0 i2c size grinding
Browse files Browse the repository at this point in the history 
This knocks 256 bytes off I2C support (text), so we can potentially
enable it on G030.

More savings are potentially available in this code, and we've got a lot
of duplication across targets that means we can probably save space on
other machines as well.

The techniques applied here:

1. Find larger chunks of repeated code and factor them into a common
   routine (the i2c_args -> i2c_args_to_device transition)
2. Find duplicate checks for things like bounds or slicing and make them
   occur in only one place, reusing the result. (e.g. when slicing like
   buf[..n], we can assign the result to a variable and reuse it -- and
   we can combine this with the check by writing buf.get(..n).)
3. Find unhandled integer overflow cases and convert them to more
   graceful failures (currently, clients can trivially crash Hiffy by
   sending large integers for many parameters; this reduces their
   number)
4. Apply shared subexpression elimination to things like the frame
   pointer / base pointer calculation in the write routine. Previously
   they were computed separately, which didn't make it clear to the
   compiler that they're trivially related, causing it to generate
   excess checks.

I've also applied a lot more 'ok_or' and '?' patterns to remove a lot of
manual match statements on errors, under the theory that the size of the
code on the page ought to more accurately reflect its
importance/complexity in the domain (I2C). I think this change makes the
actual sources of complexity more apparent.
  • Loading branch information
@cbiffle
cbiffle committed Sep 19, 2023
1 parent ab76258 commit 36147d0
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 86 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

137 changes: 52 additions & 85 deletions task/hiffy/src/stm32g0.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,64 +72,48 @@ pub enum Functions {
),
}

/// Expected stack frame:
/// ```text
/// [0] controller index (integer)
/// [1] port index (integer)
/// [2] mux index (optional integer)
/// [3] segment index (optional integer)
/// [4] i2c address (integer)
/// [5] register index (optional integer)
/// ```
#[cfg(feature = "i2c")]
#[allow(clippy::type_complexity)] // TODO - type is indeed not fantastic
fn i2c_args(
fn i2c_args_to_device(
stack: &[Option<u32>],
) -> Result<
(
Controller,
PortIndex,
Option<(Mux, Segment)>,
u8,
Option<u8>,
),
Failure,
> {
let controller = match stack[0] {
Some(controller) => match Controller::from_u32(controller) {
Some(controller) => controller,
None => return Err(Failure::Fault(Fault::BadParameter(0))),
},
None => return Err(Failure::Fault(Fault::EmptyParameter(0))),
};

let port = match stack[1] {
Some(port) => {
if port > core::u8::MAX.into() {
return Err(Failure::Fault(Fault::BadParameter(1)));
}
) -> Result<(I2cDevice, Option<u8>), Failure> {
let controller =
Controller::from_u32(stack[0].ok_or(Fault::EmptyParameter(0))?)
.ok_or(Fault::BadParameter(0))?;

PortIndex(port as u8)
}
None => {
//
// While we once upon a time allowed HIF consumers to specify
// a default port, we now expect all HIF consumers to read the
// device configuration and correctly specify a port index:
// this is an error.
//
return Err(Failure::Fault(Fault::EmptyParameter(1)));
}
//
// While we once upon a time allowed HIF consumers to specify a default
// port, we now expect all HIF consumers to read the device configuration
// and correctly specify a port index: this is an error.
//
let port = stack[1].ok_or(Fault::EmptyParameter(1))?;
let port = if port > core::u8::MAX.into() {
return Err(Failure::Fault(Fault::BadParameter(1)));
} else {
PortIndex(port as u8)
};

let mux = match (stack[2], stack[3]) {
(Some(mux), Some(segment)) => Some((
Mux::from_u32(mux).ok_or(Failure::Fault(Fault::BadParameter(2)))?,
Segment::from_u32(segment)
.ok_or(Failure::Fault(Fault::BadParameter(3)))?,
Mux::from_u32(mux).ok_or(Fault::BadParameter(2))?,
Segment::from_u32(segment).ok_or(Fault::BadParameter(3))?,
)),
_ => None,
};

let addr = match stack[4] {
Some(addr) => addr as u8,
None => return Err(Failure::Fault(Fault::EmptyParameter(4))),
};

let addr = stack[4].ok_or(Fault::EmptyParameter(4))? as u8;
let register = stack[5].map(|r| r as u8);

Ok((controller, port, mux, addr, register))
let task = I2C.get_task_id();
Ok((I2cDevice::new(task, controller, port, mux, addr), register))
}

#[cfg(feature = "i2c")]
Expand All @@ -143,41 +127,33 @@ fn i2c_read(
}

let fp = stack.len() - 7;
let (controller, port, mux, addr, register) = i2c_args(&stack[fp..])?;

let task = I2C.get_task_id();
let device = I2cDevice::new(task, controller, port, mux, addr);
let (device, register) = i2c_args_to_device(&stack[fp..])?;

match stack[fp + 6] {
Some(nbytes) => {
let n = nbytes as usize;

if rval.len() < n {
let Some(buf) = rval.get_mut(..n) else {
return Err(Failure::Fault(Fault::ReturnValueOverflow));
}
};

let res = if let Some(reg) = register {
device.read_reg_into::<u8>(reg as u8, &mut rval[0..n])
device.read_reg_into::<u8>(reg as u8, buf)
} else {
device.read_into(&mut rval[0..n])
device.read_into(buf)
};

match res {
Ok(rlen) => Ok(rlen),
Err(err) => Err(Failure::FunctionError(err.into())),
}
res.map_err(|e| Failure::FunctionError(u32::from(e)))
}

None => {
if let Some(reg) = register {
if rval.len() < 256 {
let Some(buf) = rval.get_mut(..256) else {
return Err(Failure::Fault(Fault::ReturnValueOverflow));
}
};

match device.read_block::<u8>(reg as u8, &mut rval[0..0xff]) {
Ok(rlen) => Ok(rlen),
Err(err) => Err(Failure::FunctionError(err.into())),
}
device
.read_block::<u8>(reg as u8, buf)
.map_err(|e| Failure::FunctionError(u32::from(e)))
} else {
Err(Failure::Fault(Fault::EmptyParameter(6)))
}
Expand Down Expand Up @@ -205,16 +181,14 @@ fn i2c_write(
Some(len) if len > 0 && (len as usize) < buf.len() => Ok(len as usize),
_ => Err(Failure::Fault(Fault::BadParameter(7))),
}?;
let frame_size = len.checked_add(7).ok_or(Fault::BadParameter(7))?;

if stack.len() < 7 + len {
return Err(Failure::Fault(Fault::MissingParameters));
}

let fp = stack.len() - (7 + len);
let (controller, port, mux, addr, register) = i2c_args(&stack[fp..])?;

let task = I2C.get_task_id();
let device = I2cDevice::new(task, controller, port, mux, addr);
let fp = stack
.len()
.checked_sub(frame_size)
.ok_or(Fault::MissingParameters)?;
let frame = &stack[fp..];
let (device, register) = i2c_args_to_device(frame)?;

let mut offs = 0;

Expand All @@ -223,18 +197,14 @@ fn i2c_write(
offs += 1;
}

let bp = stack.len() - (1 + len);
let bp = 6;
let bufsec = &mut buf[..len + offs];

for i in 0..len {
buf[i + offs] = match stack[bp + i] {
None => {
return Err(Failure::Fault(Fault::BadParameter(7)));
}
Some(val) => val as u8,
}
bufsec[i + offs] = frame[bp + i].ok_or(Fault::BadParameter(7))? as u8;
}

match device.write(&buf[0..len + offs]) {
match device.write(bufsec) {
Ok(_) => Ok(0),
Err(err) => Err(Failure::FunctionError(err.into())),
}
Expand Down Expand Up @@ -268,15 +238,12 @@ fn i2c_bulk_write(
}?;

let fp = stack.len() - 8;
let (controller, port, mux, addr, register) = i2c_args(&stack[fp..])?;
let (device, register) = i2c_args_to_device(&stack[fp..])?;

if register.is_some() {
return Err(Failure::Fault(Fault::BadParameter(5)));
}

let task = I2C.get_task_id();
let device = I2cDevice::new(task, controller, port, mux, addr);

match device.write(&data[offset..offset + len]) {
Ok(_) => Ok(0),
Err(err) => Err(Failure::FunctionError(err.into())),
Expand Down

0 comments on commit 36147d0

Please sign in to comment.