Feature/#97

[p14c31355]

🚀 Pull Request

Overview

• Related Issue: Combine explorer to scanner #97
• Response details: Implement glue function to scanner and explorer.

Change details

• New feature
• Refactoring
• Bug fix
• CI / Build settings correction
• Documentation update

Build / Test Results

$ cargo check     # ✅
$ cargo test      # ✅

---

[gemini-code-assist]

Summary of Changes

Hello @p14c31355, I'm Gemini Code Assist¹! 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 implements a new "glue" function to integrate the existing scanner and explorer modules, specifically for I2C bus operations. This integration streamlines the process of scanning for I2C devices and then applying an initialization sequence, enabling a more cohesive workflow for device setup and command execution.

Highlights

• Enhanced scan_init_sequence: The scan_init_sequence function in src/scanner.rs has been updated to accept a mutable init_sequence and now returns the detected_cmds as a Vec<u8, 64>, allowing for further processing of the scan results.
• New run_explorer Function: A new public function run_explorer has been introduced in src/scanner.rs. This function orchestrates the initial I2C bus scan using scan_init_sequence and then proceeds to apply a driver initialization sequence using the explorer module, handling potential errors.
• PrefixExecutor Implementation: A new PrefixExecutor struct and its implementation of the CmdExecutor trait have been added. This executor is designed to prepend a specified prefix byte to I2C commands before execution, which is useful for certain I2C devices that require a prefix for their commands.

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

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request introduces a run_explorer function to integrate the I2C scanner and explorer functionalities, along with a PrefixExecutor for command execution. The changes are generally good, but there are several areas for improvement regarding code clarity, correctness, and performance. My review includes suggestions to remove an unused generic parameter, clarify function signatures by removing ignored parameters, improve performance by avoiding repeated memory allocations, and adopt more idiomatic Rust patterns.

[gemini-code-assist]

The generic parameter E is declared but not used within the run_explorer function. The function unconditionally creates and uses a PrefixExecutor. The unused generic parameter makes the function signature misleading and should be removed.

pub fn run_explorer<I2C, S>(
    explorer: &crate::explorer::Explorer<'_>,
    i2c: &mut I2C,
    serial: &mut S,
    init_sequence: &mut [u8],
    prefix: u8,
    log_level: LogLevel,
) -> Result<(), crate::explorer::ExplorerError>
where
    I2C: crate::compat::I2cCompat,
    S: core::fmt::Write,
    <<> as crate::compat::I2cCompat>::Error: crate::compat::HalErrorExt,

[gemini-code-assist]

The init_sequence is being iterated over for printing purposes only, but iter_mut() is used. This suggests that the sequence is being mutated, which is not the case and can be misleading. Using iter() would be more appropriate here to signal a read-only iteration.

Suggested change

	for b in init_sequence.iter_mut() {
	let _ = writeln!(serial, " 0x{b:02X}");
	}
	for b in init_sequence.iter() {
	let _ = writeln!(serial, " 0x{b:02X}");
	}

[gemini-code-assist]

This match block can be expressed more idiomatically using Result::map and Result::map_err for handling the Ok and Err cases respectively. This can make the code more concise and easier to read.

    explorer.explore(
        i2c,
        serial,
        &mut PrefixExecutor::new(
            successful_seq.as_mut_slice(),
            prefix,
        ),
    )
    .map(|()| {
        let _ = writeln!(serial, "[driver] init sequence applied");
    })
    .map_err(|e| {
        let _ = writeln!(serial, "[error] explorer failed: {e:?}");
        e
    })

[gemini-code-assist]

