Support #100 - #102
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 an optimization to the I2C command sequence explorer by implementing a hash-based mechanism to identify and skip redundant "half-correct" sequences. This enhancement aims to improve the flexibility and efficiency of discovering valid I2C command permutations, particularly in embedded device bring-up scenarios where multiple initialization sequences might lead to the same state. It also involves refactoring the explorer.rs module by removing verbose internal documentation and a function for generating a single topological sort, streamlining the codebase.
Highlights
- Performance Optimization: Implemented a hash-based caching mechanism using heapless::FnvIndexMap and crc32fast to store and check previously processed command sequences. This prevents redundant execution of "half-correct" sequences, significantly improving the efficiency of the I2C command exploration process.
- Code Refactoring and Simplification: Removed extensive internal documentation comments, including module-level explanations, trait descriptions, and example usage from src/explorer.rs. This streamlines the code, focusing on the core logic.
- Feature Removal: The get_one_topological_sort_buf function, which was responsible for generating a single valid topological sort, has been removed. This indicates a shift in focus from single-sort generation to more comprehensive permutation exploration.
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 hashing mechanism to deduplicate command sequence permutations, which is a good optimization for the exploration logic. However, the changes also include the removal of a large amount of documentation and comments, which significantly reduces the maintainability of this complex module. Additionally, I've identified two critical issues that will likely cause the build to fail: a removed function that is still used elsewhere, and a type mismatch in the new hashing logic. Please see the detailed comments for suggestions on how to address these issues.
|
@gemini-code-assist |
|
Hi @p14c31355, Thanks for reaching out! I noticed your question about the detailed comments. In this pull request, a significant number of comments, including module-level documentation, struct/enum explanations, trait descriptions, and internal logic comments, were removed from For example, the extensive doc comments explaining the purpose of the While removing redundant or overly verbose comments can sometimes improve code clarity, comprehensive documentation is crucial for understanding complex algorithms and APIs, especially in a module like I recommend considering the following:
Good documentation improves maintainability, reduces the learning curve for new contributors, and helps prevent misinterpretations of the code's behavior. If the goal was to move this documentation elsewhere (e.g., to external documentation files), it would be helpful to indicate that in the code or PR description. Let me know if you'd like me to provide specific examples of comments that could be restored or rephrased. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the I2C command sequence exploration logic by introducing a hash-based approach to avoid re-testing permutations. This is a good improvement for performance and flexibility. However, the changes have removed significant amounts of documentation, valuable logging for debugging, and a performance optimization. My review focuses on restoring these aspects to improve maintainability and usability.
| if let Err(_) = executor.exec(i2c, addr_val, self.sequence[idx].bytes, logger) { | ||
| all_ok = false; | ||
| logger.log_error_fmt(|buf| { | ||
| write!(buf, "[explorer] Execution failed for addr ")?; | ||
| ascii::write_bytes_hex_fmt(buf, &[addr_val])?; | ||
| write!(buf, ": {e:?}\r\n")?; | ||
| Ok(()) | ||
| }); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The error logging for a failed command execution has been removed. Previously, if executor.exec failed, an error message including the I2C address and the specific error was logged. This is critical for debugging why a particular sequence is failing for a device. Please reintroduce this logging.
| if let Err(_) = executor.exec(i2c, addr_val, self.sequence[idx].bytes, logger) { | |
| all_ok = false; | |
| logger.log_error_fmt(|buf| { | |
| write!(buf, "[explorer] Execution failed for addr ")?; | |
| ascii::write_bytes_hex_fmt(buf, &[addr_val])?; | |
| write!(buf, ": {e:?}\r\n")?; | |
| Ok(()) | |
| }); | |
| break; | |
| } | |
| if let Err(e) = executor.exec(i2c, addr_val, self.sequence[idx].bytes, logger) { | |
| all_ok = false; | |
| logger.log_error_fmt(|buf| { | |
| use core::fmt::Write; | |
| let _ = write!(buf, "[explorer] Execution failed for addr 0x{:02X}: {:?}\r\n", addr_val, e); | |
| Ok(()) | |
| }); | |
| break; | |
| } |
| use heapless::Vec; | ||
|
|
||
| use heapless::index_map::FnvIndexMap; | ||
| use crate::scanner::{I2C_SCAN_ADDR_END, I2C_SCAN_ADDR_START}; | ||
| const I2C_ADDRESS_COUNT: usize = 128; | ||
| pub const LOG_BUFFER_CAPACITY: usize = 1024; | ||
|
|
||
| /// Errors that can occur during exploration of command sequences. | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub enum ExplorerError { | ||
| /// The provided sequence contained more commands than supported by the capacity N. | ||
| 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, | ||
| /// An internal buffer overflowed. | ||
| BufferOverflow, | ||
| /// A dependency index is out of bounds. | ||
| InvalidDependencyIndex, | ||
| } | ||
|
|
||
| /// Errors that can occur during command execution. | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub enum ExecutorError { | ||
| /// The command failed to execute due to an I2C error. | ||
| I2cError(crate::error::ErrorKind), | ||
| /// The command failed to execute (e.g., NACK, I/O error). | ||
| ExecFailed, | ||
| /// An internal buffer overflowed during command preparation. | ||
| BufferOverflow, | ||
| } | ||
|
|
There was a problem hiding this comment.
This refactoring has removed a substantial amount of documentation from the file header and for ExplorerError, ExecutorError, and CmdNode. The previous documentation provided a valuable overview, explanation of key features, and an example, which are crucial for maintainability and for new developers to understand the code. Please consider restoring the detailed documentation.
| visited_nodes[idx] = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The optimization to stop exploring once all possible I2C addresses have been found has been removed. This can lead to unnecessary work if a valid sequence is found early on. Please consider adding this optimization back before marking nodes as visited.
// Optimization: if all possible addresses have been found, we can stop.
if found_addresses.len() == (I2C_SCAN_ADDR_END - I2C_SCAN_ADDR_START + 1) as usize {
break;
}
for &idx in order.iter() {
visited_nodes[idx] = true;
}There was a problem hiding this comment.
@gemini-code-assist
This optimization has been removed because it could potentially cause the process to terminate before finishing searching for a truly valid initialization sequence command sequence for the device.
The process will be managed by hash values, which is an approach to converge the computational complexity.
There was a problem hiding this comment.
Thank you for the clarification. I understand that the optimization was removed to allow for a more exhaustive search for a "truly valid initialization sequence," and that hash values are now being used to manage the computational complexity of this broader search. This design choice makes sense given the objective of finding the most robust sequence rather than simply the first one that works for all addresses.
| if found_addresses.is_empty() { | ||
| Err(ExplorerError::NoValidAddressesFound) | ||
| } else { | ||
| Ok(ExploreResult { | ||
| found_addrs: found_addresses, | ||
| permutations_tested: permutation_count, | ||
| }) | ||
| return Err(ExplorerError::NoValidAddressesFound); | ||
| } | ||
|
|
||
| Ok(ExploreResult { | ||
| found_addrs: found_addresses, | ||
| permutations_tested: permutation_count, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The summary log message at the end of the exploration has been removed. This message, which reported the total number of found addresses and permutations tested, is very useful for understanding the outcome of the exploration process. Please add it back.
logger.log_info_fmt(|buf| {
use core::fmt::Write;
let _ = writeln!(
buf,
"[explorer] Exploration complete. {} addresses found across {} permutations.",
found_addresses.len(),
permutation_count
);
Ok(())
});
if found_addresses.is_empty() {
return Err(ExplorerError::NoValidAddressesFound);
}
Ok(ExploreResult {
found_addrs: found_addresses,
permutations_tested: permutation_count,
})
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the command sequence exploration logic, replacing the permutation-based approach with a new algorithm that uses topological sorting and hashing. This is a promising direction for improving performance. The changes to the Logger to make it generic over buffer size are also a good improvement for flexibility.
My review focuses on the new exploration logic in src/explorer.rs. I've identified a potential logic flaw that could cause valid sequences to be discarded. Additionally, a significant amount of valuable documentation has been removed throughout the file, which I recommend restoring to maintain code clarity. There are also a couple of minor stylistic issues with use statements inside closures.
| if hash_table.contains_key(&hash) { | ||
| for &idx in order.iter() { | ||
| visited_nodes[idx] = true; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic for handling a previously seen sequence hash appears to be flawed. If a hash is found in hash_table, the code marks the nodes of the current sequence as visited and then continues the loop. This means the sequence is never tested against any I2C addresses, but its constituent commands are removed from future consideration. This could lead to valid solutions being missed because a valid sequence is discarded without being tested.
As it is, this logic seems to discard potentially valid sequences. Consider removing this block or rethinking the logic to ensure all generated topological sorts are tested at least once.
There was a problem hiding this comment.
@gemini-code-assist
What you do with exec() will likely determine whether you can significantly refactor it.
There was a problem hiding this comment.
That's a very insightful question, p14c31355. The behavior of exec() is indeed crucial for certain optimizations, but the primary concern I raised about the hash-based skipping logic is more fundamental to how the permutations themselves are generated.
The current approach in the explore function iteratively calls kahn_topo_sort and then marks the nodes of the resulting sequence as visited. This visited flag then prevents these nodes from being considered in subsequent calls to kahn_topo_sort within the same explore loop. This means that once a command node is part of any generated sequence, it's effectively removed from the graph for all future topological sort generations.
If the goal is to find all valid command sequences (topological sorts) to test against I2C addresses, this method will not achieve that. Many valid permutations that share common commands will be missed because those common commands are prematurely marked as 'visited'. For example, if you have dependencies A -> B and A -> C, you could have sequences like [A, B, C] and [A, C, B]. If [A, B, C] is generated and its nodes (A, B, C) are marked visited, then [A, C, B] will never be generated.
While exec()'s idempotency or side effects would influence whether it's safe to skip re-executing an already tested sequence (e.g., if its hash matches), it doesn't resolve the issue of generating all unique sequences in the first place. To truly explore all valid permutations, a comprehensive backtracking algorithm (similar to the PermutationIter that was previously present) is typically required, rather than iteratively finding one sort and then removing its nodes from consideration.
Therefore, the refactoring would primarily involve revisiting the permutation generation strategy to ensure all possible topological sorts are produced, independent of exec()'s specific behavior. Once all permutations are reliably generated, then exec()'s characteristics can be used to optimize the testing of those permutations (e.g., by using the hash table to avoid redundant executions if exec() is idempotent).
There was a problem hiding this comment.
@gemini-code-assist
So, to perform a topological sort, attempt an I2C transmission, perform hash marking, and then perform a topological sort again as if the search has been completed, it would be better to use a helper function for the marking itself.
If you use a loop in such a process, it seems better to separate the functions.
There was a problem hiding this comment.
That's a good point about code organization, p14c31355. Encapsulating the hash marking and visited node updates into a dedicated helper function would indeed improve the readability and modularity of the explore function. Similarly, breaking down complex loops into smaller, focused functions is generally a good practice. However, it's important to reiterate that while such refactoring would make the current logic cleaner, it wouldn't address the fundamental algorithmic limitation if the goal is to find all unique topological sorts of the original dependency graph. The current approach, where kahn_topo_sort is called repeatedly and the nodes of the generated sequence are marked visited, effectively prunes the graph for subsequent iterations. This means that once a set of commands forms a valid sequence and is processed, those commands are removed from consideration for any other sequence generation within the same explore call. This inherently prevents the discovery of permutations that share common commands with already processed sequences. If the objective is to find all possible valid command sequences (topological sorts), then a comprehensive permutation generation algorithm (like the PermutationIter that was previously used, which employs backtracking) is necessary. The iterative application of Kahn's algorithm with node removal, while efficient for finding a topological sort or distinct sets of commands, is not designed to enumerate all permutations. Could you clarify if the goal has shifted from finding all valid permutations to finding a set of distinct, "half-correct" sequences as mentioned in the PR description? If the latter is the case, then the current approach might align with that new objective, but it's crucial to be clear about the intended scope of the exploration.
| use heapless::Vec; | ||
|
|
||
| use crate::scanner::{I2C_SCAN_ADDR_END, I2C_SCAN_ADDR_START}; | ||
| use heapless::Vec; |
There was a problem hiding this comment.
A significant amount of documentation has been removed from this file, including:
- The detailed module-level documentation explaining the purpose, features, and usage of the explorer (previously lines 1-91).
- Documentation for
ExplorerErrorandExecutorErrorvariants. - Documentation for the
CmdNodestruct andCmdExecutortrait.
This documentation is very valuable for maintainability and for new contributors. Please consider restoring it to maintain the project's documentation quality.
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>
add USAGE_EXPLORE.md
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new algorithm for exploring I2C command sequences, aiming to be more flexible by identifying "half-correct" sequences. It also refactors the logging mechanism to be more generic and moves documentation to a dedicated markdown file. The new exploration algorithm is an interesting idea, but its current implementation seems to have a logical flaw that prevents it from exploring more than one permutation. Additionally, there are a couple of critical compilation errors due to incomplete refactoring. The documentation has also been moved but needs some formatting cleanup.
| for &idx in order.iter() { | ||
| visited_nodes[idx] = true; | ||
| } |
There was a problem hiding this comment.
The new exploration algorithm seems to have a logical flaw that limits its effectiveness. After finding and testing the first valid command sequence (order), all nodes in that sequence are marked as visited. This prevents any of these nodes from being part of subsequent sequences found by kahn_topo_sort. As a result, the explorer will only ever test one permutation (or a few disjoint ones if the graph is disconnected), rather than exploring multiple overlapping combinations as the PR description seems to intend. You might want to reconsider how visited_nodes is updated to allow for more thorough exploration.
| @@ -0,0 +1,246 @@ | |||
| # I2C Command Sequence Explorer | |||
| //! | |||
There was a problem hiding this comment.
This new documentation file contains Rust-specific doc comments like //! and /// (e.g., on lines 2, 108, 114, etc.). These are not standard Markdown and can be confusing for readers of the documentation. It would be better to convert them into regular Markdown text, using headings, code blocks, and lists to structure the information.
For example, //! at the top of the file should be removed, and /// comments should be converted to regular text or structured with Markdown headings.
| #[derive(Debug, PartialEq, Eq)] | ||
| pub enum ExplorerError { | ||
| /// The provided sequence contained more commands than supported by the capacity N. | ||
| 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, | ||
| /// An internal buffer overflowed. | ||
| BufferOverflow, | ||
| /// A dependency index is out of bounds. | ||
| InvalidDependencyIndex, | ||
| } | ||
|
|
||
| /// Errors that can occur during command execution. | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub enum ExecutorError { | ||
| /// The command failed to execute due to an I2C error. | ||
| I2cError(crate::error::ErrorKind), | ||
| /// The command failed to execute (e.g., NACK, I/O error). | ||
| ExecFailed, | ||
| /// An internal buffer overflowed during command preparation. | ||
| BufferOverflow, | ||
| } |
There was a problem hiding this comment.
The documentation for the variants of ExplorerError and ExecutorError has been removed. These comments were helpful for understanding the meaning of each error. Please consider adding them back to improve code clarity and maintainability.
/// Errors that can occur during exploration of command sequences.
#[derive(Debug, PartialEq, Eq)]
pub enum ExplorerError {
/// The provided sequence contained more commands than supported by the capacity N.
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,
/// An internal buffer overflowed.
BufferOverflow,
/// A dependency index is out of bounds.
InvalidDependencyIndex,
}
/// Errors that can occur during command execution.
#[derive(Debug, PartialEq, Eq)]
pub enum ExecutorError {
/// The command failed to execute due to an I2C error.
I2cError(crate::error::ErrorKind),
/// The command failed to execute (e.g., NACK, I/O error).
ExecFailed,
/// An internal buffer overflowed during command preparation.
BufferOverflow,
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the I2C explorer functionality. Key changes include moving documentation to a dedicated markdown file, making the Logger and CmdExecutor more flexible with const generics, and introducing a new "pruned" exploration strategy to find partially correct command sequences. The overall changes improve modularity and add powerful new features. However, I've found a critical issue in the Explorer::max_cmd_len function that will prevent compilation or cause a panic at runtime. Additionally, the new documentation file contains raw doc comments that need to be formatted properly for readability.
| /// Returns a stack-safe iterator for all valid command permutations (topological sorts). | ||
| /// | ||
| /// This function first performs cycle detection using a modified Kahn's algorithm. | ||
| /// If a cycle is detected, it returns an `ExplorerError::DependencyCycle`. | ||
| /// Otherwise, it initializes and returns a `PermutationIter` to generate all valid permutations. | ||
|
|
||
| /// Explores valid sequences, attempting to execute them on an I2C bus. | ||
| /// | ||
| /// This function iterates through all valid command permutations generated by `PermutationIter`, | ||
| /// and for each permutation, attempts to execute it on all active I2C addresses. | ||
| /// It filters out addresses that fail to respond to any command in a sequence. | ||
| /// | ||
| /// # Parameters | ||
| /// - `i2c`: An I2C implementation used to test candidate sequences against device addresses. | ||
| /// - `executor`: The object responsible for executing a single command on the bus. | ||
| /// - `logger`: The object responsible for logging progress and results. | ||
| /// | ||
| /// # Returns | ||
| /// - `Ok(ExploreResult)` containing the list of found addresses and the number of permutations tested. | ||
| /// - `Err(ExplorerError)` if an error occurs during permutation generation or I2C execution. | ||
|
|
||
| /// Generates a single valid topological sort of the command sequence. | ||
| /// This is useful when only one valid ordering is needed, and avoids | ||
| /// the computational cost of generating all permutations. | ||
| /// | ||
| /// Returns `Ok(Vec<&'a [u8], N>)` containing one valid command sequence, | ||
| /// or `Err(ExplorerError)` if a cycle is detected or buffer overflows. | ||
|
|
||
| /// Attempts to extend the current partial permutation by adding a new command. | ||
| /// | ||
| /// It iterates through all available (not yet used) commands and checks if their | ||
| /// dependencies are satisfied (i.e., their in-degree is 0). If a valid command | ||
| /// is found, it's added to the current permutation, its `used` status is updated, | ||
| /// and the in-degrees of its dependent nodes are decremented. | ||
| /// | ||
| /// Returns `true` if a command was successfully added, `false` otherwise. | ||
|
|
||
| /// Backtracks to the previous decision point in the permutation search. | ||
| /// | ||
| /// This method undoes the last choice made: it removes the last added command | ||
| /// from the current permutation, unmarks it as used, and increments the | ||
| /// in-degrees of its dependent nodes (reversing the decrement). | ||
| /// It then updates the `loop_start_indices` to ensure the next search | ||
| /// at the parent level continues from the next sibling. | ||
| /// | ||
| /// Returns `true` if backtracking can continue (i.e., there are more options | ||
| /// at a previous level), or `false` if the root was reached and no more | ||
| /// permutations can be generated. | ||
|
|
||
| /// Runs the I2C explorer with a given initial sequence and logs the results. | ||
| /// | ||
| /// This function first performs an I2C scan with the provided `init_sequence` to identify | ||
| /// responsive commands. Then, it uses the `explorer` to find valid command sequences | ||
| /// for discovered devices, applying a `prefix` to each command. | ||
| /// | ||
| /// # Type Parameters | ||
| /// | ||
| /// - `I2C`: The I2C interface type that implements `crate::compat::I2cCompat`. | ||
| /// - `S`: The serial interface type used for logging, implementing `core::fmt::Write`. | ||
| /// - `N`: A const generic for the maximum number of commands. | ||
| /// - `BUF_CAP`: A const generic for the command buffer capacity. | ||
| /// | ||
| /// # Parameters | ||
| /// | ||
| /// - `explorer`: An instance of `Explorer` containing the command nodes and their dependencies. | ||
| /// - `i2c`: The I2C bus instance. | ||
| /// - `serial`: The serial writer for logging. | ||
| /// - `init_sequence`: The initial sequence of bytes to test for device responsiveness. | ||
| /// - `prefix`: A byte to prepend to every command sent during exploration. | ||
| /// - `log_level`: The desired logging level. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```ignore | ||
| /// use dvcdbg::prelude::*; | ||
| /// use arduino_hal::I2c; | ||
| /// use arduino_hal::hal::port::Port; | ||
| /// use arduino_hal::pac::TWI; | ||
| /// use heapless::Vec; | ||
| /// use core::fmt::Write; | ||
| /// | ||
| /// # struct MyI2c; // Dummy I2c implementation | ||
| /// # impl dvcdbg::compat::I2cCompat for MyI2c { | ||
| /// # type Error = dvcdbg::error::ErrorKind; | ||
| /// # fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error> { Ok(()) } | ||
| /// # fn read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Self::Error> { Ok(()) } | ||
| /// # fn write_read(&mut self, addr: u8, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Self::Error> { Ok(()) } | ||
| /// # } | ||
| /// # struct MySerial; // Dummy Serial implementation | ||
| /// # impl core::fmt::Write for MySerial { | ||
| /// # fn write_str(&mut self, s: &str) -> core::fmt::Result { Ok(()) } | ||
| /// # } | ||
| /// | ||
| /// let mut i2c = /* your I2C instance */; | ||
| /// let mut serial = /* your serial instance */; | ||
| /// let init_sequence = [0u8; 16]; // Example initial sequence | ||
| /// const EXPLORER_CAP: usize = 32; | ||
| /// const BUF_CAP: usize = 128; | ||
| /// let explorer = Explorer::<EXPLORER_CAP> { sequence: &[] }; // Dummy explorer | ||
| /// | ||
| /// run_explorer::<_, _, EXPLORER_CAP, BUF_CAP>( | ||
| /// &explorer, | ||
| /// &mut i2c, | ||
| /// &mut serial, | ||
| /// &init_sequence, | ||
| /// 0x00, // Example prefix | ||
| /// LogLevel::Verbose, | ||
| /// ).unwrap(); | ||
| /// # Ok::<(), dvcdbg::explorer::ExplorerError>(()) | ||
| /// # } | ||
| /// ``` | ||
|
|
||
| /// Runs the I2C explorer to find and execute a single valid command sequence. | ||
| /// | ||
| /// This function first obtains one topological sort of the commands from the explorer. | ||
| /// Then, it attempts to execute this single sequence on a specified I2C address. | ||
| /// This is useful for device initialization where only one valid sequence is needed, | ||
| /// avoiding the high computational cost of exploring all permutations. | ||
| /// | ||
| /// # Type Parameters | ||
| /// | ||
| /// - `I2C`: The I2C interface type that implements `crate::compat::I2cCompat`. | ||
| /// - `S`: The serial interface type used for logging, implementing `core::fmt::Write`. | ||
| - `N`: A const generic for the maximum number of commands. | ||
| - `BUF_CAP`: A const generic for the command buffer capacity. | ||
|
|
||
| ### Parameters | ||
|
|
||
| - `explorer`: An instance of `Explorer` containing the command nodes and their dependencies. | ||
| - `i2c`: The I2C bus instance. | ||
| - `serial`: The serial writer for logging. | ||
| - `target_addr`: The specific I2C address to execute the sequence on. | ||
| - `prefix`: A byte to prepend to every command sent during execution. | ||
| - `log_level`: The desired logging level. | ||
|
|
||
| ### Returns | ||
|
|
||
| Returns `Ok(())` if the sequence was successfully executed, | ||
| or `Err(ExplorerError)` if an error occurred (e.g., cycle detected, execution failed). No newline at end of file |
There was a problem hiding this comment.
The documentation from line 106 onwards appears to be raw Rust doc comments copied directly into the markdown file. This makes the documentation difficult to read. Please reformat this section into proper markdown, for example by using headers for function names and code blocks for signatures.
|
@gemini-code-assist |
|
Hi @p14c31355, I understand you're encountering garbled characters in your test output on the actual device. This can be a tricky issue to debug, as it often stems from various layers, from hardware to software configuration. From the library's perspective, the recent changes primarily involved refactoring the logging mechanism to use a generic buffer capacity ( Here are some areas to investigate that might be contributing to the garbled characters:
I recommend starting by meticulously checking your serial port settings on both your device's firmware and your host's serial monitor. If the issue persists, try increasing the |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the I2C explorer module, adding a new "pruning" exploration strategy to find valid command sequences by iteratively removing commands that fail. This is a valuable enhancement. The changes include moving documentation to a dedicated file, making the logger and executor more flexible with const generics, and improving the I2C scanning logic. The refactoring is generally well-executed, but there are several issues to address, including outdated documentation, a few bugs in the new logic, and some dead code. Overall, this is a great step forward for the library's capabilities.
| if visited_count != len { | ||
| return Err(ExplorerError::DependencyCycle); | ||
| } |
There was a problem hiding this comment.
The cycle detection logic is incorrect when failed_nodes are present. visited_count correctly tracks the number of non-failed nodes that can be topologically sorted. However, it's being compared against len, the total number of commands, including failed ones. This will cause the function to incorrectly report a dependency cycle if any nodes have failed. The comparison should be against the number of non-failed nodes.
if visited_count != len - failed_nodes.iter().filter(|&&f| f).count() {
return Err(ExplorerError::DependencyCycle);
}| let mut commands_found = 0; | ||
| loop { | ||
| let (sequence_bytes, sequence_len) = | ||
| match explorer.get_one_topological_sort_buf::<MAX_CMD_LEN>(&mut serial_logger, &failed_nodes) { | ||
| Ok(seq) => seq, | ||
| Err(e) => { | ||
| if commands_found == explorer.sequence.len() { | ||
| serial_logger.log_info("[explorer] All commands successfully executed."); | ||
| return Ok(()); |
There was a problem hiding this comment.
The logic for commands_found in run_pruned_explorer seems incorrect. It's incremented by explorer.sequence.len() every time a full sequence succeeds for a device. This means if multiple devices work with the same sequence, commands_found can become a multiple of the sequence length. The check if commands_found == explorer.sequence.len() on line 475 will then only be true if exactly one device is successfully configured. This seems like a bug. The intent of this variable and its check is unclear and should be revisited.
| # I2C Command Sequence Explorer | ||
|
|
||
| This module provides an algorithm and supporting utilities for discovering | ||
| valid sequences of I2C commands when device dependencies are unknown or | ||
| partially specified. It is designed for embedded bring-up scenarios, where | ||
| experimenting with permutations of initialization commands can reveal the | ||
| correct sequence for a new or undocumented device. | ||
|
|
||
| ## Overview | ||
| - [`Explorer`] manages a dependency graph of commands and produces only | ||
| valid permutations that satisfy the declared constraints. | ||
| - [`PermutationIter`] generates these permutations using an iterative, | ||
| stack-safe backtracking algorithm (no recursion). | ||
| - [`CmdExecutor`] abstracts the execution of a command on the I2C bus. | ||
| - [`Logger`] provides pluggable logging backends (serial console, null logger, etc.). | ||
|
|
||
| ## Key Features | ||
| 1. **Separation of Concerns**: The permutation engine (`PermutationIter`) is | ||
| isolated from bus execution logic (`explore`). | ||
| 2. **Iterator-based API**: The [`Explorer::permutations`] method yields an | ||
| iterator, making the algorithm testable and composable. | ||
| 3. **Generic Capacity**: The const generic `N` defines the maximum command | ||
| capacity at compile time, enabling efficient use on resource-constrained | ||
| microcontrollers. | ||
| 4. **Flexible Logging**: The [`Logger`] trait supports both formatted and | ||
| lightweight logging without allocating large buffers. | ||
| 5. **Robust Error Handling**: [`ExplorerError`] reports dependency cycles, | ||
| buffer exhaustion, and runtime execution issues explicitly. | ||
|
|
||
| ## Typical Use Case | ||
| - Bring-up of a new I2C peripheral when the required initialization sequence | ||
| is undocumented or incomplete. | ||
| - Automated discovery of valid command orderings under dependency constraints. | ||
| - Filtering of device addresses that respond consistently to a tested sequence. | ||
|
|
||
| ## Example | ||
| ```ignore | ||
| use dvcdbg::prelude::*; | ||
| use heapless::Vec; | ||
| // Example executor for a specific I2C implementation. | ||
| struct MyExecutor; | ||
| impl<I2C: crate::compat::I2cCompat> CmdExecutor<I2C> for MyExecutor { | ||
| fn exec( | ||
| &mut self, | ||
| i2c: &mut I2C, | ||
| addr: u8, | ||
| cmd: &[u8] | ||
| ) -> Result<(), crate::explorer::ExecutorError> { | ||
| // Simplified example: prepend 0x00 control byte. | ||
| if cmd.len() != 1 { | ||
| return Err(crate::explorer::ExecutorError::ExecFailed); | ||
| } | ||
| let buf = [0x00, cmd[0]]; | ||
| i2c.write(addr, &buf) | ||
| .map_err(|e| crate::explorer::ExecutorError::I2cError(e.to_compat(Some(addr)))) | ||
| } | ||
| } | ||
|
|
||
| // Dummy logger that ignores messages. | ||
| struct NullLogger; | ||
| impl Logger for NullLogger { | ||
| fn log_info(&mut self, _msg: &str) {} | ||
| fn log_warning(&mut self, _msg: &str) {} | ||
| fn log_error(&mut self, _msg: &str) {} | ||
| fn log_info_fmt<F>(&mut self, _fmt: F) | ||
| where F: FnOnce(&mut heapless::String<{ crate::explorer::LOG_BUFFER_CAPACITY }>) -> Result<(), core::fmt::Error> {} | ||
| fn log_error_fmt<F>(&mut self, _fmt: F) | ||
| where F: FnOnce(&mut heapless::String<{ crate::explorer::LOG_BUFFER_CAPACITY }>) -> Result<(), core::fmt::Error> {} | ||
| } | ||
|
|
||
| // Define candidate commands with dependencies. | ||
| const CAPACITY: usize = 32; | ||
| let cmds = &[ | ||
| CmdNode { bytes: &[0x01], deps: &[] }, | ||
| CmdNode { bytes: &[0x02], deps: &[0] }, // depends on first command | ||
| CmdNode { bytes: &[0x03], deps: &[0] }, | ||
| ]; | ||
|
|
||
| let explorer = Explorer::<CAPACITY> { sequence: cmds }; | ||
| let mut executor = MyExecutor; | ||
| let mut logger = NullLogger; | ||
| let mut i2c = /* platform-specific I2C impl */; | ||
| let result = explorer.explore(&mut i2c, &mut executor, &mut logger); | ||
| if let Err(e) = result { | ||
| logger.log_error(&format!("Exploration failed: {:?}", e)); | ||
| } | ||
| ``` | ||
|
|
||
| ### `CmdNode` | ||
| Represents a single I2C command in the dependency graph. | ||
|
|
||
| - `bytes` - The I2C command bytes to be sent. | ||
| - `deps` - The indices of the commands that must precede this command. | ||
|
|
||
| The dependency is now on the index of the dependent command in the sequence. | ||
|
|
||
| ```rust | ||
| #[derive(Copy, Clone)] | ||
| pub struct CmdNode { | ||
| pub bytes: &'static [u8], | ||
| pub deps: &'static [usize], | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
| Returns a stack-safe iterator for all valid command permutations (topological sorts). | ||
|
|
||
| This function first performs cycle detection using a modified Kahn's algorithm. | ||
| If a cycle is detected, it returns an `ExplorerError::DependencyCycle`. | ||
| Otherwise, it initializes and returns a `PermutationIter` to generate all valid permutations. | ||
|
|
||
| Explores valid sequences, attempting to execute them on an I2C bus. | ||
|
|
||
| This function iterates through all valid command permutations generated by `PermutationIter`, | ||
| and for each permutation, attempts to execute it on all active I2C addresses. | ||
| It filters out addresses that fail to respond to any command in a sequence. | ||
|
|
||
| # Parameters | ||
| - `i2c`: An I2C implementation used to test candidate sequences against device addresses. | ||
| - `executor`: The object responsible for executing a single command on the bus. | ||
| - `logger`: The object responsible for logging progress and results. | ||
|
|
||
| # Returns | ||
| - `Ok(ExploreResult)` containing the list of found addresses and the number of permutations tested. | ||
| - `Err(ExplorerError)` if an error occurs during permutation generation or I2C execution. | ||
|
|
||
| Generates a single valid topological sort of the command sequence. | ||
| This is useful when only one valid ordering is needed, and avoids | ||
| the computational cost of generating all permutations. | ||
|
|
||
| Returns `Ok(Vec<&'a [u8], N>)` containing one valid command sequence, | ||
| or `Err(ExplorerError)` if a cycle is detected or buffer overflows. | ||
|
|
||
| Attempts to extend the current partial permutation by adding a new command. | ||
|
|
||
| It iterates through all available (not yet used) commands and checks if their | ||
| dependencies are satisfied (i.e., their in-degree is 0). If a valid command | ||
| is found, it's added to the current permutation, its `used` status is updated, | ||
| and the in-degrees of its dependent nodes are decremented. | ||
|
|
||
| Returns `true` if a command was successfully added, `false` otherwise. | ||
|
|
||
| Backtracks to the previous decision point in the permutation search. | ||
|
|
||
| This method undoes the last choice made: it removes the last added command | ||
| from the current permutation, unmarks it as used, and increments the | ||
| in-degrees of its dependent nodes (reversing the decrement). | ||
| It then updates the `loop_start_indices` to ensure the next search | ||
| at the parent level continues from the next sibling. | ||
|
|
||
| Returns `true` if backtracking can continue (i.e., there are more options | ||
| at a previous level), or `false` if the root was reached and no more | ||
| permutations can be generated. | ||
|
|
||
| Runs the I2C explorer with a given initial sequence and logs the results. | ||
|
|
||
| This function first performs an I2C scan with the provided `init_sequence` to identify | ||
| responsive commands. Then, it uses the `explorer` to find valid command sequences | ||
| for discovered devices, applying a `prefix` to each command. | ||
|
|
||
| # Type Parameters | ||
| - `I2C`: The I2C interface type that implements `crate::compat::I2cCompat`. | ||
| - `S`: The serial interface type used for logging, implementing `core::fmt::Write`. | ||
| - `N`: A const generic for the maximum number of commands. | ||
| - `BUF_CAP`: A const generic for the command buffer capacity. | ||
|
|
||
| # Parameters | ||
| - `explorer`: An instance of `Explorer` containing the command nodes and their dependencies. | ||
| - `i2c`: The I2C bus instance. | ||
| - `serial`: The serial writer for logging. | ||
| - `init_sequence`: The initial sequence of bytes to test for device responsiveness. | ||
| - `prefix`: A byte to prepend to every command sent during exploration. | ||
| - `log_level`: The desired logging level. | ||
|
|
||
| # Example | ||
| ```ignore | ||
| use dvcdbg::prelude::*; | ||
| use arduino_hal::I2c; | ||
| use arduino_hal::hal::port::Port; | ||
| use arduino_hal::pac::TWI; | ||
| use heapless::Vec; | ||
| use core::fmt::Write; | ||
|
|
||
| # struct MyI2c; // Dummy I2c implementation | ||
| # impl dvcdbg::compat::I2cCompat for MyI2c { | ||
| # type Error = dvcdbg::error::ErrorKind; | ||
| # fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error> { Ok(()) } | ||
| # fn read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Self::Error> { Ok(()) } | ||
| # fn write_read(&mut self, addr: u8, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Self::Error> { Ok(()) } | ||
| # } | ||
| # struct MySerial; // Dummy Serial implementation | ||
| # impl core::fmt::Write for MySerial { | ||
| # fn write_str(&mut self, s: &str) -> core::fmt::Result { Ok(()) } | ||
| # } | ||
|
|
||
| let mut i2c = /* your I2C instance */; | ||
| let mut serial = /* your serial instance */; | ||
| let init_sequence = [0u8; 16]; // Example initial sequence | ||
| const EXPLORER_CAP: usize = 32; | ||
| const BUF_CAP: usize = 128; | ||
| let explorer = Explorer::<EXPLORER_CAP> { sequence: &[] }; // Dummy explorer | ||
|
|
||
| run_explorer::<_, _, EXPLORER_CAP, BUF_CAP>( | ||
| &explorer, | ||
| &mut i2c, | ||
| &mut serial, | ||
| &init_sequence, | ||
| 0x00, // Example prefix | ||
| LogLevel::Verbose, | ||
| ).unwrap(); | ||
| # Ok::<(), dvcdbg::explorer::ExplorerError>(()) | ||
| # } | ||
| ``` | ||
|
|
||
| Runs the I2C explorer to find and execute a single valid command sequence. | ||
|
|
||
| This function first obtains one topological sort of the commands from the explorer. | ||
| Then, it attempts to execute this single sequence on a specified I2C address. | ||
| This is useful for device initialization where only one valid sequence is needed, | ||
| avoiding the high computational cost of exploring all permutations. | ||
|
|
||
| # Type Parameters | ||
| - `I2C`: The I2C interface type that implements `crate::compat::I2cCompat`. | ||
| - `S`: The serial interface type used for logging, implementing `core::fmt::Write`. | ||
| - `N`: A const generic for the maximum number of commands. | ||
| - `BUF_CAP`: A const generic for the command buffer capacity. | ||
|
|
||
| ### Parameters | ||
|
|
||
| - `explorer`: An instance of `Explorer` containing the command nodes and their dependencies. | ||
| - `i2c`: The I2C bus instance. | ||
| - `serial`: The serial writer for logging. | ||
| - `target_addr`: The specific I2C address to execute the sequence on. | ||
| - `prefix`: A byte to prepend to every command sent during execution. | ||
| - `log_level`: The desired logging level. | ||
|
|
||
| ### Returns | ||
|
|
||
| Returns `Ok(())` if the sequence was successfully executed, | ||
| or `Err(ExplorerError)` if an error occurred (e.g., cycle detected, execution failed). No newline at end of file |
There was a problem hiding this comment.
The documentation in this file is a great addition, but it seems to be out of sync with the latest refactoring. Several examples and descriptions refer to old APIs that have been changed or removed. For instance:
- The
CmdExecutorandLoggerexamples don't reflect that these traits are now generic over a buffer capacity (const BUF_CAP: usizeandconst B: usize). - The
Explorer::exploremethod call example is outdated. - The file documents private methods of
PermutationIter(try_extend,backtrack), which might not be necessary for a usage guide. - The documentation for
Explorer::permutationsis still present, but this method has been replaced byPermutationIter::new.
Please review and update the entire document to match the new API.
| let mut visited_nodes: Vec<bool, N> = Vec::new(); | ||
| visited_nodes | ||
| .resize(self.sequence.len(), false) | ||
| .map_err(|_| ExplorerError::BufferOverflow)?; |
There was a problem hiding this comment.
The visited_nodes variable is initialized but never used within the explore function. This appears to be dead code and should be removed.
| pub fn get_one_topological_sort_buf<const MAX_CMD_LEN: usize>( | ||
| &self, | ||
| serial: &mut impl core::fmt::Write, | ||
| ) -> Result<([[u8; MAX_CMD_LEN]; N], [usize; N]), ExplorerError> { | ||
| _serial: &mut impl core::fmt::Write, | ||
| failed_nodes: &[bool; N], | ||
| ) -> Result<(heapless::Vec<heapless::Vec<u8, MAX_CMD_LEN>, N>, heapless::Vec<usize, N>), ExplorerError> { |
There was a problem hiding this comment.
The _serial parameter in get_one_topological_sort_buf is no longer used. It should be removed from the function signature to clean up the API.
pub fn get_one_topological_sort_buf<const MAX_CMD_LEN: usize>(
&self,
failed_nodes: &[bool; N],
) -> Result<(heapless::Vec<heapless::Vec<u8, MAX_CMD_LEN>, N>, heapless::Vec<usize, N>), ExplorerError> {| $crate::logger::LogLevel::Normal => { | ||
| let _ = writeln!(serial, "[ok] Found devices at:"); | ||
| for addr in found_addrs { | ||
| let _ = writeln!(serial, " 0x{:02X}", addr); | ||
| write!(serial, " ").ok(); | ||
| $crate::compat::ascii::write_bytes_hex_fmt(serial, &[*addr]).ok(); | ||
| writeln!(serial, "").ok(); | ||
| } | ||
| let _ = writeln!(serial); | ||
| writeln!(serial).ok(); | ||
| } |
There was a problem hiding this comment.
In the scan_i2c macro expansion, the log output for LogLevel::Normal seems to have lost its header. Previously, it would print [ok] Found devices at:, but now it just lists the addresses. This appears to be an unintentional regression.
| $crate::logger::LogLevel::Normal => { | |
| let _ = writeln!(serial, "[ok] Found devices at:"); | |
| for addr in found_addrs { | |
| let _ = writeln!(serial, " 0x{:02X}", addr); | |
| write!(serial, " ").ok(); | |
| $crate::compat::ascii::write_bytes_hex_fmt(serial, &[*addr]).ok(); | |
| writeln!(serial, "").ok(); | |
| } | |
| let _ = writeln!(serial); | |
| writeln!(serial).ok(); | |
| } | |
| $crate::logger::LogLevel::Normal => { | |
| writeln!(serial, "[ok] Found devices at:").ok(); | |
| for addr in found_addrs { | |
| write!(serial, " ").ok(); | |
| $crate::compat::ascii::write_bytes_hex_fmt(serial, &[*addr]).ok(); | |
| writeln!(serial, "").ok(); | |
| } | |
| writeln!(serial).ok(); | |
| } |
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 significant refactoring to the I2C explorer, aiming to more flexibly discover correct command sequences by handling partial sequences. A new run_pruned_explorer function implements this new logic, and documentation has been moved to a dedicated USAGE_EXPLORE.md file. The changes are a good step forward, but I've found a few issues. The new documentation is out of sync with the code changes, there's some dead code in the explore function, and the termination logic in the new run_pruned_explorer appears to be flawed. Please see my detailed comments.
| Err(e) => { | ||
| if commands_found == explorer.sequence.len() { | ||
| serial_logger.log_info("[explorer] All commands successfully executed."); | ||
| return Ok(()); | ||
| } else { | ||
| serial_logger.log_error_fmt(|buf| { | ||
| writeln!(buf, "[error] Failed to generate a new topological sort. Aborting.") | ||
| }); | ||
| return Err(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The termination logic here seems incorrect. The commands_found variable is incremented by the entire sequence length for each device that successfully completes a sequence. This means the check if commands_found == explorer.sequence.len() will likely only pass if exactly one device succeeds. This could cause the loop to terminate incorrectly or not at all when multiple devices are present. The logic for tracking progress and determining when to exit needs to be reconsidered.
| ## Example | ||
| ```ignore | ||
| use dvcdbg::prelude::*; | ||
| use heapless::Vec; | ||
| // Example executor for a specific I2C implementation. | ||
| struct MyExecutor; | ||
| impl<I2C: crate::compat::I2cCompat> CmdExecutor<I2C> for MyExecutor { | ||
| fn exec( | ||
| &mut self, | ||
| i2c: &mut I2C, | ||
| addr: u8, | ||
| cmd: &[u8] | ||
| ) -> Result<(), crate::explorer::ExecutorError> { | ||
| // Simplified example: prepend 0x00 control byte. | ||
| if cmd.len() != 1 { | ||
| return Err(crate::explorer::ExecutorError::ExecFailed); | ||
| } | ||
| let buf = [0x00, cmd[0]]; | ||
| i2c.write(addr, &buf) | ||
| .map_err(|e| crate::explorer::ExecutorError::I2cError(e.to_compat(Some(addr)))) | ||
| } | ||
| } | ||
|
|
||
| // Dummy logger that ignores messages. | ||
| struct NullLogger; | ||
| impl Logger for NullLogger { | ||
| fn log_info(&mut self, _msg: &str) {} | ||
| fn log_warning(&mut self, _msg: &str) {} | ||
| fn log_error(&mut self, _msg: &str) {} | ||
| fn log_info_fmt<F>(&mut self, _fmt: F) | ||
| where F: FnOnce(&mut heapless::String<{ crate::explorer::LOG_BUFFER_CAPACITY }>) -> Result<(), core::fmt::Error> {} | ||
| fn log_error_fmt<F>(&mut self, _fmt: F) | ||
| where F: FnOnce(&mut heapless::String<{ crate::explorer::LOG_BUFFER_CAPACITY }>) -> Result<(), core::fmt::Error> {} | ||
| } | ||
|
|
||
| // Define candidate commands with dependencies. | ||
| const CAPACITY: usize = 32; | ||
| let cmds = &[ | ||
| CmdNode { bytes: &[0x01], deps: &[] }, | ||
| CmdNode { bytes: &[0x02], deps: &[0] }, // depends on first command | ||
| CmdNode { bytes: &[0x03], deps: &[0] }, | ||
| ]; | ||
|
|
||
| let explorer = Explorer::<CAPACITY> { sequence: cmds }; | ||
| let mut executor = MyExecutor; | ||
| let mut logger = NullLogger; | ||
| let mut i2c = /* platform-specific I2C impl */; | ||
| let result = explorer.explore(&mut i2c, &mut executor, &mut logger); | ||
| if let Err(e) = result { | ||
| logger.log_error(&format!("Exploration failed: {:?}", e)); | ||
| } | ||
| ``` | ||
|
|
||
| ### `CmdNode` | ||
| Represents a single I2C command in the dependency graph. | ||
|
|
||
| - `bytes` - The I2C command bytes to be sent. | ||
| - `deps` - The indices of the commands that must precede this command. | ||
|
|
||
| The dependency is now on the index of the dependent command in the sequence. | ||
|
|
||
| ```rust | ||
| #[derive(Copy, Clone)] | ||
| pub struct CmdNode { | ||
| pub bytes: &'static [u8], | ||
| pub deps: &'static [usize], | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
| Returns a stack-safe iterator for all valid command permutations (topological sorts). | ||
|
|
||
| This function first performs cycle detection using a modified Kahn's algorithm. | ||
| If a cycle is detected, it returns an `ExplorerError::DependencyCycle`. | ||
| Otherwise, it initializes and returns a `PermutationIter` to generate all valid permutations. | ||
|
|
There was a problem hiding this comment.
The documentation in this file appears to be out of sync with the recent code changes, which could confuse users. For example:
- The
CmdExecutorandLoggerexamples are missing the newconstgeneric parameters for buffer capacity. - The documentation describes
Explorer::permutations, but this method has been removed and its logic is now inPermutationIter::new.
Please review the entire document and update it to reflect the current API.
| let mut visited_nodes: Vec<bool, N> = Vec::new(); | ||
| visited_nodes | ||
| .resize(self.sequence.len(), false) | ||
| .map_err(|_| ExplorerError::BufferOverflow)?; |
There was a problem hiding this comment.
The visited_nodes variable is initialized here but is not used anywhere within the explore function. This adds unnecessary code and a small allocation overhead. It should be removed.
1 participant
🚀 Pull Request
Overview
Change details
Others
If you have any suggestions for a better approach than recording the "half-correct" initialization sequence of a device, please let us know.