From 8bdaa72bc3a4be098c11d56b7cd3520c5ae1cc90 Mon Sep 17 00:00:00 2001 From: Stephen Waits Date: Tue, 7 Apr 2026 08:59:10 -0600 Subject: [PATCH] refactor: deduplicate USB/UART protocol framing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Made protocol_io.rs compile for all boards (not just heltec_v3) - Renamed FrameAccumulator → CobsDecoder, encode_response → cobs_encode_response - Refactored USB task to use shared CobsDecoder and route_command from protocol_io - Removed ~80 lines of duplicated COBS framing and command routing from usb/task.rs - Single canonical MAX_FRAME definition in protocol_io.rs --- src/main.rs | 2 +- src/protocol_io.rs | 8 ++-- src/uart/task.rs | 10 ++--- src/usb/task.rs | 110 +++++++-------------------------------------- 4 files changed, 25 insertions(+), 105 deletions(-) diff --git a/src/main.rs b/src/main.rs index c9d9f47..6802886 100644 --- a/src/main.rs +++ b/src/main.rs @@ -24,7 +24,7 @@ mod display; mod protocol; #[cfg(not(test))] mod radio; -#[cfg(all(not(test), feature = "heltec_v3"))] +#[cfg(not(test))] mod protocol_io; #[cfg(all(not(test), feature = "heltec_v3"))] mod uart; diff --git a/src/protocol_io.rs b/src/protocol_io.rs index e1d1791..114d1a0 100644 --- a/src/protocol_io.rs +++ b/src/protocol_io.rs @@ -18,13 +18,13 @@ const _: () = assert!( "MAX_FRAME too small for max payload + COBS overhead" ); -/// Accumulates bytes into complete COBS frames. -pub struct FrameAccumulator { +/// Decodes COBS-framed commands from a byte stream. +pub struct CobsDecoder { buf: [u8; MAX_FRAME], len: usize, } -impl FrameAccumulator { +impl CobsDecoder { pub const fn new() -> Self { Self { buf: [0u8; MAX_FRAME], @@ -67,7 +67,7 @@ impl FrameAccumulator { /// COBS-encode a response into `encode_buf` with trailing 0x00 sentinel. /// /// Returns the slice to send (encoded frame + sentinel), or `None` on overflow. -pub fn encode_response<'a>( +pub fn cobs_encode_response<'a>( response: Response, write_buf: &mut [u8; MAX_FRAME], encode_buf: &'a mut [u8; MAX_FRAME], diff --git a/src/uart/task.rs b/src/uart/task.rs index 46f23da..a911119 100644 --- a/src/uart/task.rs +++ b/src/uart/task.rs @@ -9,7 +9,7 @@ use embassy_executor::task; use embedded_io_async::Write; use crate::channel::{CommandChannel, DisplayCommandChannel, ResponseChannel}; -use crate::protocol_io::{self, FrameAccumulator, MAX_FRAME}; +use crate::protocol_io::{self, CobsDecoder, MAX_FRAME}; #[task] pub async fn uart_task( @@ -25,7 +25,7 @@ pub async fn uart_task( let mut read_buf = [0u8; 64]; let mut write_buf = [0u8; MAX_FRAME]; let mut cobs_encode_buf = [0u8; MAX_FRAME]; - let mut accumulator = FrameAccumulator::new(); + let mut decoder = CobsDecoder::new(); loop { use embassy_futures::select::select; @@ -42,14 +42,14 @@ pub async fn uart_task( Ok(0) => continue, Ok(n) => n, Err(_) => { - accumulator.reset(); + decoder.reset(); embassy_time::Timer::after_millis(100).await; continue; } }; let mut cmds = heapless::Vec::<_, 4>::new(); - accumulator.feed(&read_buf[..n], |cmd| { + decoder.feed(&read_buf[..n], |cmd| { let _ = cmds.push(cmd); }); for cmd in cmds { @@ -61,7 +61,7 @@ pub async fn uart_task( } Either::Second(response) => { if let Some(frame) = - protocol_io::encode_response(response, &mut write_buf, &mut cobs_encode_buf) + protocol_io::cobs_encode_response(response, &mut write_buf, &mut cobs_encode_buf) { if tx.write_all(frame).await.is_err() { warn!("UART write failed, response dropped"); diff --git a/src/usb/task.rs b/src/usb/task.rs index 246be4f..aeb534d 100644 --- a/src/usb/task.rs +++ b/src/usb/task.rs @@ -11,18 +11,10 @@ use embassy_usb::Builder; use static_cell::StaticCell; use crate::channel::{CommandChannel, DisplayCommand, DisplayCommandChannel, ResponseChannel}; -use crate::protocol::{Command, ErrorCode, Response}; +use crate::protocol_io::{self, CobsDecoder, MAX_FRAME}; -const MAX_FRAME: usize = 512; const MAX_PACKET: usize = 64; -// Worst case: tag(1) + rssi(2) + snr(2) + len(2) + payload(256) = 263 bytes. -// COBS adds ceil(263/254)+1 = 3 bytes. 266 < 512. Plenty of room. -const _: () = assert!( - MAX_FRAME >= crate::protocol::MAX_PAYLOAD + 64, - "MAX_FRAME too small for max payload + COBS overhead" -); - #[task] pub async fn usb_task( parts: crate::board::UsbParts, @@ -103,11 +95,7 @@ async fn protocol_loop<'d, D: embassy_usb_driver::Driver<'d>>( let mut read_buf = [0u8; MAX_PACKET]; let mut write_buf = [0u8; MAX_FRAME]; let mut cobs_encode_buf = [0u8; MAX_FRAME]; - - // COBS accumulator: collect bytes until 0x00 sentinel - let mut frame_buf = [0u8; MAX_FRAME]; - let mut frame_len: usize = 0; - + let mut decoder = CobsDecoder::new(); let mut was_connected = false; loop { @@ -123,61 +111,33 @@ async fn protocol_loop<'d, D: embassy_usb_driver::Driver<'d>>( Ok(0) => continue, Ok(n) => n, Err(_) => { - frame_len = 0; // reset accumulator + decoder.reset(); embassy_time::Timer::after_millis(100).await; continue; } }; - // Feed bytes into the COBS accumulator - for &byte in &read_buf[..n] { - if byte == 0x00 { - // End of COBS frame — decode and parse - if frame_len > 0 { - let mut decode_buf = [0u8; MAX_FRAME]; - if let Some(decoded_len) = - ucobs::decode(&frame_buf[..frame_len], &mut decode_buf) - { - if let Some(cmd) = Command::from_bytes(&decode_buf[..decoded_len]) { - route_command( - cmd, - commands, - responses, - display_commands, - has_display, - mac, - ) - .await; - } - } - } - frame_len = 0; - } else if frame_len < MAX_FRAME { - frame_buf[frame_len] = byte; - frame_len += 1; - } else { - // Frame too large — discard - frame_len = 0; - } + let mut cmds = heapless::Vec::<_, 4>::new(); + decoder.feed(&read_buf[..n], |cmd| { + let _ = cmds.push(cmd); + }); + for cmd in cmds { + protocol_io::route_command( + cmd, commands, responses, display_commands, has_display, mac, + ) + .await; } } Either3::Second(response) => { - // Serialize response to fixed-size LE bytes, then COBS encode - let raw_len = response.write_to(&mut write_buf); - let encoded_len = - ucobs::encode(&write_buf[..raw_len], &mut cobs_encode_buf).unwrap_or(0); - // Append 0x00 sentinel - if encoded_len < cobs_encode_buf.len() { - cobs_encode_buf[encoded_len] = 0x00; - let frame = &cobs_encode_buf[..encoded_len + 1]; + if let Some(frame) = + protocol_io::cobs_encode_response(response, &mut write_buf, &mut cobs_encode_buf) + { for chunk in frame.chunks(MAX_PACKET) { if sender.write_packet(chunk).await.is_err() { warn!("USB write failed, response dropped"); break; } } - } else { - warn!("COBS encode buffer overflow"); } } Either3::Third(()) => { @@ -196,43 +156,3 @@ async fn protocol_loop<'d, D: embassy_usb_driver::Driver<'d>>( was_connected = connected; } } - -/// Route a parsed command to the appropriate handler. -/// -/// Display and MAC commands are handled locally (response sent immediately). -/// All other commands are forwarded to the radio task (response sent later). -/// Both paths feed the same `ResponseChannel`, so the one-outstanding-command -/// rule in PROTOCOL.md is required to keep solicited responses ordered. -async fn route_command( - cmd: Command, - commands: &CommandChannel, - responses: &ResponseChannel, - display_commands: &DisplayCommandChannel, - has_display: bool, - mac: [u8; 6], -) { - match cmd { - Command::DisplayOn => { - if has_display { - display_commands.send(DisplayCommand::On).await; - responses.send(Response::Ok).await; - } else { - responses.send(Response::Error(ErrorCode::NoDisplay)).await; - } - } - Command::DisplayOff => { - if has_display { - display_commands.send(DisplayCommand::Off).await; - responses.send(Response::Ok).await; - } else { - responses.send(Response::Error(ErrorCode::NoDisplay)).await; - } - } - Command::GetMac => { - responses.send(Response::MacAddress(mac)).await; - } - other => { - commands.send(other).await; - } - } -}