Conversation
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>
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>
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>
feature/explore
p14c31355
added
bug
![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 delivers the full implementation of an I2C command sequence exploration algorithm, designed to automatically discover valid initialization sequences for embedded devices. It significantly refines the underlying I2C communication and error reporting mechanisms, making the system more robust and efficient for resource-constrained microcontrollers. The changes aim to provide a powerful tool for embedded system bring-up and debugging.
Highlights
- Core Algorithm Implementation: The complete sequence exploration algorithm has been implemented, enabling automated discovery of I2C device initialization sequences.
- I2C Compatibility and Error Handling Refinement: The I2C compatibility layer has been enhanced with
probeand NACK detection methods, and the error handling system has undergone a significant refactoring for improved clarity and robustness. - Bitflag Utility and Logging Streamlining: A new
BitFlagsutility has been introduced for efficient I2C address tracking, and the dedicated logging module has been removed in favor of directcore::fmt::Writefor streamlined output. - Explorer API and Macros: The explorer API has been redesigned with
TopologicalIterfor efficient sorting and new macros (nodes!,pruning_sort!,get_one_sort!) have been added to simplify command sequence definition and exploration. - Documentation and Build Process Updates: The documentation for the exploration feature has been completely rewritten, and the
Cargo.tomlrelease profile settings have been optimized for embedded targets.
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 major and impressive refactoring of the sequence exploration logic. The move from generating all permutations with PermutationIter to a single topological sort with TopologicalIter is a significant improvement in terms of performance and practicality. The removal of the custom Logger trait in favor of the standard core::fmt::Write simplifies the API. The new BitFlags utility and the nodes! macro are also excellent additions.
However, the review identified a few critical issues that need to be addressed. The pruning_explorer's logic seems to be flawed due to command batching, which prevents correct pruning of failing commands. There's also a bug in scan_i2c where the ctrl_byte is not used during the scan. Additionally, the error types are missing Debug implementations, which is crucial for diagnostics. I've provided detailed comments and suggestions for these points. Once these issues are resolved, this will be a fantastic contribution to the library.
| match i2c.write(addr, &batched) { | ||
| Ok(_) => { | ||
| write!( | ||
| serial, | ||
| "[I] OK batched @ {addr:02X} ({} bytes)\r\n", | ||
| batched.len() | ||
| ) | ||
| .ok(); | ||
| } | ||
| Err(_) => { | ||
| 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 current implementation of pruning_explorer batches all commands from a topological sort into a single i2c.write call. If this write fails, it's impossible to determine which specific command in the batch caused the failure. The logic then proceeds to mark all nodes as failed, which is incorrect and defeats the purpose of "pruning" individual failing commands.
To correctly identify and prune the failing node, the commands should be sent one by one within the loop. Batching can be a performance optimization, but in this case, it breaks the core pruning functionality.
| #[derive(Clone, Copy, PartialEq, Eq)] | ||
| pub enum ErrorKind { |
There was a problem hiding this comment.
The error enums in this file (e.g., ErrorKind, UartError, I2cError, ExplorerError, etc.) should implement core::fmt::Debug. This is standard practice for error types in Rust and is essential for debugging, for example when printing an error with {:?}. Please add #[derive(Debug)] to all error enums in this file.
| #[derive(Clone, Copy, PartialEq, Eq)] | |
| pub enum ErrorKind { | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |
| pub enum ErrorKind { |
| pub fn scan_i2c<I2C, W>( | ||
| i2c: &mut I2C, | ||
| writer: &mut W, | ||
| ctrl_byte: u8, |
There was a problem hiding this comment.
The ctrl_byte parameter in scan_i2c is not used in the actual scanning logic; it is only used for logging. The scan is performed by internal_scan, which calls i2c.probe(). The probe implementations perform a zero-byte write, ignoring the ctrl_byte. This is misleading, as the function signature suggests the ctrl_byte is being used to probe devices. The probe implementation should be updated to write the ctrl_byte or this function should be changed to not take it.
| pub fn one_topological_explorer<I2C, S, const N: usize, const INIT_SEQUENCE_LEN: usize, const CMD_BUFFER_SIZE: usize, const MAX_DEPS: usize>( | ||
| explorer: &Explorer<N, MAX_DEPS>, | ||
| i2c: &mut I2C, | ||
| serial: &mut S, | ||
| prefix: u8, | ||
| ) -> Result<(), ExplorerError> | ||
| ``` |
There was a problem hiding this comment.
The INIT_SEQUENCE_LEN const generic parameter is confusing because the implementation in runner.rs hardcodes an empty initialization sequence, making this parameter seem redundant for the caller. To simplify the API, consider removing it and hardcoding the generic argument to 0 where PrefixExecutor is instantiated.
| match embedded_hal_0_2::blocking::i2c::Write::write(self, addr, &[]) { | ||
| Ok(_) => Ok(true), | ||
| Err(_) => Ok(false), // Assume any error means the device is not present. | ||
| } |
There was a problem hiding this comment.
The probe implementation for ehal-0.2 swallows all I2C errors by mapping Err(_) to Ok(false). This can hide important bus errors (e.g., arbitration loss) by misreporting them as a simple NACK (device not present). While ehal-0.2 errors are not standardized, it would be safer to propagate any error. Since distinguishing errors is hard, at a minimum this behavior should be clearly documented in the function's doc comment to warn users about the risk of masked bus faults.
| { | ||
| let mut last_error = None; | ||
| for _attempt in 0..10 { | ||
| for _attempt in 0..2 { |
There was a problem hiding this comment.
The number of retries in write_with_retry has been reduced from 10 to 2 (one initial attempt, one retry). This might be too low for noisy I2C buses or slow devices, potentially leading to premature failures. Consider making the retry count configurable or increasing it to a more robust value like 3 or 5.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and impressive refactoring of the I2C sequence exploration functionality. The move from generating all permutations to using a topological sort is a major improvement for performance and practicality in embedded environments. The introduction of the BitFlags utility, the cleanup of error handling with Display implementations, and the new nodes! macro for defining command sequences greatly enhance the library's usability and efficiency.
While the overall direction is excellent, I've identified a few issues. Most critically, the new pruning logic in pruning_explorer appears to be too aggressive, potentially marking all commands as failed globally after a single batch failure. I've also noted some areas for improvement in the ehal_0_2 compatibility layer, API consistency in the new runner functions, and error type definitions. Addressing these points will help solidify this powerful new feature.
| match i2c.write(addr, &batched) { | ||
| Ok(_) => { | ||
| write!( | ||
| serial, | ||
| "[I] OK batched @ {addr:02X} ({} bytes)\r\n", | ||
| batched.len() | ||
| ) | ||
| .ok(); | ||
| } | ||
| Err(_) => { | ||
| 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 pruning logic in pruning_explorer appears to be flawed. When a batched write fails, all command nodes are marked as failed for the current address. These failures are then merged into global_failed_nodes. This means that a single failure on one address will cause all subsequent explorations on other addresses to start with all nodes marked as failed, leading to empty command sequences.
This behavior is too aggressive and doesn't achieve granular pruning. The logic should ideally identify the specific failing command or, if that's not possible with batching, at least not pollute the global failure state so drastically. This is a critical issue as it breaks the core functionality of the pruning explorer.
| fn probe(&mut self, addr: u8) -> Result<bool, Self::Error> { | ||
| // For ehal 0.2.x, there's no standard way to check for NACK specifically | ||
| // without a custom error type that implements a `kind()` method. | ||
| // We'll assume any error means the device is not present or there's a bus issue. | ||
| // A successful 0-byte write implies the device is present. | ||
| match embedded_hal_0_2::blocking::i2c::Write::write(self, addr, &[]) { | ||
| Ok(_) => Ok(true), | ||
| Err(_) => Ok(false), // Assume any error means the device is not present. | ||
| } |
There was a problem hiding this comment.
The probe implementation for ehal_0_2 compatibility swallows all errors and returns Ok(false). This can hide important bus errors (e.g., arbitration lost) by treating them as a simple non-response (NACK). This prevents the caller from distinguishing between a missing device and a malfunctioning I2C bus.
To ensure robustness, underlying bus errors should be propagated to the caller instead of being suppressed.
match embedded_hal_0_2::blocking::i2c::Write::write(self, addr, &[]) {
Ok(_) => Ok(true),
Err(e) => {
// Attempt to check if the error is a NACK. Since ehal 0.2 lacks a standard way,
// this is a best-effort guess. For many HALs, a NACK is the only error from a 0-byte write.
// If not a NACK, the error should be propagated.
// For now, we will treat any error as a potential NACK, but ideally, HALs would provide more info.
Ok(false)
// A more robust implementation would be:
// if is_nack_error(&e) { Ok(false) } else { Err(e) }
// But without a standard way to check, this is a compromise.
}
}| pub fn one_topological_explorer<I2C, S, const N: usize, const INIT_SEQUENCE_LEN: usize, const CMD_BUFFER_SIZE: usize, const MAX_DEPS: usize>( | ||
| explorer: &Explorer<N, MAX_DEPS>, | ||
| i2c: &mut I2C, | ||
| serial: &mut S, | ||
| prefix: u8, | ||
| ) -> Result<(), ExplorerError> | ||
| ``` |
There was a problem hiding this comment.
The function signature for one_topological_explorer includes the const generic parameter INIT_SEQUENCE_LEN, but this function's purpose is to execute a single command sequence on a detected device, not to perform an initialization sequence scan. The implementation in runner.rs confirms this by always using an empty init sequence. This parameter is confusing for users of the API and should be removed to improve clarity.
| #[derive(PartialEq, Eq)] | ||
| pub enum ExplorerError { | ||
| /// The provided sequence contained more commands than supported by the capacity N. | ||
| /// The provided sequence contained more commands than supported by the capacity. | ||
| TooManyCommands, | ||
| /// The command dependency graph contains a cycle. | ||
| DependencyCycle, | ||
| /// No valid I2C addresses were found for any command sequence. | ||
| NoValidAddressesFound, | ||
| /// An I2C command execution failed. | ||
| ExecutionFailed(crate::error::ErrorKind), | ||
| /// An internal buffer overflowed. | ||
| ExecutionFailed(ErrorKind), | ||
| /// An internal buffer overflowed during the exploration process. | ||
| BufferOverflow, | ||
| /// A dependency index is out of bounds. | ||
| InvalidDependencyIndex, | ||
| /// An I2C scan operation failed. | ||
| DeviceNotFound(crate::error::ErrorKind), | ||
| /// An I2C device was not found during a scan operation. | ||
| DeviceNotFound(ErrorKind), | ||
| /// An error occurred in the BitFlags utility. | ||
| BitFlags(BitFlagsError), | ||
| } | ||
|
|
||
| impl fmt::Display for ExplorerError { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| ExplorerError::TooManyCommands => f.write_str("TooManyCommands"), | ||
| ExplorerError::DependencyCycle => f.write_str("DependencyCycle"), | ||
| ExplorerError::NoValidAddressesFound => f.write_str("NoValidAddressesFound"), | ||
| ExplorerError::ExecutionFailed(kind) => write!(f, "ExecutionFailed: {kind}"), | ||
| ExplorerError::BufferOverflow => f.write_str("BufferOverflow"), | ||
| ExplorerError::InvalidDependencyIndex => f.write_str("InvalidDependencyIndex"), | ||
| ExplorerError::DeviceNotFound(kind) => write!(f, "DeviceNotFound: {kind}"), | ||
| ExplorerError::BitFlags(e) => write!(f, "BitFlagsError: {e}"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Errors that can occur during command execution. | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| #[derive(PartialEq, Eq)] | ||
| pub enum ExecutorError { | ||
| /// The command failed to execute due to an I2C error. | ||
| I2cError(crate::error::ErrorKind), | ||
| /// A command failed to execute due to an I2C error. | ||
| I2cError(ErrorKind), | ||
| /// The command failed to execute (e.g., NACK, I/O error). | ||
| ExecFailed, | ||
| /// An internal buffer overflowed during command preparation. | ||
| BufferOverflow, | ||
| /// An error occurred in the BitFlags utility. | ||
| BitFlags(BitFlagsError), | ||
| /// An error occurred in the explorer module. | ||
| Explorer(ExplorerError), | ||
| } |
There was a problem hiding this comment.
The ExplorerError and ExecutorError enums are missing the #[derive(Debug)] attribute. While Display is implemented, which is great for user-facing messages, Debug is standard for error types in Rust. It's crucial for developers during debugging to be able to print the full structure of an error, especially for nested errors like ExecutionFailed(ErrorKind). Please add #[derive(Debug)] to both enums.
/// Errors that can occur during the exploration of command sequences.
#[derive(Debug, PartialEq, Eq)]
pub enum ExplorerError {
/// The provided sequence contained more commands than supported by the capacity.
TooManyCommands,
/// The command dependency graph contains a cycle.
DependencyCycle,
/// No valid I2C addresses were found for any command sequence.
NoValidAddressesFound,
/// An I2C command execution failed.
ExecutionFailed(ErrorKind),
/// An internal buffer overflowed during the exploration process.
BufferOverflow,
/// A dependency index is out of bounds.
InvalidDependencyIndex,
/// An I2C device was not found during a scan operation.
DeviceNotFound(ErrorKind),
/// An error occurred in the BitFlags utility.
BitFlags(BitFlagsError),
}
impl fmt::Display for ExplorerError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
ExplorerError::TooManyCommands => f.write_str("TooManyCommands"),
ExplorerError::DependencyCycle => f.write_str("DependencyCycle"),
ExplorerError::NoValidAddressesFound => f.write_str("NoValidAddressesFound"),
ExplorerError::ExecutionFailed(kind) => write!(f, "ExecutionFailed: {kind}"),
ExplorerError::BufferOverflow => f.write_str("BufferOverflow"),
ExplorerError::InvalidDependencyIndex => f.write_str("InvalidDependencyIndex"),
ExplorerError::DeviceNotFound(kind) => write!(f, "DeviceNotFound: {kind}"),
ExplorerError::BitFlags(e) => write!(f, "BitFlagsError: {e}"),
}
}
}
/// Errors that can occur during command execution.
#[derive(Debug, PartialEq, Eq)]
pub enum ExecutorError {
/// A command failed to execute due to an I2C error.
I2cError(ErrorKind),
/// The command failed to execute (e.g., NACK, I/O error).
ExecFailed,
/// An internal buffer overflowed during command preparation.
BufferOverflow,
/// An error occurred in the BitFlags utility.
BitFlags(BitFlagsError),
/// An error occurred in the explorer module.
Explorer(ExplorerError),
}| { | ||
| let mut last_error = None; | ||
| for _attempt in 0..10 { | ||
| for _attempt in 0..2 { |
There was a problem hiding this comment.
The number of retries in write_with_retry has been reduced to 2 attempts (1 retry). This may be insufficient for I2C buses in noisy environments, potentially leading to premature failures. The previous implementation used 10 attempts. Consider increasing this value or making it a configurable constant to improve robustness.
| for _attempt in 0..2 { | |
| for _attempt in 0..5 { |
| pub fn one_topological_explorer< | ||
| I2C, | ||
| S, | ||
| const N: usize, | ||
| const INIT_SEQUENCE_LEN: usize, | ||
| const CMD_BUFFER_SIZE: usize, | ||
| const MAX_DEPS: usize, | ||
| >( | ||
| explorer: &Explorer<N, MAX_DEPS>, | ||
| i2c: &mut I2C, | ||
| serial: &mut S, | ||
| target_addr: u8, | ||
| prefix: u8, | ||
| log_level: LogLevel, | ||
| ) -> Result<(), ExplorerError> |
There was a problem hiding this comment.
The function one_topological_explorer and its wrapper macro get_one_sort! include a const INIT_SEQUENCE_LEN generic parameter. However, this function always initializes the PrefixExecutor with an empty init sequence, making this parameter unused and confusing. To improve API clarity, this generic parameter should be removed or defaulted to 0.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
🚀 Pull Request
Overview
Change details
Build / Test Results
Target board with confirmed operation
Screenshots / Demos (optional)
Others
Finally, a sequence exploration algorithm that runs on AVR has been realized.