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 focuses on enhancing the explorer module by introducing new macros that simplify the definition and initialization of command sequences and explorers, making the API more user-friendly. A key improvement for embedded environments is the optimization of the topological sort algorithm, which now avoids dynamic memory allocations by utilizing fixed-size arrays. These changes collectively aim to improve code maintainability, performance, and resource management within the exploration framework.
Highlights
- New Macro Definitions: Introduced
nodes!,new_explorer!,factorial_sort!,pruning_sort!, andget_one_sort!macros to simplify API usage and enable compile-time constant evaluation forExplorerinitialization and command execution flows. - Performance Optimization for Embedded Systems: Refactored the topological sort algorithm in
get_one_sort(formerlyget_one_topological_sort_buf) to replaceheapless::Vecwith fixed-size arrays for graph representation and queue management. This significantly reduces dynamic memory allocations, improving performance and predictability on embedded targets. - API Refinements and Consistency: Renamed
execute_and_log_commandtoexec_loggerand updatedExplorerstruct to usenodesinstead ofsequencefor better clarity. Explorer runner functions were also updated to integrate with the new macros and internalize I2C address scanning, streamlining their usage.
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 refactors the explorer logic to use compile-time constants and macros for defining command nodes, which is a great improvement for embedded systems. It also optimizes the topological sort algorithm. My review focuses on a few critical issues that will prevent compilation, such as unbound lifetimes and incorrect macro definitions. I've also pointed out some areas for improving code clarity and consistency, like renaming variables and functions, and standardizing error logging. Overall, these are solid improvements, and with a few fixes, this will be a great addition.
src/explore/runner.rs
Outdated
| ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr) => { | ||
| factorial_explorer::< | ||
| _, _, N, INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE | ||
| >($explorer, $i2c, $serial, $prefix, $init_sequence) |
There was a problem hiding this comment.
The const generic parameters N, INIT_SEQUENCE_LEN, and CMD_BUFFER_SIZE are used here but are not captured from the macro's arguments. This will cause a compilation error as they are undefined in the macro's expansion context. These values should be passed as arguments to the macro.
($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr, $N:expr, $INIT_LEN:expr, $CMD_BUF:expr) => {
factorial_explorer::<_, _, {$N}, {$INIT_LEN}, {$CMD_BUF}>($explorer, $i2c, $serial, $prefix, $init_sequence)
}
src/explore/runner.rs
Outdated
| ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr) => { | ||
| pruning_explorer::< | ||
| _, _, N, INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE | ||
| >($explorer, $i2c, $serial, $prefix, $init_sequence) |
There was a problem hiding this comment.
The const generic parameters N, INIT_SEQUENCE_LEN, and CMD_BUFFER_SIZE are used here but are not captured from the macro's arguments. This will cause a compilation error as they are undefined in the macro's expansion context. These values should be passed as arguments to the macro.
($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $init_sequence:expr, $N:expr, $INIT_LEN:expr, $CMD_BUF:expr) => {
pruning_explorer::<_, _, {$N}, {$INIT_LEN}, {$CMD_BUF}>($explorer, $i2c, $serial, $prefix, $init_sequence)
}| macro_rules! get_one_sort { | ||
| ($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr) => { | ||
| one_topological_explorer::< | ||
| _, _, INIT_SEQUENCE_LEN, INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE | ||
| >($explorer, $i2c, $serial, $prefix) | ||
| }; |
There was a problem hiding this comment.
This macro has two issues. First, like the other new macros, it uses uncaptured const generic parameters (INIT_SEQUENCE_LEN, CMD_BUFFER_SIZE). Second, it incorrectly uses INIT_SEQUENCE_LEN for the N const generic parameter of one_topological_explorer. N should be the number of nodes in the explorer, not the length of the init sequence. This will likely lead to incorrect behavior or compilation errors.
macro_rules! get_one_sort {
($explorer:expr, $i2c:expr, $serial:expr, $prefix:expr, $N:expr, $INIT_LEN:expr, $CMD_BUF:expr) => {
one_topological_explorer::<_, _, {$N}, {$INIT_LEN}, {$CMD_BUF}>($explorer, $i2c, $serial, $prefix)
};
}
src/explore/explorer.rs
Outdated
| } | ||
|
|
||
| pub fn execute_and_log_command<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( | ||
| pub fn exec_logger<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( |
There was a problem hiding this comment.
The function execute_and_log_command was renamed to exec_logger. While shorter, exec_logger is less descriptive and its purpose is not immediately clear. Consider a more descriptive name that reflects its action, such as execute_command_and_log, or reverting to the original name. Clear naming is important for maintainability.
| pub fn exec_logger<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( | |
| pub fn execute_command_and_log<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( |
src/explore/runner.rs
Outdated
| let mut target_addr = match crate::scanner::scan_i2c(i2c, serial, prefix) { | ||
| Ok(addr) => addr, |
There was a problem hiding this comment.
The variable found_addrs (plural) was renamed to target_addr (singular), but it holds a heapless::Vec of addresses from scan_i2c, which can contain multiple addresses. This is misleading. Consider renaming it to target_addrs (plural) for clarity. Also, in the match arm, addr should be addrs to reflect that it's a collection.
let mut target_addrs = match crate::scanner::scan_i2c(i2c, serial, prefix) {
Ok(addrs) => addrs,
src/explore/runner.rs
Outdated
| let target_addr = match crate::scanner::scan_i2c(i2c, serial, prefix) { | ||
| Ok(addr) => addr, | ||
| Err(e) => { | ||
| util::prevent_garbled(serial, format_args!("[error] Failed to scan I2C: {e}")); |
There was a problem hiding this comment.
The error logging here uses format_args!("[error] Failed to scan I2C: {e}"), which uses the Display format for the error. However, other similar error logs in this file use the Debug format ({e:?}). The Debug format for ErrorKind is more informative. For consistency and better diagnostics, please use the Debug format here as well.
util::prevent_garbled(serial, format_args!("[error] Failed to scan I2C: {e:?}"));
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements, including macros for easier API usage and significant performance optimizations in the topological sort algorithm. The changes make the library more ergonomic and efficient for embedded environments. I've identified a critical bug in the cycle detection logic that needs to be addressed, along with a couple of minor suggestions to improve code clarity and consistency.
src/explore/explorer.rs
Outdated
| } | ||
|
|
||
| pub fn execute_and_log_command<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( | ||
| pub fn exec_logger<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( |
There was a problem hiding this comment.
The function name exec_logger is a bit ambiguous. A more descriptive name like execute_and_log_node or exec_and_log_cmd would better reflect its purpose of executing a command node and logging the result. The previous name execute_and_log_command was also clear.
| pub fn exec_logger<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( | |
| pub fn execute_and_log_node<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( |
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 significant and valuable refactoring. The move to 'static lifetimes for command nodes via the new nodes! macro simplifies the API and improves efficiency. The topological sort implementation has been impressively optimized using bitmasks and stack-based data structures, which is a great fit for embedded systems. My review focuses on adding a critical safety check to prevent a panic with the new optimizations and improving function names for better long-term maintainability.
| pub fn get_one_sort( | ||
| &self, | ||
| _writer: &mut impl core::fmt::Write, | ||
| failed_nodes: &[bool; N], | ||
| ) -> Result<(heapless::Vec<&'a [u8], N>, heapless::Vec<u8, N>), ExplorerError> { | ||
| let len = self.sequence.len(); | ||
| let mut in_degree: heapless::Vec<u8, N> = heapless::Vec::new(); | ||
| in_degree | ||
| .resize(len, 0) | ||
| .map_err(|_| ExplorerError::BufferOverflow)?; | ||
| let mut adj_list_rev: heapless::Vec<heapless::Vec<u8, N>, N> = heapless::Vec::new(); | ||
| adj_list_rev | ||
| .resize(len, heapless::Vec::new()) | ||
| .map_err(|_| ExplorerError::BufferOverflow)?; | ||
|
|
||
| for (i, node) in self.sequence.iter().enumerate() { | ||
| ) -> Result<(heapless::Vec<&'static [u8], N>, heapless::Vec<u8, N>), ExplorerError> { | ||
| let len = self.nodes.len(); |
There was a problem hiding this comment.
This function has a couple of issues:
- (High severity) It uses a
u128bitmask for the adjacency list, which limitsNto 128. IfN > 128, the bit shift on line 395 will panic. A compile-timeassert!should be added to enforce this limit, similar to what's done inPermutationIter::new. - (Medium severity) The name
get_one_sortis less descriptive than the originalget_one_topological_sort_buf. A more descriptive name likeget_one_topological_sortwould improve clarity.
The suggestion below addresses both points.
pub fn get_one_topological_sort(
&self,
_writer: &mut impl core::fmt::Write,
failed_nodes: &[bool; N],
) -> Result<(heapless::Vec<&'static [u8], N>, heapless::Vec<u8, N>), ExplorerError> {
const {
assert!(
N <= 128,
"get_one_topological_sort uses a u128 bitmask, so N cannot exceed 128"
);
};
let len = self.nodes.len();| } | ||
|
|
||
| pub fn execute_and_log_command<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( | ||
| pub fn exec_log_cmd<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( |
There was a problem hiding this comment.
The function execute_and_log_command has been renamed to exec_log_cmd. While shorter, the new name is less descriptive and could harm readability and maintainability, especially for new contributors. I'd recommend sticking with the more explicit name execute_and_log_command for clarity.
| pub fn exec_log_cmd<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( | |
| pub fn execute_and_log_command<I2C, E, W, const MAX_BYTES_PER_CMD: usize>( |
|
@gemini-code-assist |
|
Hello p14c31355, Thank you for reaching out! I apologize if my previous comment about a "bug in the cycle detection logic" was unclear. To clarify, the The "bug" I was likely referring to was not in the detection itself, but rather in the diagnostic information provided when a cycle is found. Currently, the function simply returns To improve this, I recommend logging the indices of the nodes that are part of, or reachable from, the detected cycle. These are typically the nodes whose in-degree remains greater than zero after the topological sort queue has been exhausted. This would provide valuable context for debugging. Here's a code suggestion to add this diagnostic logging within the // Cycle detected
writeln!(
_writer,
"[error] Dependency cycle detected. Nodes involved (or reachable from cycle):"
)
.ok();
for i in 0..len {
// A node is part of a cycle if its in-degree is still > 0 after the topological sort
// and it wasn't already marked as failed.
if in_degree[i] > 0 && !failed_nodes[i] {
writeln!(_writer, " - Node index: {}", i).ok();
}
}
return Err(ExplorerError::DependencyCycle);This change would be applied to Let me know if this clarifies the point and if you have any further questions! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements by moving the definition of command nodes and sequences to compile-time using macros. This is an excellent change for an embedded context, reducing runtime overhead and improving type safety. The new nodes!, factorial_sort!, and pruning_sort! macros enhance ergonomics. The refactoring of the explorer and runner modules also improves code clarity.
My review focuses on a critical logic issue in the new topological sort implementation that could lead to incorrect behavior. Otherwise, the changes are well-implemented.
src/explore/explorer.rs
Outdated
| if visited_count != len { | ||
| // Cycle detected | ||
| writeln!( | ||
| _writer, | ||
| "[error] Dependency cycle detected. Nodes involved (or reachable from cycle):" | ||
| ) | ||
| .ok(); | ||
| for i in 0..len { | ||
| // A node is part of a cycle if its in-degree is still > 0 after the topological sort | ||
| // and it wasn't already marked as failed. | ||
| if in_degree[i] > 0 && !failed_nodes[i] { | ||
| writeln!(_writer, " - Node index: {i}").ok(); | ||
| } | ||
| } | ||
| Err(ExplorerError::DependencyCycle) | ||
| } else { | ||
| Ok((result_sequence, result_len_per_node)) | ||
| } |
There was a problem hiding this comment.
The cycle detection logic is incorrect when failed_nodes contains true values. The condition if visited_count != len does not account for nodes that are intentionally skipped. This will cause the function to incorrectly report a dependency cycle whenever any node has failed, preventing the pruning explorer from working as intended.
The number of visited nodes should be compared against the number of non-failed nodes.
let non_failed_count = len - failed_nodes.iter().filter(|&&f| f).count();
if visited_count != non_failed_count {
// Cycle detected
writeln!(
_writer,
"[error] Dependency cycle detected. Nodes involved (or reachable from cycle):"
)
.ok();
for i in 0..len {
// A node is part of a cycle if its in-degree is still > 0 after the topological sort
// and it wasn't already marked as failed.
if in_degree[i] > 0 && !failed_nodes[i] {
writeln!(_writer, " - Node index: {i}").ok();
}
}
Err(ExplorerError::DependencyCycle)
} else {
Ok((result_sequence, result_len_per_node))
}
|
/gemini summary |
Summary of ChangesThis pull request introduces a set of enhancements to the exploration framework, primarily focusing on compile-time command definition and significant performance improvements in the topological sorting algorithms. These changes aim to reduce runtime overhead and simplify the process of defining and executing command sequences, making the framework more efficient and user-friendly, especially for resource-constrained environments. Highlights
Changelog
Activity
|
1 participant
🚀 Pull Request
Overview
Change details
Build / Test Results
Target board with confirmed operation