Feature/#99 - #100
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 introduces a substantial refactoring of the I2C command sequence explorer module. The primary goal is to improve the architecture by separating the core permutation algorithm from the I2C execution logic, making the code more modular and testable. Key changes include the adoption of an iterator-based API for permutations, the use of const generics for flexible capacity management, and the abstraction of logging through a new Logger trait. Additionally, error handling has been made more robust, and the command dependency mechanism has been refined for clarity and reliability. The PrefixExecutor in the scanner module has also been optimized for better performance.
Highlights
- Architectural Refactoring: The
explorermodule has undergone a significant architectural refactoring, separating the core permutation logic into a newPermutationIterstruct, which now implements theIteratortrait for a more idiomatic and composable API. - Generic Capacity: The
Explorerand related data structures now leverage Rust'sconst genericsby using a generic constantNfor capacity. This allows for compile-time fixed-size allocations, which is crucial for memory-constrained embedded environments. - Abstracted Logging: A new
Loggertrait has been introduced, providing a flexible and abstract way to handle logging. This decouples the logging mechanism from specific I/O implementations, making the library more adaptable to various embedded platforms without directcore::fmt::Writedependencies. - Enhanced Error Handling: The
ExplorerErrorenum has been expanded to include more granular error types such asDependencyCycle,NoValidAddressesFound,ExecutionFailed, andBufferOverflow, providing more robust and informative error handling. - Updated Dependency Mechanism: The
CmdNodedependencies have been updated to refer to command indices (usize) instead of the first byte of the dependent command (u8). This change provides a more explicit and robust way to define the dependency graph, preventing potential ambiguities. - Optimized PrefixExecutor: The
PrefixExecutorinscanner.rshas been optimized to apply the initial sequence only once per I2C address when it's first encountered, rather than for every command. This significantly improves performance by reducing redundant I2C bus operations.
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 introduces a significant and well-executed refactoring of the I2C command explorer. The separation of the permutation logic into a dedicated PermutationIter and the introduction of Logger and CmdExecutor traits greatly improve modularity, testability, and separation of concerns. The change from byte-based to index-based dependencies is also a major improvement in robustness.
My review focuses on a few areas for further improvement:
- A potential correctness issue in how the initialization sequence is sent in the new
PrefixExecutor. - Several opportunities to enhance performance by replacing linear searches over
Vecwith constant-time lookups in boolean arrays, which is more suitable for the embedded context.
Overall, this is a high-quality refactoring that makes the code more robust and maintainable. Addressing the feedback will further enhance its correctness and performance.
| let mut init_buf: Vec<u8, 64> = Vec::new(); | ||
| init_buf.push(self.prefix).map_err(|_| ())?; | ||
| init_buf.extend_from_slice(&self.init_sequence).map_err(|_| ())?; | ||
| i2c.write(addr, &init_buf).map_err(|_| ())?; |
There was a problem hiding this comment.
The new PrefixExecutor sends the entire initialization sequence as a single I2C write transaction. The previous implementation sent each command from the sequence in a separate transaction.
Many I2C devices require each command to be in its own transaction. Sending the whole sequence at once might cause the device initialization to fail. It's safer to revert to the previous behavior of iterating through the init_sequence and sending each command individually.
| let mut init_buf: Vec<u8, 64> = Vec::new(); | |
| init_buf.push(self.prefix).map_err(|_| ())?; | |
| init_buf.extend_from_slice(&self.init_sequence).map_err(|_| ())?; | |
| i2c.write(addr, &init_buf).map_err(|_| ())?; | |
| // First, send the init_sequence with the prefix | |
| for &c in self.init_sequence.iter() { | |
| let command = [self.prefix, c]; | |
| i2c.write(addr, &command).map_err(|_| ())?; | |
| } |
| let deps_satisfied = node.deps.iter().all(|&d| { | ||
| self.staged_and_current_indices.contains(&d) | ||
| }); |
There was a problem hiding this comment.
The current implementation checks for dependency satisfaction by calling .contains() on staged_and_current_indices, which is an O(N) operation. This check is in a hot loop within the permutation generation, and a similar check exists in the stage function. This can lead to significant performance degradation as the number of commands increases.
For better performance, consider replacing the staged_and_current_indices: Vec<usize, N> with a boolean array, like is_processed: [bool; N]. This would allow for O(1) dependency checks.
Here's how you could adapt the logic:
- In
Explorer::permutations, initialize a[bool; N]array, settingtruefor all indices that are part of the initialstaged_indices. - Pass this boolean array to
PermutationIter. - In
try_extend, check dependencies withself.is_processed[d]. - When a command is added, set
self.is_processed[idx] = true. - In
backtrack, reset it withself.is_processed[node_idx] = false.
This change would also apply to the stage function and would remove the need for the potentially panicking .unwrap() in backtrack since you would no longer need to find and remove elements from the Vec.
| use heapless::Vec; | ||
|
|
||
| // Check if the address has already been initialized | ||
| if self.initialized_addrs.iter().find(|&&a| a == addr).is_none() { |
There was a problem hiding this comment.
The check to see if an address has been initialized is done by a linear search (.iter().find()) on initialized_addrs. Since this check is performed for every command executed by the explorer, it can become a performance bottleneck.
A more performant approach would be to use a boolean array for tracking initialized addresses, which provides O(1) lookup time.
You would need to change the PrefixExecutor struct definition at line 397 from initialized_addrs: heapless::Vec<u8, 128> to initialized_addrs: [bool; 128] and initialize it accordingly in new().
| if self.initialized_addrs.iter().find(|&&a| a == addr).is_none() { | |
| if !self.initialized_addrs[addr as usize] { |
|
/gemini summary |
Summary of ChangesThis pull request introduces a significant refactoring of the I2C command sequence explorer, enhancing its modularity, flexibility, and error handling, particularly for embedded systems. The core algorithm is now separated from I2C execution logic, and a new iterator-based API improves testability and composability. Highlights
Changelog
Activity
|
Refactoring Candidates1. Separation of Responsibilities
Example: struct Explorer<'a> { sequence: &'a [CmdNode<'a>] }
impl<'a> Explorer<'a> {
fn stage(&self) -> (Vec<&[u8], CMD_CAPACITY>, Vec<usize, CMD_CAPACITY>);
fn permutations(&self, staged: &[&[u8]], unresolved: &[usize]) -> PermutationIter<'a>;
}→ 2. Hiding PermutationState
3. Abstraction of Log Processing
4. Change dependency resolution keys from
|
|
Thank you for this very detailed and insightful breakdown of the refactoring candidates and the improvements implemented! It's clear a lot of thought has gone into enhancing the design and robustness of the I particularly appreciate the emphasis on:
Regarding point 4, the change from Your summary also highlights the performance optimizations, such as the improved topological sorting efficiency and the removal of linear searches, which are critical for resource-constrained systems. The reduction in memory copies is also a welcome improvement. Overall, this refactoring demonstrates a strong commitment to clean architecture, maintainability, and performance, which are all highly valued in embedded systems development. Excellent work! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-designed refactoring of the explorer module, greatly improving its structure by separating concerns, introducing an iterator-based API, and enhancing error handling. The addition of the compat/ascii.rs module for hex formatting is also a good improvement. My review focuses on a critical bug in the new permutation algorithm, a design inconsistency regarding generic capacities that undermines one of the refactoring's goals, and a few smaller opportunities for improving code style and API consistency. Despite these issues, the overall direction of the changes is excellent.
| fn backtrack(&mut self) -> bool { | ||
| if let Some(last_added_pos) = self.path_stack.pop() { | ||
| let node_idx = self.unresolved_indices[last_added_pos]; | ||
| self.used_indices[node_idx] = false; | ||
|
|
||
| self.used[last_added_pos] = false; | ||
| self.current_permutation.pop(); | ||
|
|
||
| state.loop_start_indices.pop(); | ||
| self.path_stack.pop(); | ||
| self.path_stack.push(last_added_pos).unwrap(); | ||
|
|
||
| if let Some(last_loop_idx) = state.loop_start_indices.last_mut() { | ||
| *last_loop_idx += 1; | ||
| } else { | ||
| return false; | ||
| } | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
There appears to be a critical logic error in the backtrack function. The path_stack is popped twice: once in the if let Some(...) binding, and then again on line 381. This will corrupt the state of the permutation iterator, likely causing it to terminate prematurely or produce incorrect results.
The logic to advance to the next sibling in the search is correctly handled by the try_extend function, which uses the new top of the path_stack after the initial pop. The second pop and subsequent push are incorrect and should be removed.
fn backtrack(&mut self) -> bool {
if let Some(last_added_pos) = self.path_stack.pop() {
let node_idx = self.unresolved_indices[last_added_pos];
self.used_indices[node_idx] = false;
self.used[last_added_pos] = false;
self.current_permutation.pop();
true
} else {
false
}
}| pub struct Explorer<'a> { | ||
| /// The input sequence of command nodes (with dependencies). | ||
| pub sequence: &'a [CmdNode<'a>], | ||
| } | ||
|
|
There was a problem hiding this comment.
The documentation and overall design goal of this refactoring is to make the command capacity generic (e.g., Explorer<CAPACITY>). However, the Explorer struct itself is not generic, and its related components like PermutationIter and the stage function use hardcoded heapless::Vec capacities of 128. This creates a significant inconsistency between the documented design and the implementation, preventing the capacity from being truly configurable by the user.
To fully realize the generic capacity goal, Explorer and PermutationIter should be made generic over a const N: usize capacity parameter, which would then be used for the internal heapless collections.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@gemini-code-assist
This results in "multiple initialization sequences that appear correct." -- Reasons for this phenomenon:
Therefore, during bring-up, it is necessary to first resolve dependencies using an algorithm (Kahn algorithm or DFS search), For example:
In other words, the "search + log output framework" you're currently building is a tool for narrowing down the correct answer from multiple seemingly correct sequences. Directions:
If you want to automate bring-up, you'll need an observable and quantifiable "snowstorm" definition. Possible definitions1. I²C-level errors
2. Frame buffer echo check
3. Observation using an external sensor
4. Internal Logical Inference
Practical Step-by-Step Approach
In other words, if we "define snow algorithmically," Key points:
This image is for Arduino Uno + SH1107G 128x128. #![no_std]
#![no_main]
use core::fmt::Write;
use panic_abort as _;
use dvcdbg::prelude::*;
use dvcdbg::explorer::{CmdNode, ExplorerError};
adapt_serial!(UnoWrapper);
const BUF_CAP: usize = 4;
#[arduino_hal::entry]
fn main() -> ! {
let dp = arduino_hal::Peripherals::take().unwrap();
let pins = arduino_hal::pins!(dp);
let mut serial = UnoWrapper(arduino_hal::default_serial!(dp, pins, 57600));
arduino_hal::delay_ms(1000);
writeln!(serial, "[SH1107G Auto Backtrack Test]").ok();
let mut i2c = arduino_hal::I2c::new(
dp.TWI,
pins.a4.into_pull_up_input(),
pins.a5.into_pull_up_input(),
100_000,
);
writeln!(serial, "[Info] I2C initialized").ok();
static EXPLORER_CMDS: [CmdNode; 17] = [
CmdNode { bytes: &[0xAE], deps: &[] },
CmdNode { bytes: &[0xD5, 0x51], deps: &[0] },
CmdNode { bytes: &[0xA8, 0x3F], deps: &[1] },
CmdNode { bytes: &[0xD3, 0x60], deps: &[2] },
CmdNode { bytes: &[0x40, 0x00], deps: &[3] },
CmdNode { bytes: &[0xA1, 0x00], deps: &[4] },
CmdNode { bytes: &[0xA0], deps: &[5] },
CmdNode { bytes: &[0xC8], deps: &[6] },
CmdNode { bytes: &[0xAD, 0x8A], deps: &[7] },
CmdNode { bytes: &[0xD9, 0x22], deps: &[8] },
CmdNode { bytes: &[0xDB, 0x35], deps: &[9] },
CmdNode { bytes: &[0x8D, 0x14], deps: &[10] },
CmdNode { bytes: &[0xB0], deps: &[11] },
CmdNode { bytes: &[0x00], deps: &[11] },
CmdNode { bytes: &[0x10], deps: &[11] },
CmdNode { bytes: &[0xA6], deps: &[12, 13, 14] },
CmdNode { bytes: &[0xAF], deps: &[15] },
];
writeln!(serial, "[Info] Starting backtrack exploration...").ok();
let mut sequence = heapless::Vec::<usize, 17>::new();
let mut visited = [false; 17];
if backtrack(
&EXPLORER_CMDS,
&mut i2c,
&mut serial,
0x3C,
0x00,
&mut visited,
&mut sequence,
) {
writeln!(serial, "[OK] Found working sequence: {:?}", sequence).ok();
} else {
writeln!(serial, "[Fail] No working sequence found").ok();
}
loop { arduino_hal::delay_ms(1000); }
}
fn backtrack<I2C, S>(
cmds: &[CmdNode],
i2c: &mut I2C,
serial: &mut S,
addr: u8,
prefix: u8,
visited: &mut [bool; 17],
sequence: &mut heapless::Vec<usize, 17>,
) -> bool
where
I2C: embedded_hal::blocking::i2c::Write + embedded_hal::blocking::i2c::WriteRead,
<I2C as embedded_hal::blocking::i2c::Write>::Error: core::fmt::Debug,
S: core::fmt::Write,
{
if sequence.len() == cmds.len() {
if verify_vram(i2c, addr) {
return true;
} else {
writeln!(serial, "[Info] Sequence invalid, backtracking...").ok();
return false;
}
}
for i in 0..cmds.len() {
if visited[i] { continue; }
if cmds[i].deps.iter().all(|&d| visited[d]) {
writeln!(serial, "[Try] Node {} bytes={:02X?}", i, cmds[i].bytes).ok();
let buf_len = 1 + cmds[i].bytes.len();
if buf_len > BUF_CAP { continue; }
let mut buf = [0u8; BUF_CAP];
buf[0] = prefix;
buf[1..buf_len].copy_from_slice(cmds[i].bytes);
if i2c.write(addr, &buf[..buf_len]).is_err() {
writeln!(serial, "[Fail] Node {} write failed", i).ok();
continue;
}
visited[i] = true;
sequence.push(i).ok();
if backtrack(cmds, i2c, serial, addr, prefix, visited, sequence) {
return true;
}
visited[i] = false;
sequence.pop();
}
}
false
}
fn verify_vram<I2C>(i2c: &mut I2C, addr: u8) -> bool
where
I2C: embedded_hal::blocking::i2c::WriteRead,
{
let mut buf = [0u8; 16];
let cmd = [0xB0, 0x00, 0x10];
if i2c.write_read(addr, &cmd, &mut [0u8]).is_err() { return false; }
true
} |
|
Thank you for this incredibly detailed and insightful explanation of the challenges involved in bringing up I2C display controllers like the SH1107/SH1106/SSD130x! Your breakdown of why multiple sequences might return ACKs but still result in a "snowstorm" is very clear and highlights the complexities beyond simple I2C communication. Your analysis perfectly aligns with the broader design philosophy behind the I particularly appreciate your practical step-by-step approach to defining and detecting "snowstorm" conditions. This kind of domain-specific knowledge is invaluable for turning a generic permutation explorer into a truly effective bring-up tool. It's a testament to the iterative nature of embedded development, where initial algorithmic pruning is followed by increasingly sophisticated validation methods. It's great to see the |
|
/gemini summary |
Summary of ChangesThis pull request introduces a comprehensive refactoring of the I2C command sequence explorer, specifically tailored for embedded systems. The changes enhance modularity by separating the core permutation algorithm from I2C execution, improve flexibility through generic capacity and abstracted interfaces for logging and command execution, and bolster robustness with refined error handling and optimized performance. The update also provides new utilities for hex formatting and addresses critical issues related to I2C communication reliability and serial output clarity. Highlights
Changelog
Activity
|
1 participant
🚀 Pull Request
Overview
Change details
Build / Test Results