The new function for PrefixExecutor takes a generic parameter T which is ignored. This is confusing for callers, who might think the argument is used when it is not. It should be removed to make the function's intent clearer. Remember to update the call site in run_explorer accordingly.

    fn new(prefix: u8) -> Self {

[gemini-code-assist]

A new heapless::Vec buffer is allocated on the stack every time exec is called. Since exec might be called frequently within the explorer's permutation loop, this is inefficient. To improve performance, consider making the buffer a field of PrefixExecutor and reusing it across calls by clearing it at the beginning of exec.

You would need to update PrefixExecutor struct and its new function:

struct PrefixExecutor {
    prefix: u8,
    buffer: heapless::Vec<>,
}

impl PrefixExecutor {
    fn new(prefix: u8) -> Self {
        Self { prefix, buffer: heapless::Vec::new() }
    }
}

Then, exec can be updated as follows:

    fn exec(&mut self, i2c: &mut I2C, addr: u8, cmd: &[u8]) -> bool {
        // This executor is a dummy for the explorer.
        self.buffer.clear();

        if self.buffer.push(self.prefix).is_err() || self.buffer.extend_from_slice(cmd).is_err() {
            return false;
        }
        i2c.write(addr, &self.buffer).is_ok()
    }

[p14c31355]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a glue function to connect the scanner and explorer modules, along with some logging improvements. The overall direction is good, but there's a significant logical issue in the new run_explorer function where the result from the scanner is calculated but then discarded, which likely indicates a bug. I've also pointed out some opportunities to improve code clarity by using immutable references where appropriate and fixing some minor formatting issues. Addressing the logical bug in run_explorer and PrefixExecutor should be the priority.

[gemini-code-assist]

There are a couple of issues in this new run_explorer function and the PrefixExecutor struct:

1. The successful_seq variable is calculated on line 303, but its value is effectively discarded. It's passed to PrefixExecutor::new, but PrefixExecutor::exec clears its buffer on every call, ignoring the initial content. This seems like a bug. The successful_seq should either be used correctly, or if it's only for logging, the scan_init_sequence function shouldn't return it.

2. The PrefixExecutor takes a buffer in its new function, which is misleading for the reason above. It should probably initialize its own empty buffer.

3. The comment // This executor is a dummy for the explorer. on line 346 is confusing. The executor performs real I2C operations and is essential for the explorer's function. It should be rephrased to be more descriptive.

Here is a suggested refactoring to address these points. Note that this leaves successful_seq as an unused variable, which you should address by either using it or removing its calculation.

pub fn run_explorer<I2C, S>(
    explorer: &crate::explorer::Explorer<'_>,
    i2c: &mut I2C,
    serial: &mut S,
    init_sequence: &mut [u8],
    prefix: u8,
    log_level: LogLevel,
) -> Result<(), crate::explorer::ExplorerError>
where
    I2C: crate::compat::I2cCompat,
    S: core::fmt::Write,
    <I2C as crate::compat::I2cCompat>::Error: crate::compat::HalErrorExt,
{
    let _ = writeln!(serial, "[log] Scanning I2C bus...");
    let successful_seq = crate::scanner::scan_init_sequence(
        i2c,
        serial,
        init_sequence,
        log_level,
    );
    // TODO: The 'successful_seq' is currently unused.
    let _ = writeln!(serial, "[scan] initial sequence scan completed");

    let _ = writeln!(serial, "[log] Start driver safe init");

    explorer.explore(
        i2c,
        serial,
        &mut PrefixExecutor::new(prefix),
    )
    .map(|()| {
        let _ = writeln!(serial, "[driver] init sequence applied");
    })
    .map_err(|e| {
        let _ = writeln!(serial, "[error] explorer failed: {e:?}");
        e
    })
}
struct PrefixExecutor {
    prefix: u8,
    buffer: heapless::Vec<u8, 64>,
}

impl PrefixExecutor {
    fn new(prefix: u8) -> Self {
        Self {
            prefix,
            buffer: heapless::Vec::new(),
        }
    }
}

impl<I2C> crate::explorer::CmdExecutor<I2C> for PrefixExecutor
where
    I2C: crate::compat::I2cCompat,
    <I2C as crate::compat::I2cCompat>::Error: crate::compat::HalErrorExt,
{
    fn exec(&mut self, i2c: &mut I2C, addr: u8, cmd: &[u8]) -> bool {
        // This executor prepends a prefix to each command.
        self.buffer.clear();

        if self.buffer.push(self.prefix).is_err() || self.buffer.extend_from_slice(cmd).is_err() {
            return false;
        }
        i2c.write(addr, &self.buffer).is_ok()
    }
}

[p14c31355]

@gemini-code-assist
Is there a good approach to ensure successful_seq is not discarded?

[gemini-code-assist]

Hi @p14c31355,

That's a great question regarding successful_seq.

Currently, the successful_seq that is passed to PrefixExecutor::new is indeed effectively discarded for its operational use within the PrefixExecutor.

Here's why:

1. When PrefixExecutor::new(prefix, successful_seq) is called, successful_seq is moved into the buffer field of the PrefixExecutor struct.
2. However, in the PrefixExecutor::exec method, the very first line is self.buffer.clear();.

This means that the contents of successful_seq are immediately cleared from the buffer every time exec is called, and the buffer is then repopulated only with the prefix and the cmd passed to exec.

To ensure successful_seq is not discarded and is utilized by the PrefixExecutor, we need to clarify its intended role. Could you elaborate on how you envision successful_seq being used by the PrefixExecutor?

For example, if successful_seq should act as a constant prefix for all commands executed by this executor (along with the prefix byte), then the PrefixExecutor struct would need to store successful_seq in a separate field, and the exec method would need to construct the command by combining successful_seq, prefix, and the cmd argument.

Let me know your thoughts on the intended behavior, and I can provide a more specific suggestion!

[p14c31355]

@gemini-code-assist
If an incorrect command is passed to the search function, the entire function may behave unintendedly.

[gemini-code-assist]

Hi @p14c31355,

Thanks for raising this important point. You're right to be concerned about how incorrect commands might lead to unintended behavior, especially in a search or exploration function.

To help me understand the specific scenario you're envisioning, could you clarify a few things?

1. What constitutes an "incorrect command" in this context? Are we talking about a command that's syntactically malformed, a command that's valid but leads to an unexpected device state, or something else?
2. Which "search function" are you referring to? Is it the explorer.explore function, the scanner::scan_init_sequence function, or another part of the code?
3. Could you describe the "unintended behavior" you anticipate? For example, would it lead to a panic, an infinite loop, incorrect device configuration, or something else?

Understanding these details will help me provide more targeted feedback and suggestions, especially in the context of how successful_seq (or any initial sequence) is handled and used by the PrefixExecutor or other parts of the system.

Looking forward to your clarification!

[p14c31355]

@gemini-code-assist
For example, if you look at the wrong datasheet and enter the wrong command, in the current implementation, scan_init_sequence will return only the correct command.
By doing so, scan_init_sequence is called as a preprocessing step before the user can search for the initialization sequence more efficiently.

[gemini-code-assist]

Hi @p14c31355,

Thanks for clarifying the role of scan_init_sequence as a preprocessing step to filter out commands that don't yield a response on the I2C bus, and your intent for the explorer to then work with this filtered set of "correct" commands.

You've accurately identified that scan_init_sequence indeed returns only the commands that were successfully detected. However, there are two crucial points in the current implementation that prevent this filtered successful_seq from being fully utilized by the explorer:

1. PrefixExecutor Discards successful_seq: As we discussed earlier (in comment 3), the successful_seq passed to PrefixExecutor::new is stored in its buffer field. However, the exec method of PrefixExecutor immediately calls self.buffer.clear() at the beginning of every execution. This means the successful_seq is effectively discarded and not used to influence the commands executed by the PrefixExecutor.

2. Explorer Uses Its Own init_sequence: The explorer.explore method operates on the self.sequence that the Explorer instance was initialized with. In the current run_explorer function (src/scanner.rs), the explorer instance is passed as an immutable reference (&crate::explorer::Explorer<'_>). This means its internal sequence cannot be updated within run_explorer with the successful_seq returned by scan_init_sequence. Therefore, the explorer will still attempt to explore permutations of the original init_sequence it was given, not the filtered successful_seq.

To align the code with your intent – where scan_init_sequence acts as a pre-filter for the explorer's search space – the Explorer API or the run_explorer function's signature would need to be modified. For example, run_explorer could take a mutable explorer reference (&mut Explorer), or Explorer could expose a method to update its internal sequence, or run_explorer could be responsible for creating and returning the Explorer instance itself after the scan.

Let me know if this clarifies the situation and if you'd like to discuss potential API changes to achieve this behavior!

[p14c31355]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable feature by adding a glue function to connect the I2C scanner and explorer. The implementation is generally well-done. I've identified a couple of areas for improvement in the new code within src/scanner.rs. My suggestions focus on enhancing the API's ergonomics and refactoring a new function for better clarity and maintainability. The changes in src/explorer.rs are positive logging improvements.

[gemini-code-assist]

The implementation of exec can be simplified for better readability and maintainability. The current approach uses self.buffer for both the initialization sequence and the explorer command, which requires clearing it within the loop and makes the logic for buffer management complex.

A cleaner approach is to handle the fixed-size initialization sequence writes using a temporary stack-allocated array. This reserves self.buffer solely for the variable-length explorer command, making the function's logic easier to follow and maintain.

        // Init sequence before exploring
        for &c in self.init_sequence.iter() {
            let command_with_prefix = [self.prefix, c];
            if i2c.write(addr, &command_with_prefix).is_err() {
                return false;
            }
        }

        // Run the explorer command
        self.buffer.clear();
        if self.buffer.push(self.prefix).is_err() || self.buffer.extend_from_slice(cmd).is_err() {
            return false;
        }

        i2c.write(addr, &self.buffer).is_ok()

[p14c31355]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new run_explorer function to connect the I2C scanner and command sequence explorer. It refactors scan_init_sequence to return detected commands and adds a PrefixExecutor to manage command execution with prefixes and initial sequences. My review includes suggestions to improve the documentation example for run_explorer for clarity and to refactor the PrefixExecutor::exec method for better readability and efficiency.

[gemini-code-assist]

The example in the run_explorer documentation has a few issues that could confuse users:

1. Explorer::new is used, but this constructor doesn't exist. It should be Explorer { sequence: &[] } or similar, based on other examples in the codebase.
2. &mut init_sequence is passed. While this coerces to &[u8], it's misleading as the sequence is not mutated. Using &init_sequence would be clearer.
3. LogLevel::Info is not a valid variant of LogLevel. It should be LogLevel::Verbose or LogLevel::Quiet.
4. The function signature of main is -> ! but the ? operator is used, which requires a Result return type.

Here is a corrected version of the example.

# use dvcdbg::prelude::*;
# use dvcdbg::explorer::ExplorerError;
# use dvcdbg::scanner::LogLevel;
# fn main() -> Result<(), ExplorerError> {
let mut i2c = /* your I2C instance */;
let mut serial = /* your serial instance */;
let init_sequence = [0u8; 16]; // Example initial sequence
let explorer = Explorer { sequence: &[] }; // Dummy explorer

run_explorer(
    &explorer,
    &mut i2c,
    &mut serial,
    &init_sequence,
    0x3C, // Example prefix
    LogLevel::Verbose,
)?;
# Ok(())
# }

[gemini-code-assist]

The implementation of exec can be simplified for better readability and to avoid unnecessary operations on self.buffer. The loop for sending the init_sequence repeatedly clears and fills self.buffer for each command. This can be made more efficient by creating the two-byte command on the stack within the loop. This avoids modifying self.buffer until it's needed for the main explorer command.

    fn exec(&mut self, i2c: &mut I2C, addr: u8, cmd: &[u8]) -> bool {
        // Init sequence before exploring
        for &c in self.init_sequence.iter() {
            let command = [self.prefix, c];
            if i2c.write(addr, &command).is_err() {
                return false;
            }
        }

        // Run the explorer command
        self.buffer.clear();
        if self.buffer.push(self.prefix).is_err() || self.buffer.extend_from_slice(cmd).is_err() {
            return false;
        }

        i2c.write(addr, &self.buffer).is_ok()
    }

[p14c31355]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new run_explorer function that integrates the scanner and explorer modules to find valid I2C device initialization sequences. The changes include adding the PrefixExecutor to manage command execution with a prefix and an initial sequence, updating function signatures to return detected commands, and improving log messages. My review focuses on improving the robustness and clarity of the new functionality. I've pointed out a documentation inaccuracy and a potential for silent failure in the PrefixExecutor that could mislead users. Overall, this is a good step towards automating device analysis.

[gemini-code-assist]

A buffer overflow here will be treated as a regular I2C failure by the Explorer, which could be misleading when debugging. The PrefixExecutor's buffer has a fixed capacity of 64 bytes, but commands from the Explorer can be of any length. If 1 + cmd.len() exceeds 64, this check will fail, and exec will return false without indicating the true cause of the failure.

To prevent this silent failure, I recommend adding a check at the beginning of run_explorer to validate command lengths against the buffer capacity and return a distinct error if a command is too long. This will provide clearer feedback to the user. For example:

// In run_explorer, before calling explorer.explore()
const PREFIX_EXECUTOR_BUF_CAP: usize = 64; // Ideally, this constant is shared with PrefixExecutor
if explorer.sequence.iter().any(|node| 1 + node.bytes.len() > PREFIX_EXECUTOR_BUF_CAP) {
    let _ = writeln!(serial, "[error] Command too long for executor buffer.");
    // You might want to add a new error variant to ExplorerError and return it here.
    return Ok(()); // Or Err(...)
}