Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @p14c31355, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily addresses a bug causing garbled characters by adjusting a buffer size. Additionally, it includes a significant refactoring of the command node and explorer structures, transitioning to const generics for improved memory efficiency and type safety in handling command data and their dependencies. These changes enhance the robustness and clarity of the system's command processing capabilities.
Highlights
- Garbled Character Fix: The buffer size in the
prevent_garbledfunction withinsrc/compat/util.rswas reduced from 512 to 256 bytes, directly addressing the reported issue of garbled characters. - Refactoring of CmdNode Structure: The
CmdNodestruct insrc/explore/explorer.rswas refactored to use fixed-size arrays ([u8; N],[u8; D]) forbytesanddeps, along with newbytes_lenanddeps_lenfields. This change improves memory management and type safety by allowing command data and dependencies to be stored in compile-time sized buffers. - Enhanced Explorer Generics: The
Explorerstruct and its associated functions and macros acrosssrc/explore/explorer.rsandsrc/explore/runner.rswere updated to leverage newconst genericsparameters (NODE_COUNT,BYTES_MAX,DEPS_MAX). This enables more precise type definitions and compile-time size guarantees for the command nodes and their data. - New Utility Function for Node Dependencies: A new helper function
write_node_depswas added tosrc/compat/util.rsto facilitate formatted printing of node dependencies, which is now utilized within theExplorer'sget_one_sortmethod for debugging or logging purposes.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with garbled characters and also introduces a significant refactoring of the Explorer and related components. The reduction of the buffer size in prevent_garbled is a good step to prevent potential stack overflows on memory-constrained devices like the ATmega328p. The move to const generics and stack-based arrays from &'static slices is a great improvement for an embedded context.
However, the refactoring has introduced some critical issues where a generic parameter for maximum byte length (N) has been used instead of the parameter for the number of nodes (NODE_COUNT) when sizing arrays. This will likely cause panics due to out-of-bounds memory access. I've provided detailed comments and suggestions to fix these critical bugs.
I've also included a couple of medium-severity comments regarding inconsistent error handling in a new utility function and a misleading code comment.
src/compat/util.rs
Outdated
| pub fn write_node_deps<W: core::fmt::Write>( | ||
| w: &mut W, | ||
| index: usize, | ||
| deps: &[u8], | ||
| ) -> core::fmt::Result | ||
| where | ||
| W: core::fmt::Write, | ||
| { | ||
| write!(w, "node ").ok(); | ||
| if index < 256 { | ||
| write!(w, "{:02X}", index as u8)?; | ||
| } else { | ||
| write!(w, "?"); // overflow fallback | ||
| } | ||
| write!(w, ": deps=").ok(); | ||
| write_bytes_hex_fmt(w, deps)?; | ||
| writeln!(w).ok(); // Add a newline since write_bytes_hex_fmt doesn't include one | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This new function has inconsistent error handling and a redundant where clause.
- Inconsistent Error Handling: Some
write!andwriteln!calls use.ok()to ignore errors, while others use?to propagate them. Since the function returns acore::fmt::Result, it's best practice to propagate all errors using?and let the caller decide whether to handle or ignore them. This makes the function more robust and predictable. - Redundant
whereClause: Thewhere W: core::fmt::Writeon line 158 is redundant, as this bound is already specified on line 152.
I've provided a suggestion that addresses both points for cleaner and more idiomatic Rust code.
pub fn write_node_deps<W: core::fmt::Write>(
w: &mut W,
index: usize,
deps: &[u8],
) -> core::fmt::Result {
write!(w, "node ")?;
if index < 256 {
write!(w, "{:02X}", index as u8)?;
} else {
write!(w, "?")?; // overflow fallback
}
write!(w, ": deps=")?;
write_bytes_hex_fmt(w, deps)?;
writeln!(w)?; // Add a newline since write_bytes_hex_fmt doesn't include one
Ok(())
}
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@gemini-code-assist |
|
@p14c31355 Thanks for reaching out! I understand you're encountering SRAM limit issues on your Arduino Uno. This is a common challenge in embedded development, especially with limited resources like the ATmega328P's 2KB SRAM. The recent changes in this Pull Request, specifically how Previously, In this PR, For example, if you have 100 To address this, I recommend reverting the Here are the suggested changes to revert the memory-intensive parts: 1. --- a/src/explore/explorer.rs
+++ b/src/explore/explorer.rs
@@ -7,10 +7,11 @@ use crate::error::{ExecutorError, ExplorerError};
use crate::scanner::{I2C_SCAN_ADDR_END, I2C_SCAN_ADDR_START};
const I2C_ADDRESS_COUNT: usize = 128;
-pub struct CmdNode<const N: usize, const D: usize> {
- pub bytes: [u8; N],
- pub bytes_len: u8,
- pub deps: [u8; D],
- pub deps_len: u8,
+#[derive(Copy, Clone)]
+pub struct CmdNode {
+ pub bytes: &'static [u8],
+ pub deps: &'static [u8],
}
pub trait CmdExecutor<I2C, const CMD_BUFFER_SIZE: usize> {
@@ -214,50 +215,60 @@ where
macro_rules! nodes {
(
prefix = $prefix:expr,
- bytes_max = $bytes_max:expr,
- deps_max = $deps_max:expr,
+ // Removed bytes_max and deps_max parameters
[
$( [ $( $b:expr ),* ] $( @ [ $( $d:expr ),* ] )? ),* $(,)?
]
) => {{
- const NODE_COUNT: usize = $crate::count_exprs!($( [ $( $b ),* ] ),*);
-
- const NODES: [$crate::explore::explorer::CmdNode<$bytes_max, $deps_max>; NODE_COUNT] = [
+ static NODES: &[$crate::explore::explorer::CmdNode] = &[
$(
$crate::explore::explorer::CmdNode {
- bytes: {
- let mut arr = [0u8; $bytes_max];
- let mut i = 0;
- $(
- arr[i] = $b;
- i += 1;
- )*
- arr
- },
- bytes_len: $crate::count_exprs!($($b),*) as u8,
- deps: {
- let mut arr = [0u8; $deps_max];
- let mut i = 0;
- $(
- $(
- arr[i] = $d;
- i += 1;
- )*
- )?
- arr
- },
- deps_len: $crate::count_exprs!($($($d),*)?) as u8,
+ bytes: &[ $( $b ),* ],
+ deps: &[ $( $( $d ),* )? ],
}
),*
];
- const EXPLORER: $crate::explore::explorer::Explorer<NODE_COUNT, $bytes_max, $deps_max> = $crate::explore::explorer::Explorer {
- nodes: &NODES,
+ static EXPLORER: $crate::explore::explorer::Explorer<{NODES.len()}> = $crate::explore::explorer::Explorer {
+ nodes: NODES,
};
EXPLORER
}};
}
/// simple macro to count comma-separated expressions at compile time
#[macro_export]
macro_rules! count_exprs {
() => (0usize);
($x:expr $(, $xs:expr)*) => (1usize + $crate::count_exprs!($($xs),*));
}
-pub struct Explorer<'a, const NODE_COUNT: usize, const N: usize, const D: usize> {
- pub nodes: &'a [CmdNode<N, D>; NODE_COUNT],
+pub struct Explorer<const N: usize> {
+ pub(crate) nodes: &'static [CmdNode],
}
pub struct ExploreResult {
@@ -266,7 +277,7 @@ pub struct ExploreResult {
pub permutations_tested: usize,
}
-impl<'a, const NODE_COUNT: usize, const N: usize, const D: usize> Explorer<'a, NODE_COUNT, N, D> {
+impl<const N: usize> Explorer<N> {
// This function calculates the max length of a single command's byte array
pub const fn max_cmd_len(&self) -> usize {
let mut max_len = 0;
@@ -281,7 +292,7 @@ impl<const N: usize> Explorer<N> {
max_len
}
- pub const fn new(nodes: &'a [CmdNode<N, D>; NODE_COUNT]) -> Self {
+ pub const fn new(nodes: &'static [CmdNode]) -> Self {
Self { nodes }
}
@@ -368,20 +379,20 @@ impl<const N: usize> Explorer<N> {
}
pub fn get_one_sort(
- &'a self,
+ &self,
_writer: &mut impl core::fmt::Write,
- failed_nodes: &[bool; NODE_COUNT], // This is fine, it's a reference to a fixed-size array
- ) -> Result<(heapless::Vec<&'a [u8], NODE_COUNT>, heapless::Vec<u8, NODE_COUNT>), ExplorerError> {
+ failed_nodes: &[bool; N],
+ ) -> Result<(heapless::Vec<&'static [u8], N>, heapless::Vec<u8, N>), ExplorerError> {
let len = self.nodes.len();
- let mut in_degree: [u8; NODE_COUNT] = [0; NODE_COUNT];
- let mut adj_list_rev: [u128; NODE_COUNT] = [0; NODE_COUNT];
+ let mut in_degree: [u8; N] = [0; N];
+ let mut adj_list_rev: [u128; N] = [0; N];
for (i, node) in self.nodes.iter().enumerate().take(len) {
- util::write_node_deps(_writer, i, &node.deps[..node.deps_len as usize]).ok();
+ util::write_node_deps(_writer, i, node.deps).ok();
}
// Ensure NODE_COUNT is large enough for the sequence
- if len > NODE_COUNT {
+ if len > N {
return Err(ExplorerError::TooManyCommands);
}
@@ -390,25 +401,25 @@ impl<const N: usize> Explorer<N> {
if failed_nodes[i] {
continue;
}
- for &dep_idx in node.deps.iter().take(node.deps_len as usize) {
+ for &dep_idx in node.deps.iter() {
let dep_idx_usize = dep_idx as usize;
if dep_idx_usize >= len {
return Err(ExplorerError::InvalidDependencyIndex);
}
- in_degree[dep_idx_usize] += 1;
+ in_degree[dep_idx_usize] += 1;
// Use a bitmask (u128) to represent the adjacency list.
// This replaces the heapless::Vec<heapless::Vec<u8, N>, N> from the original.
adj_list_rev[dep_idx_usize] |= 1u128 << (i as u128);
}
}
- let mut result_sequence: heapless::Vec<&[u8], NODE_COUNT> = heapless::Vec::new();
- let mut result_len_per_node: heapless::Vec<u8, NODE_COUNT> = heapless::Vec::new();
+ let mut result_sequence: heapless::Vec<&'static [u8], N> = heapless::Vec::new();
+ let mut result_len_per_node: heapless::Vec<u8, N> = heapless::Vec::new();
let mut visited_count = 0;
// Use a fixed-size array as a queue to avoid heap allocation.
// `q_head` and `q_tail` manage the queue's state.
- let mut q: [u8; NODE_COUNT] = [0; NODE_COUNT];
+ let mut q: [u8; N] = [0; N];
let mut q_head: usize = 0;
let mut q_tail: usize = 0;
@@ -430,9 +441,9 @@ impl<const N: usize> Explorer<N> {
let u = u_u8 as usize;
visited_count += 1;
- let cmd_bytes = &self.nodes[u].bytes[..self.nodes[u].bytes_len as usize];
- result_len_per_node // This is fine, it's a copy of the length
- .push(self.nodes[u].bytes_len)
+ let cmd_bytes = self.nodes[u].bytes;
+ result_len_per_node
+ .push(cmd_bytes.len() as u8)
.map_err(|_| ExplorerError::BufferOverflow)?;
result_sequence
.push(cmd_bytes)
@@ -475,8 +486,9 @@ impl<const N: usize> Explorer<N> {
}
}
-pub struct PermutationIter<'a, const NODE_COUNT: usize, const N: usize, const D: usize> {
- // Added NODE_COUNT
- pub explorer: &'a Explorer<'a, NODE_COUNT, N, D>,
+pub struct PermutationIter<'a, const N: usize> {
+ // Reverted to original N (NODE_COUNT)
+ pub explorer: &'a Explorer<N>,
pub total_nodes: usize,
pub current_permutation: [&'a [u8]; N],
pub current_permutation_len: u8,
@@ -488,8 +500,10 @@ pub struct PermutationIter<'a, const N: usize> {
pub is_done: bool,
}
-impl<'a, const NODE_COUNT: usize, const N: usize, const D: usize>
- PermutationIter<'a, NODE_COUNT, N, D>
-{
- pub fn new(explorer: &'a Explorer<'a, NODE_COUNT, N, D>) -> Result<Self, ExplorerError> {
+impl<'a, const N: usize> PermutationIter<'a, N> {
+ pub fn new(explorer: &'a Explorer<N>) -> Result<Self, ExplorerError> {
const {
assert!
N <= 128,
@@ -605,7 +619,7 @@ impl<'a, const N: usize> PermutationIter<'a, N> {
self.used.set(i).unwrap();
if self.current_permutation_len < N as u8 {
self.current_permutation[self.current_permutation_len as usize] =
- &self.explorer.nodes[i].bytes[..self.explorer.nodes[i].bytes_len as usize];
+ self.explorer.nodes[i].bytes;
self.current_permutation_len += 1;
} else {
self.is_done = true;
@@ -656,7 +670,9 @@ impl<'a, const N: usize> PermutationIter<'a, N> {
}
}
-impl<'a, const NODE_COUNT: usize, const N: usize, const D: usize> Iterator
- for PermutationIter<'a, NODE_COUNT, N, D>
-{
+impl<'a, const N: usize> Iterator for PermutationIter<'a, N> {
type Item = [&'a [u8]; N];
fn next(&mut self) -> Option<Self::Item> {2. --- a/src/explore/runner.rs
+++ b/src/explore/runner.rs
@@ -7,8 +7,8 @@ use crate::scanner::I2C_MAX_DEVICES;
#[macro_export]
macro_rules! factorial_sort {
- ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr, $n:expr, $d:expr, $init_len:expr, $cmd_buf:expr) => {
- $crate::explore::runner::factorial_explorer::<_, _, $n, $d, $init_len, $cmd_buf>(
+ ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr, $n:expr, $init_len:expr, $cmd_buf:expr) => {
+ $crate::explore::runner::factorial_explorer::<_, _, $n, $init_len, $cmd_buf>(
$explorer,
$i2c,
$serial,
@@ -21,11 +21,13 @@ pub fn factorial_explorer<
I2C,
S,
- const NODE_COUNT: usize,
- const BYTES_MAX: usize,
- const DEPS_MAX: usize,
+ const N: usize,
const INIT_SEQUENCE_LEN: usize,
const CMD_BUFFER_SIZE: usize,
>(
- explorer: &Explorer<NODE_COUNT, BYTES_MAX, DEPS_MAX>,
+ explorer: &Explorer<N>,
i2c: &mut I2C,
serial: &mut S,
prefix: u8,
@@ -96,25 +98,29 @@ where
#[macro_export]
macro_rules! pruning_sort {
- ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr, $node_count:expr, $bytes_max:expr, $deps_max:expr, $init_len:expr, $cmd_buf:expr) => {
- $crate::explore::runner::pruning_explorer::<
+ ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr, $n:expr, $init_len:expr, $cmd_buf:expr) => {
+ $crate::explore::runner::pruning_explorer::<
_,
_,
- $node_count,
- $bytes_max,
- $deps_max,
+ $n,
$init_len,
$cmd_buf,
>($explorer, $i2c, $serial, $prefix, $init_sequence)
};
}
pub fn pruning_explorer<
I2C,
S,
- const NODE_COUNT: usize,
- const BYTES_MAX: usize,
- const DEPS_MAX: usize,
+ const N: usize,
const INIT_SEQUENCE_LEN: usize,
const CMD_BUFFER_SIZE: usize,
>(
- explorer: &Explorer<NODE_COUNT, BYTES_MAX, DEPS_MAX>,
+ explorer: &Explorer<N>,
i2c: &mut I2C,
serial: &mut S,
prefix: u8,
@@ -163,7 +169,7 @@ where
&successful_seq,
);
- let mut failed_nodes = [false; NODE_COUNT];
+ let mut failed_nodes = [false; N];
loop {
let (sequence_bytes, _sequence_len) = match explorer.get_one_sort(serial, &failed_nodes) {
@@ -230,21 +236,29 @@ where
#[macro_export]
macro_rules! get_one_sort {
- ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $n:expr, $bytes_max:expr, $d:expr, $init_len:expr, $cmd_buf:expr) => {
- $crate::explore::runner::one_topological_explorer::<
+ ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $n:expr, $init_len:expr, $cmd_buf:expr) => {
+ $crate::explore::runner::one_topological_explorer::<
_,
_,
- $n,
- $bytes_max,
- $d,
+ $n,
$init_len,
$cmd_buf,
>($explorer, $i2c, $serial, $prefix)
};
}
pub fn one_topological_explorer<
I2C,
S,
- const NODE_COUNT: usize,
- const BYTES_MAX: usize,
- const DEPS_MAX: usize,
+ const N: usize,
const INIT_SEQUENCE_LEN: usize,
const CMD_BUFFER_SIZE: usize,
>(
- explorer: &Explorer<NODE_COUNT, BYTES_MAX, DEPS_MAX>,
+ explorer: &Explorer<N>,
i2c: &mut I2C,
serial: &mut S,
prefix: u8,
@@ -270,7 +284,7 @@ where
return Err(ExplorerError::NoValidAddressesFound);
}
- let single_sequence = explorer.get_one_sort(serial, &[false; NODE_COUNT])?;
+ let single_sequence = explorer.get_one_sort(serial, &[false; N])?;
let sequence_len = explorer.nodes.len();
These changes should significantly reduce your SRAM footprint by leveraging flash memory for static data, which is crucial for resource-constrained microcontrollers like the Arduino Uno. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with garbled characters by reducing a buffer size, which is a sensible fix for memory-constrained environments like the ATmega328p. Additionally, a new helper function write_node_deps is introduced. My review includes a suggestion to refactor this new function to make it more concise and idiomatic.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Root Cause of SRAM ConsumptionThe
These data structures expand the information for the entire graph ( Refactoring Strategy1. Optimizing Graph RepresentationBitmasks like
2. On-Demand SortingRather than attempting to sort all nodes at once, this approach dynamically determines the next command at runtime. This is similar to backtracking or recursive approaches, but minimizes state.
3. Using Iterators and Reducing StateBecause the result of
4. Integrating Execution and Sorting
These refactorings may increase code complexity, but they provide a more robust and efficient solution for memory-constrained embedded systems. I'd like to implement refactorings 2-4. Please output the code block. |
|
Hello @p14c31355, Thank you for your detailed analysis of the SRAM consumption in Your proposed refactoring strategies (On-Demand Sorting, Using Iterators, and Integrating Execution and Sorting) are excellent and represent the best practices for optimizing memory usage in such environments. While you mentioned "Optimizing Graph Representation" (Refactoring 1) with I've implemented the refactorings for "On-Demand Sorting" (2), "Using Iterators" (3), and "Integrating Execution and Sorting" (4) as you requested. This involves introducing a new Here are the updated code blocks for
|
|
@gemini-code-assist |
|
Hello @p14c31355, My apologies for the oversight in the previous refactoring. You are absolutely correct that some original functionality, particularly related to error handling and debugging output, was inadvertently altered or removed. Thank you for pointing this out! I've re-evaluated the changes and have updated the code to ensure that the explicit Here are the updated code blocks for
|
|
@gemini-code-assist |
Summary of ChangesThis pull request primarily focuses on resolving a "garbled characters" issue, which led to a broader effort to optimize memory consumption within the application's topological sorting logic. The core changes involve refactoring the command exploration and execution mechanisms to use an on-demand, iterator-based approach, significantly reducing SRAM usage critical for resource-constrained embedded environments. Additionally, it includes the introduction of a new utility function and ensures the integrity of existing error handling and debugging features. Highlights
Changelog
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the topological sort logic into a memory-efficient iterator, which is a great improvement for the target embedded platform. The fix for garbled characters by reducing a buffer size also seems appropriate for memory-constrained devices. I've found a critical bug in the pruning_explorer where an incorrect prefix was being used for I2C commands. Additionally, there are a few areas for improvement regarding performance and robustness, such as an inefficient queue operation and a missing compile-time check that could lead to a panic. I've also pointed out some unused code that can be cleaned up.
src/explore/explorer.rs
Outdated
| } | ||
|
|
||
| impl<'a, const N: usize> TopologicalIter<'a, N> { | ||
| pub fn new(explorer: &'a Explorer<N>, failed_nodes: &[bool; N]) -> Result<Self, ExplorerError> { |
There was a problem hiding this comment.
The implementation of TopologicalIter uses a u128 as a bitmask for the reverse adjacency list (adj_list_rev). This implies that the number of nodes, N, cannot exceed 128. If N is greater than 128, the expression 1 << i in the loop at line 60 will panic when i >= 128. To prevent this at compile time, you should add a check similar to the one in PermutationIter.
const {
assert!(
N <= 128,
"TopologicalIter uses a u128 bitmask, so N cannot exceed 128"
);
};
pub fn new(explorer: &'a Explorer<N>, failed_nodes: &[bool; N]) -> Result<Self, ExplorerError> {
src/explore/explorer.rs
Outdated
| return None; | ||
| } | ||
|
|
||
| let u_u8 = self.queue.remove(0); |
There was a problem hiding this comment.
Using remove(0) on a Vec is an O(N) operation because it requires shifting all subsequent elements. This can be inefficient, especially on embedded systems. Using pop() is an O(1) operation. This will change the processing order from FIFO (queue) to LIFO (stack), but it will still produce a valid topological sort and be much more performant.
let u_u8 = self.queue.pop().unwrap();
src/explore/runner.rs
Outdated
| util::prevent_garbled(serial, format_args!("Sending commands to {addr:02X}")); | ||
|
|
||
| let mut all_ok = true; | ||
| let mut completed_node_count = 0; |
There was a problem hiding this comment.
The variable completed_node_count is initialized here but its value is never used. It should be removed.
src/explore/runner.rs
Outdated
| failed_nodes[cmd_idx] = true; | ||
| break; | ||
| } | ||
| completed_node_count += 1; |
There was a problem hiding this comment.
The variable completed_node_count is incremented here but its value is never used. This line should be removed along with the variable's declaration.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring to fix an issue with garbled characters and to improve the I2C device exploration logic. The introduction of AsciiSafeWriter is a great improvement for serial output. The move from a permutation-based explorer to a topological sort iterator is a solid architectural change that simplifies the code. However, the refactoring has introduced a few issues, including a performance regression in serial writing, a behavioral change in the I2C scanner, and a couple of logic errors. I've provided specific comments and suggestions to address these points.
src/explore/explorer.rs
Outdated
| core::fmt::Write::write_str(writer, "[Info] Device found at ").ok(); | ||
| crate::compat::util::write_bytes_hex_fmt(writer, &[addr]).ok(); | ||
| core::fmt::Write::write_str(writer, ", sending init sequence...\r\n").ok(); | ||
| let _ = self.initialized_addrs.set(addr_idx); |
There was a problem hiding this comment.
This call to self.initialized_addrs.set(addr_idx) is redundant. The same call is made and its result is correctly handled on line 323. This one ignores the Result and should be removed to avoid confusion and potential silent failures.
| impl<'a, W: core::fmt::Write> core::fmt::Write for AsciiSafeWriter<'a, W> { | ||
| fn write_str(&mut self, s: &str) -> core::fmt::Result { | ||
| for c in s.chars() { | ||
| if c.is_ascii() { | ||
| write!(self.0, "{c}")?; | ||
| } else { | ||
| write!(self.0, "\\u{{{:X}}}", c as u32)?; | ||
| } | ||
| } | ||
| start = end; | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new AsciiSafeWriter is a great addition for handling non-ASCII characters. However, it seems to be implemented in a way that could be more efficient. By iterating over s.chars() and then using write! for each character, you are potentially doing more work than necessary, especially for strings that are mostly or entirely ASCII. A more performant approach would be to find contiguous chunks of ASCII characters and write them in a single write_str call, only falling back to character-by-character processing for non-ASCII parts.
impl<'a, W: core::fmt::Write> core::fmt::Write for AsciiSafeWriter<'a, W> {
fn write_str(&mut self, s: &str) -> core::fmt::Result {
let mut last = 0;
for (idx, c) in s.char_indices() {
if !c.is_ascii() {
if last < idx {
self.0.write_str(&s[last..idx])?;
}
write!(self.0, "\\u{{{:X}}}", c as u32)?;
last = idx + c.len_utf8();
}
}
if last < s.len() {
self.0.write_str(&s[last..])?;
}
Ok(())
}
}
src/explore/runner.rs
Outdated
| let mut executor = | ||
| crate::explore::explorer::PrefixExecutor::<INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE>::new( | ||
| target_addrs[0], | ||
| &successful_seq, | ||
| ); | ||
|
|
||
| let mut failed_nodes = [false; N]; | ||
| PrefixExecutor::<INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE>::new(prefix, init_sequence); |
There was a problem hiding this comment.
The PrefixExecutor is initialized with the full init_sequence. However, the logic inside PrefixExecutor::exec will then send this sequence to every device upon first contact. This is likely not the intended behavior. The init_sequence parameter for pruning_explorer is probably meant for device-specific initialization, but it's being applied universally. This could be simplified by initializing PrefixExecutor with an empty sequence and handling initialization more explicitly if needed.
| let mut executor = | |
| crate::explore::explorer::PrefixExecutor::<INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE>::new( | |
| target_addrs[0], | |
| &successful_seq, | |
| ); | |
| let mut failed_nodes = [false; N]; | |
| PrefixExecutor::<INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE>::new(prefix, init_sequence); | |
| let mut executor = | |
| PrefixExecutor::<INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE>::new(prefix, &[]); |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an impressive and extensive refactoring that goes well beyond a simple bugfix. You've not only addressed the garbled character issue with a robust AsciiSafeWriter but also completely overhauled the error handling, I2C abstraction, and the core exploration logic. The move from a permutation-based explorer to a topological sort-based pruning algorithm is a fantastic improvement for both performance and maintainability. The code is much cleaner and more idiomatic. I've found a critical issue related to cycle detection that could lead to an infinite loop, and a minor issue regarding Rust API guidelines for error types. Overall, excellent work!
| use core::fmt; | ||
|
|
||
| /// Defines the category of an error. | ||
| #[derive(Clone, Copy, PartialEq, Eq)] |
There was a problem hiding this comment.
It seems Debug was intentionally removed from ErrorKind and other error enums. While the new Display implementations are great for user-facing messages, Debug is essential for developer diagnostics (e.g., with unwrap(), expect(), or {:?} formatting). It's a strong convention in Rust for all error types to implement Debug. Please consider adding #[derive(Debug)] back to ErrorKind and the other error enums in this file (UartError, I2cError, etc.), as well as to ExplorerError and ExecutorError.
| #[derive(Clone, Copy, PartialEq, Eq)] | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring to fix an issue with garbled character output and to improve the I2C command exploration logic. The fix for garbled characters using an AsciiSafeWriter is well-implemented. The exploration logic is significantly improved by replacing a brute-force permutation approach with an efficient topological sort iterator, which is a great enhancement for no_std environments. However, the refactoring has introduced a few issues: several error types are missing Debug derives, which impacts developer experience; a From implementation for errors is misleading; the logic in pruning_explorer seems to be incomplete or incorrect; and a performance regression was introduced in the adapt_serial! macro. I've provided specific comments and suggestions to address these points.
| impl From<ErrorKind> for ExplorerError { | ||
| fn from(error: ErrorKind) -> Self { | ||
| ExplorerError::DeviceNotFound(error) | ||
| } |
There was a problem hiding this comment.
This From<ErrorKind> implementation for ExplorerError converts any ErrorKind into ExplorerError::DeviceNotFound. This can be misleading, as the underlying error might not be related to a device not being found (e.g., it could be a BufferError::Overflow). This loss of semantic meaning can make debugging difficult. Consider creating a more generic error variant in ExplorerError for wrapping ErrorKind, such as ExplorerError::HalError(ErrorKind).
src/explore/runner.rs
Outdated
| let mut failed_nodes = util::BitFlags::new(); | ||
| loop { | ||
| if target_addrs.is_empty() { | ||
| write!(serial, "[I] All valid addresses explored. Done.\r\n").ok(); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let successful_seq_len = successful_seq.len(); | ||
| let mut addrs_to_remove = heapless::Vec::<usize, { I2C_MAX_DEVICES }>::new(); | ||
|
|
||
| util::prevent_garbled( | ||
| serial, | ||
| format_args!("[scan] initial sequence scan completed"), | ||
| ); | ||
| for (addr_idx, &addr) in target_addrs.iter().enumerate() { | ||
| core::fmt::Write::write_str(serial, "[I] RUN ON ").ok(); | ||
| crate::compat::util::write_bytes_hex_fmt(serial, &[addr]).ok(); | ||
| core::fmt::Write::write_str(serial, "\r\n").ok(); | ||
|
|
||
| let mut sort_iter = match explorer.topological_iter(&failed_nodes) { | ||
| Ok(iter) => iter, | ||
| Err(e) => { | ||
| write!(serial, "[E] Failed GEN topological sort: {e}\r\n").ok(); | ||
| addrs_to_remove.push(addr_idx).ok(); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| let mut executor = | ||
| crate::explore::explorer::PrefixExecutor::<INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE>::new( | ||
| target_addrs[0], | ||
| &successful_seq, | ||
| ); | ||
| let command_to_fail = None; | ||
|
|
||
| let mut failed_nodes = [false; N]; | ||
| let mut batched: heapless::Vec<u8, { CMD_BUFFER_SIZE }> = heapless::Vec::new(); | ||
| batched | ||
| .push(prefix) | ||
| .map_err(|_| ExplorerError::BufferOverflow)?; | ||
|
|
||
| loop { | ||
| let (sequence_bytes, _sequence_len) = match explorer.get_one_sort(serial, &failed_nodes) { | ||
| Ok(seq) => seq, | ||
| Err(ExplorerError::DependencyCycle) => { | ||
| util::prevent_garbled( | ||
| serial, | ||
| format_args!("[error] Dependency cycle detected, stopping exploration"), | ||
| ); | ||
| break; | ||
| } | ||
| Err(e) => { | ||
| util::prevent_garbled( | ||
| serial, | ||
| format_args!("[error] Failed to generate topological sort: {e}. Aborting."), | ||
| ); | ||
| return Err(e); | ||
| for cmd_idx in sort_iter.by_ref() { | ||
| let cmd_bytes = explorer.nodes[cmd_idx].bytes; | ||
| if batched.len() + cmd_bytes.len() > CMD_BUFFER_SIZE { | ||
| write!( | ||
| serial, | ||
| "[E] Batch buffer overflow (need {} bytes)\r\n", | ||
| batched.len() + cmd_bytes.len() | ||
| ) | ||
| .ok(); | ||
| return Err(ExplorerError::BufferOverflow); | ||
| } | ||
| batched | ||
| .extend_from_slice(cmd_bytes) | ||
| .map_err(|_| ExplorerError::BufferOverflow)?; | ||
| } | ||
| }; | ||
|
|
||
| let mut addrs_to_remove: heapless::Vec<usize, I2C_MAX_DEVICES> = heapless::Vec::new(); | ||
|
|
||
| for (addr_idx, &addr) in target_addrs.iter().enumerate() { | ||
| util::prevent_garbled(serial, format_args!("Sending commands to {addr:02X}")); | ||
| if sort_iter.is_cycle_detected() { | ||
| write!(serial, "[E] Dependency cycle detected. Aborting.\r\n").ok(); | ||
| return Err(ExplorerError::DependencyCycle); | ||
| } | ||
|
|
||
| let mut all_ok = true; | ||
| writeln!(serial, "I2C WRITE @ {addr:02X}:").ok(); | ||
| for b in batched.iter() { | ||
| write!(serial, "{b:02X} ").ok(); | ||
| } | ||
| writeln!(serial).ok(); | ||
|
|
||
| for i in 0..explorer.nodes.len() { | ||
| if failed_nodes[i] { | ||
| continue; | ||
| match i2c.write(addr, &batched) { | ||
| Ok(_) => { | ||
| write!(serial, "[E] OK batched ({} bytes)\r\n", batched.len()).ok(); | ||
| } | ||
| let cmd_bytes = &sequence_bytes[i]; | ||
|
|
||
| if exec_log_cmd(i2c, &mut executor, serial, addr, cmd_bytes, i).is_err() { | ||
| util::prevent_garbled( | ||
| Err(e) => { | ||
| write!( | ||
| serial, | ||
| format_args!("[warn] Command {i} failed on {addr:02X}"), | ||
| ); | ||
| all_ok = false; | ||
| if i >= successful_seq_len { | ||
| failed_nodes[i] = true; | ||
| } | ||
| break; | ||
| "[E] FAIL batched: {}\r\n", | ||
| crate::compat::HalErrorExt::to_compat(&e, Some(addr)) | ||
| ) | ||
| .ok(); | ||
| } | ||
| } | ||
|
|
||
| if all_ok { | ||
| let is_cycle_detected = sort_iter.is_cycle_detected(); | ||
|
|
||
| if is_cycle_detected { | ||
| write!(serial, "[E] Dependency cycle detected. Aborting.\r\n").ok(); | ||
| return Err(ExplorerError::DependencyCycle); | ||
| } | ||
|
|
||
| if let Some(cmd_idx) = command_to_fail { | ||
| failed_nodes.set(cmd_idx).unwrap(); | ||
| } else { | ||
| addrs_to_remove.push(addr_idx).ok(); | ||
| } | ||
| } | ||
|
|
||
| for &idx in addrs_to_remove.iter().rev() { | ||
| target_addrs.swap_remove(idx); | ||
| } | ||
|
|
||
| if target_addrs.is_empty() || failed_nodes.iter().all(|&x| x) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The pruning logic in pruning_explorer seems to be missing in this refactoring. The command_to_fail variable is always None, which means the else branch is always taken, removing the I2C address from the target list after a single attempt, regardless of whether the batched write succeeded or failed. This means no actual pruning of failed command nodes occurs. The function's name is now misleading, and the intended logic of retrying with a pruned command set seems to be lost.
src/compat/util.rs
Outdated
| pub const fn calculate_cmd_buffer_size(num_commands: usize, max_cmd_len: usize) -> usize { | ||
| num_commands * (max_cmd_len + 1) + num_commands * 2 | ||
| } |
There was a problem hiding this comment.
This function calculate_cmd_buffer_size is marked as deprecated by being moved to the bottom of the file and appears to be unused in the new implementation. It's good practice to remove dead code to improve maintainability.
| fn write(&mut self, buf: &[u8]) -> Result<usize, Self::Error> { | ||
| self.0.write(buf)?; | ||
| for &b in buf { | ||
| self.0.write(&[b])?; | ||
| } | ||
| Ok(buf.len()) |
There was a problem hiding this comment.
The write implementation for the adapt_serial! wrapper now writes byte-by-byte in a loop. The underlying SerialCompat::write trait accepts a slice &[u8], so writing the entire buffer at once is more efficient. This change introduces a performance regression.
self.0.write(buf)?;
Ok(buf.len())| fn write_str(&mut self, s: &str) -> core::fmt::Result { | ||
| self.0.write(s.as_bytes()).map_err(|_| core::fmt::Error)?; | ||
| for &b in s.as_bytes() { | ||
| self.0.write(&[b]).map_err(|_| core::fmt::Error)?; | ||
| } | ||
| self.0.flush().map_err(|_| core::fmt::Error)?; | ||
| Ok(()) |
There was a problem hiding this comment.
Similar to the write implementation, write_str now writes byte-by-byte, which is inefficient. It's better to write the entire string's byte slice at once and then flush.
self.0.write(s.as_bytes()).map_err(|_| core::fmt::Error)?;
self.0.flush().map_err(|_| core::fmt::Error)?;
Ok(())Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring. It replaces the complex and inefficient permutation-based command exploration with a much cleaner and faster topological sort approach. The fix for garbled serial output using an AsciiSafeWriter is robust, and the error handling and logging infrastructure have been greatly improved. I've identified a few issues, including a critical one related to a recursive error type and a high-severity issue with the new pruning logic, that should be addressed. Overall, this is excellent work.
| BitFlagsError(BitFlagsError), | ||
| BitFlags(BitFlagsError), | ||
| /// An error occurred in the explorer module. | ||
| Explorer(ExplorerError), |
There was a problem hiding this comment.
The ExecutorError enum contains an ExplorerError variant by value. This creates a recursive type definition, as ExplorerError can be created from an ExecutorError via the From trait implementation. This will cause a compile error because the type has an infinite size. To fix this, you should use a pointer type like Box<ExplorerError>. Since this is a no_std crate, you may need to enable the alloc feature and use Box from the alloc crate if it's not already available.
| write!(serial, "[W] Failed batched @ {addr:02X}, pruning nodes\r\n").ok(); | ||
| for cmd_idx in 0..explorer.nodes.len() { | ||
| failed_nodes.set(cmd_idx).ok(); | ||
| } |
There was a problem hiding this comment.
The failure handling logic for the batched write appears to be incorrect. When a batched write fails for a single address, the code marks all possible commands as failed. This set of failed nodes is then merged into global_failed_nodes. As a result, a single failure on one address will prevent any commands from being tested on all subsequent addresses, which is likely not the intended behavior.
Since the batched approach loses the granularity to know which specific command failed, you might want to reconsider this pruning strategy. A possible fix is to not update global_failed_nodes at all when a batched write fails, and just treat it as a failure for that specific address.
| write!(serial, "[W] Failed batched @ {addr:02X}, pruning nodes\r\n").ok(); | |
| for cmd_idx in 0..explorer.nodes.len() { | |
| failed_nodes.set(cmd_idx).ok(); | |
| } | |
| write!(serial, "[W] Failed batched @ {addr:02X}, cannot determine failing command.\r\n").ok(); |
| } | ||
|
|
||
| /// Errors that can occur during the exploration of command sequences. | ||
| #[derive(PartialEq, Eq)] |
There was a problem hiding this comment.
The Debug derive is missing for ExplorerError. It's good practice for error types to be debug-printable to aid in debugging.
| #[derive(PartialEq, Eq)] | |
| #[derive(Debug, PartialEq, Eq)] |
|
|
||
| /// Errors that can occur during command execution. | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| #[derive(PartialEq, Eq)] |
There was a problem hiding this comment.
The Debug derive is missing for ExecutorError. It's good practice for error types to be debug-printable to aid in debugging.
| #[derive(PartialEq, Eq)] | |
| #[derive(PartialEq, Eq, Debug)] |
|
/gemini summary |
Summary of ChangesThis pull request primarily focuses on resolving critical memory consumption challenges, particularly on resource-constrained embedded systems like the Arduino Uno, by fundamentally refactoring the application's command exploration and topological sorting mechanisms. It transitions from memory-intensive fixed-size arrays and dense graph representations to an efficient, iterator-based, and sparse adjacency list approach. This optimization, alongside a fix for garbled serial output and improvements to I2C communication, aims to drastically reduce SRAM usage while preserving core functionality and robust error handling. Highlights
Changelog
Activity
|
1 participant
🚀 Pull Request
Overview
Change details
Build / Test Results
Target board with confirmed operation