diff --git a/ANSWERS.md b/ANSWERS.md new file mode 100644 index 00000000..8a0a60b8 --- /dev/null +++ b/ANSWERS.md @@ -0,0 +1,257 @@ +## Module Naming & Structure + + 1. The unified host communication module — do you prefer comm/, host/, or something else? + +host/ + + 2. Should protocol.rs stay as a single file, or split into protocol/types.rs + protocol/framing.rs (absorbing protocol_io.rs)? + +Split it apart logically + + 3. Or should the framing code live inside the comm/ module since it's only used there? + +You decide what makes the most sense based on our first principles + + 4. The radio/ directory currently has just task.rs (and presumably a mod.rs). Should it stay a directory or flatten to radio.rs? + +Flatten. No directory should ever have just one (or even 2?) things in it! + + 5. Same question for display/ — it has task.rs, render.rs, and the driver. Keep as directory? + +Nope. See previous question. + + 6. Should there be a board/drivers/ subdirectory, or should the SH1106 driver just stay at the board level (e.g. board/sh1106.rs)? + +Somehow, things like drivers need to be clearly separated from boards, both by +filename+location and by how the code is structured (abstractions, etc) + + 7. The channel.rs file is standalone — is that fine, or should inter-task communication be grouped differently? + +I guess fine? + + Board Definition Contract + + 8. into_parts() is currently an inherent method, not on the trait. Should it be added to LoRaBoard (requires associated types), or is the implicit contract fine? + +Nothing ever implicit. This is rust. + + 9. Should we add a spawn_host_task() function to each board, or keep that concern in the comm/ module with internal cfg? + +If it is going to vary by hardware, then yeah I think we have to push it down +into the board abstraction level. Which sucks, but at least so far seems to be +how it works. I frankly still don't believe that the Heltec V3 cannot be made to +look like a proper ttyACM0 device! I'm not giving up on that shit yet!!! + + 10. Should boards export a single BoardParts struct (with radio, comm, display fields) instead of returning a tuple? + +struct > tuple. + + 11. Should LoRaBoard gain associated constants like const HAS_DISPLAY: bool or const COMM_TYPE: CommType? + +I don't know. You can have a HTV4 with an OLED or without one. Does anything +above the board abstraction layer need to know this??? + + 12. Is the current LoRaBoard trait the right level of abstraction? Should it grow (more methods), shrink (fewer), or stay the same? + +You decide. + + 13. Should mac_address() remain on the trait, or become a field in a parts struct? + +I think it can fit nicely in the struct. If that's a pattern we're choosing, be +consistent. + + 14. The into_parts() currently consumes self (the Board). Is this the right ownership model? + +I guess? Sounds rusty. + +## Chip-Family Helpers + + 15. For nrf52840.rs — should it be a single file or a directory (nrf52840/mod.rs + submodules)? + +Maybe a directory? But how are we separating components from board +definitions??? I don't think we should just have one directory full of random +directories for MCUs, displays, UARTS, and boards all plopped in there. That +sounds **disorganized**, not **organized**. + + 16. The nRF52840 bind_interrupts! macro — should it live in the helper or per-board? (Boards might use different peripherals.) + +If you're **very** confident we'll need to customize it per-board, then go ahead +and break it out. If not, leave it generic across all boards. + + 17. The mac_address() unsafe FICR block is identical in both nRF boards. Definitely extract to helper? + +Yes. + + 18. Both ESP32-S3 boards have identical Vext GPIO36 and display reset GPIO21 logic. Should this be in esp32s3.rs or per-board? + +If you're confident this **will need to be board specific** before long, break +it out. Otherwise conslidate. + + 19. Should chip family helpers define ALL possible init functions (USB, UART, etc.) even if not all boards use them, or only what's actually used? + +Good question. I'm not sure. Best advice from you? + + 20. Should esp32s3.rs be split into submodules (esp32s3/radio.rs, esp32s3/usb.rs, esp32s3/display.rs)? + +Uhhhh.. and ESP32 is an MCU, not a radio, not a USB, not a display. GET THE +ABSTRACTIONS AND SEPARATION RIGHT!!!!!!!!!!!!!!!!!!!!! + + 21. If we add an nrf52840.rs, should the existing esp32s3.rs be renamed for consistency (both are chip-family helpers but esp32s3 is named after the chip while nrf52840 would + be named after the chip too — good)? + +Make the naming scheme consistent. + +## Display + + 22. Should the board provide an already-initialized display (DisplayParts { display: DisplayDriver, mac }) or raw parts with a separate create_display() async function? + +Board should provide the means to its consumer to init display, radio, etc. It +shouldn't preemptively init stuff. + + 23. Since into_parts() is sync but display init is async — does the create_display() approach seem right? + +I don't know. What's your advice? + + 24. Do we need an AsyncDisplay trait (for flush + brightness), or is relying on the concrete type's methods via duck-typing sufficient? + +What do you think? + + 25. Both drivers implement DrawTarget — rendering is already generic. The only non-generic part is flush() and init(). Is pushing init/flush into the board layer the right call? + +I think so. + + 26. The SH1106 driver (display/sh1106.rs) — is it a hardware driver that belongs in board/drivers/, or display infrastructure that should stay in display/? + +Think of the abstractions. Imagine you're just looking at this project for the +first time. Where would you look for a display driver??? drivers/? Something else? + + 27. The display init retry logic (try twice, give up) is identical for both drivers. Should this be a common wrapper? + +I think that retry is stupid. Let the client retry. + + 28. Display brightness: SSD1306 uses Brightness::BRIGHTEST, SH1106 uses 0xFF. Should create_display() handle this internally? + +Make it generic across displays. + + 29. Currently display errors are silently ignored (let _ = display.flush().await). Is this intentional and should remain? + +Seems like it should be elegantly handled and propagated up the abstraction layers. + +## Host Communication + + 30. USB has DTR disconnect detection that sends DisplayCommand::Reset/On. UART has no equivalent. Is it OK for UART boards to simply not have this feature? + +I mean, do we have a choice? I **really** don't want to support these UART +boards. And I'm still refusing to believe that the Heltec V3's USB hardware +cannot be made to look like a proper ttyACM. People use these MCUs and boards to +create keyboards/HID and other things after all! So my REAL preference is to NOT +support these boards. That said, we have to support the Heltec V3. So, if we +can't do it properly, then I guess we don't have a choice here. + + 31. The USB task runs join(usb_dev.run(), protocol_loop(...)) — UART just loops. Is having structurally different task bodies behind the same host_task name acceptable? + +Consistency is important to me. I don't know the right answer here. But be +consistent and smart. + + 32. Should comm/mod.rs compile ONLY the active transport, or compile both and let the linker dead-code-eliminate? + +Umm if there's a nice way to do it, then sure. But no leaky abstractions! Use +traits and abstractions and layers so we compile the right thing for a given configuration. + + 33. The USB task's COBS accumulation is inline and the UART task uses FrameAccumulator. I've verified they're semantically identical. Can I just replace the USB inline code with FrameAccumulator? + +Sure. Is "FrameAccumulator" the right name for this though? + + 34. The USB task's route_command (inline, lines 206-238) is byte-for-byte identical to protocol_io::route_command. Can I just delete the inline one? + +Yes. + + 35. MAX_FRAME is defined in both usb/task.rs (line 16) and protocol_io.rs (line 12). Should there be one canonical definition? + +Yes. + +## Build System + + 36. How should build.rs discover chip-family helpers? Options: (a) scan board files for use super::, (b) naming convention, (c) explicit list in build.rs, (d) marker comments in board files? + +I like build.rs scanning for board files. I just want them all in a logical +directory tree. + + 37. The Jinja template approach — keep it, or is there something better? + +I'm pretty happy with it. You tell me: is there something better??? + + 38. The justfile _info helper uses a hardcoded if/elif chain. Should this become data-driven (scan a file or Cargo.toml)? + +Yes. + + 39. Should the justfile board list at line 15 be auto-derived from Cargo.toml features? + +Maybe. What if it's derived from the source files it finds? + + 40. build.rs excludes mod, traits, esp32s3 from board discovery. With nrf52840.rs added, the exclusion list grows. Is a naming convention better (e.g. + all-lowercase-with-underscores = board, CamelCase or chip-family-names = helper)? + +"Exclustion lists" are evil. Fix the abstractions and design so we don't have to +deal with this shit. + +## Future-Proofing + + 41. Do ALL future boards use SX1262, or could there be SX1276/SX1280/other radios? + +There will be other radios. + + 42. Could a future board have NO display at all? (Currently all boards return Some(DisplayParts) or have optional display.) + +Yes, many boards will have no display. + + 43. Could a future board use SPI display instead of I2C? + +Probably. + + 44. Could a future board use a completely different MCU family (STM32, RP2040)? + +Yes. In fact I have both STM and RP2040 boards sitting here now. + + 45. Is there a realistic scenario where a board needs both USB AND UART? + +No. FUCK UART. + + 46. Could a future board need additional tasks beyond radio/comm/display (e.g. LED, sensor)? + +I am thinking of adding an RGB LED interface down into the boards. Then if the +host has turned on the display, we'd blink the LED green anytime we receive and +red any time we transmit a packet. + +And these boards often have lots of other hardware. Wi-Fi, BLE, sensors, +whatever. DongLoRa is really just about the radio though so I'm not prioritizing +supporting these, beyond the RGB LED thing I mentioned. + +## Migration & Testing + + 47. Should this be one atomic refactor, or phased (dedup first, then reorg, then display)? I propose phased — each step independently compilable and testable. + +Probably phased. Use jj to checkpoint as you go along. When you're happy with a +phase, "jj commit -m 'WIP: phase description'" (jj commit is the same as a jj +describe + jj new) + + 48. Are there any in-flight changes on other branches/bookmarks that would conflict? + +Nope. We just finished getting the Heltec V3 to work, which are the last few +"WIP" commits. + + 49. The protocol tests (just test) use DONGLORA_HOST_TEST=1 to skip build.rs board logic. Will the reorg affect this? + +You tell me. What I do know is testing is extremely important. I'd like +comprehensive tests, property tests, mutation testing, fuzz testing. Basically +throw everything we have at it. + + 50. After the reorg, do you want a CI-enforceable rule like "no #[cfg(feature = \"board_name\")] outside board/ and comm/mod.rs"? + +That sounds great. Any mechanisms to enforce our abstractions and first +principles are very welcome. + + +BTW, NO unsafe code unless it is ABSOLUTELY REQUIRED. I know we're doing +embedded stuff. There are probably a few unsafe things we have to do. But always +research for ways to eliminate unsafe code. diff --git a/firmware/src/main.rs b/firmware/src/main.rs index c9d9f47a..68028861 100644 --- a/firmware/src/main.rs +++ b/firmware/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/firmware/src/protocol_io.rs b/firmware/src/protocol_io.rs index e1d17915..114d1a09 100644 --- a/firmware/src/protocol_io.rs +++ b/firmware/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/firmware/src/uart/task.rs b/firmware/src/uart/task.rs index 46f23daa..a9111199 100644 --- a/firmware/src/uart/task.rs +++ b/firmware/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/firmware/src/usb/task.rs b/firmware/src/usb/task.rs index 246be4f5..aeb534d9 100644 --- a/firmware/src/usb/task.rs +++ b/firmware/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; - } - } -}