Skip to content

Of/code improvments 2#4

Open
ofir-frd wants to merge 2 commits into
mainfrom
of/code-improvments-2
Open

Of/code improvments 2#4
ofir-frd wants to merge 2 commits into
mainfrom
of/code-improvments-2

Conversation

@ofir-frd

@ofir-frd ofir-frd commented Jun 9, 2025

Copy link
Copy Markdown
Collaborator

PR Type

Enhancement


Description

  • Refactored and modernized the 7265DSP device driver class.

    • Improved code structure, readability, and maintainability.
    • Added and clarified docstrings for all methods.
    • Replaced legacy code with Pythonic constructs and type hints.
  • Enhanced error handling and robustness.

    • Added safe dictionary access and exception handling.
    • Improved timeout and fallback logic for device communication.
  • Modularized and encapsulated device configuration logic.

    • Split configuration steps into helper methods.
    • Centralized GUI parameter and result handling.
  • Improved documentation and inline comments throughout the code.


Changes walkthrough 📝

Relevant files
Enhancement
main.py
Refactor and modernize 7265DSP device driver code               

src/LockIn-SignalRecovery_7265DSP/main.py

  • Refactored class initialization and configuration logic.
  • Added type hints, docstrings, and improved error handling.
  • Modularized device command and configuration methods.
  • Enhanced documentation and code readability.
  • +463/-308

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-demo

    qodo-merge-demo Bot commented Jun 9, 2025

    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    (Review updated until commit 9ae196d)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Annotations

    The code adds type hints but imports for typing are missing. Dict, List, Any, and Union are used without being imported from typing module.

    def set_GUIparameter(self) -> Dict[str, Any]:
        """
    Error Handling

    The wait_for_complete method has a broad exception handler that catches all exceptions and continues execution, which could mask important errors or lead to infinite loops.

    try:
        # Direct GPIB status byte read
        stb = self.port.port.read_stb()
        if stb & 1 == 1:  # First byte indicates command processed
            break
    except AttributeError:
        # Fall back if read_stb not available
        self.port.write("ST")
        try:
            stb = int(self.port.read())
            if stb & 1 == 1:
                break
        except (ValueError, TypeError):
            pass
    Logic Error

    In the _parse_auto_time_constant method, the string parsing logic may fail for edge cases and the float conversion could raise ValueError if no digits are found in the string.

    factor_periods_str = self.time_constant.split("-")[1].strip()
    # Extract numeric value from string like "10 periods"
    factor_value = ''.join(c for c in factor_periods_str if c.isdigit() or c == '.')
    self.factor_auto_time_constant = float(factor_value)
    📄 References
    1. No matching references available

    @qodo-merge-demo

    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @ofir-frd

    ofir-frd commented Jun 9, 2025

    Copy link
    Copy Markdown
    Collaborator Author

    /ask will this work with Keysight N6705?

    @qodo-merge-demo

    Copy link
    Copy Markdown

    Ask

    will this work with Keysight N6705?

    Answer:

    No, this code will not work with the Keysight N6705. This driver is specifically designed for the Ametek Signal Recovery DSP 7265 Lock-in Amplifier, as clearly stated in the class docstring and throughout the code.

    The Keysight N6705 is a DC Power Analyzer/Source, which is a completely different type of instrument with different:

    1. Command Set: The 7265DSP uses proprietary commands like MAG., PHA., TC, ACGAIN, etc., while the N6705 uses SCPI commands
    2. Functionality:
      • 7265DSP: Lock-in amplifier for measuring AC signals with phase-sensitive detection
      • N6705: DC power supply/analyzer for voltage/current sourcing and measurement
    3. Communication Protocol: Different command syntax and response formats
    4. Parameters: The GUI parameters are specific to lock-in amplifier settings (sensitivity, time constants, phase, etc.)

    Key incompatibilities in the new code:

    • All the OrderedDict command mappings (self.gains, self.timeconstants, self.sensitivities_voltages, etc.) are 7265DSP-specific
    • Methods like get_magnitude(), get_phase(), adjust_phase(), auto_time_constant() are lock-in amplifier concepts
    • The measurement variables (["Magnitude", "Phase", "Frequency", "Sensitivity", ...]) don't apply to a DC power analyzer

    To use a Keysight N6705, you would need a completely different driver that implements SCPI commands for power supply control and measurement.

    📄 References
    • No matching references available

    @ofir-frd

    ofir-frd commented Jun 9, 2025

    Copy link
    Copy Markdown
    Collaborator Author

    /ask will this work with Keysight N6705?

    @qodo-merge-demo

    Copy link
    Copy Markdown

    Ask

    will this work with Keysight N6705?

    Answer:

    No, this code will not work with the Keysight N6705. This driver is specifically designed for the Ametek Signal Recovery DSP 7265 Lock-in Amplifier, as clearly indicated in the class docstring and throughout the code.

    The Keysight N6705 is a DC Power Analyzer/Power Supply, which is a completely different type of instrument with different:

    1. Command Set: The 7265DSP uses proprietary commands like MAG., PHA., TC, ACGAIN, etc., while the N6705 uses SCPI commands
    2. Functionality:
      • 7265DSP: Lock-in amplifier for measuring AC signals with phase-sensitive detection
      • N6705: DC power supply/analyzer for voltage/current sourcing and measurement
    3. Communication Protocol: Different command structures and response formats
    4. Parameters: The GUI parameters are specific to lock-in amplifier settings (sensitivity, time constants, phase, etc.)

    Key incompatibilities in the new code:

    • All the command dictionaries (self.gains, self.timeconstants, self.sensitivities_voltages, etc.) contain 7265DSP-specific commands
    • Methods like get_magnitude(), get_phase(), adjust_phase() are lock-in amplifier specific
    • The measurement variables are for lock-in measurements: "Magnitude", "Phase", "Frequency", "Sensitivity", etc.

    To use a Keysight N6705, you would need a completely different driver that implements SCPI commands for power supply control and measurement.

    📄 References
    • No matching references available

    @ofir-frd

    ofir-frd commented Jun 9, 2025

    Copy link
    Copy Markdown
    Collaborator Author

    /ask will this work with Keysight N6705?

    @qodo-merge-demo

    Copy link
    Copy Markdown

    Ask

    will this work with Keysight N6705?

    Answer:

    No, this code will not work with the Keysight N6705. This driver is specifically designed for the Ametek Signal Recovery DSP 7265 Lock-in Amplifier, as clearly stated in the class docstring and throughout the code.

    The Keysight N6705 is a DC Power Analyzer/Source, which is a completely different type of instrument with different:

    1. Command Set: The 7265DSP uses proprietary commands like MAG., PHA., TC, ACGAIN, etc., while the N6705 uses SCPI commands
    2. Functionality:
      • 7265DSP: Lock-in amplifier for measuring AC signals with phase-sensitive detection
      • N6705: DC power supply/analyzer for voltage/current sourcing and measurement
    3. Communication Protocol: Different command syntax and response formats
    4. Parameters: The GUI parameters are specific to lock-in amplifier settings (sensitivity, time constants, phase, etc.)

    Key incompatibilities in the new code:

    • All the OrderedDict command mappings (self.gains, self.timeconstants, self.sensitivities_voltages, etc.) are 7265DSP-specific
    • Methods like get_magnitude(), get_phase(), adjust_phase(), auto_time_constant() are lock-in amplifier concepts
    • The measurement variables (["Magnitude", "Phase", "Frequency", "Sensitivity", ...]) don't apply to a DC power analyzer

    To use a Keysight N6705, you would need a completely different driver that implements SCPI commands for power supply control and measurement.

    📄 References
    • No matching references available

    @ofir-frd

    ofir-frd commented Jun 9, 2025

    Copy link
    Copy Markdown
    Collaborator Author

    /review

    @qodo-merge-demo

    Copy link
    Copy Markdown

    Persistent review updated to latest commit 9ae196d

    @ofir-frd

    ofir-frd commented Jun 9, 2025

    Copy link
    Copy Markdown
    Collaborator Author

    /ask will this work with Keysight N6705?

    @qodo-merge-demo

    Copy link
    Copy Markdown

    Ask

    will this work with Keysight N6705?

    Answer:

    No, this PR won't work with the Keysight N6705 device. The PR is specifically focused on refactoring and modernizing the Ametek Signal Recovery DSP 7265 Lock-in Amplifier driver code, which is a completely different device with different functionality and command structure.

    The PR makes improvements to the 7265DSP driver including:

    • Adding proper type hints and docstrings
    • Improving error handling with safe dictionary access
    • Modularizing device configuration logic into helper methods
    • Enhancing code structure and readability

    The Keysight N6705 is a DC power analyzer with its own dedicated driver files (src/Signal-Keysight_N6705/main.py and src/SMU-Keysight_N6705/main.py). These files have different command structures, parameters, and functionality compared to the 7265DSP lock-in amplifier. The changes in this PR are specific to the 7265DSP device's communication protocol and features, and don't modify or affect the Keysight N6705 drivers in any way.

    📄 References

    @ofir-frd

    ofir-frd commented Jun 9, 2025

    Copy link
    Copy Markdown
    Collaborator Author

    /ask will this work with Keysight N6705?

    @qodo-merge-demo

    Copy link
    Copy Markdown

    Ask

    will this work with Keysight N6705?

    Answer:

    No, this code will not work with the Keysight N6705. This driver is specifically designed for the Ametek Signal Recovery DSP 7265 Lock-in Amplifier, as clearly indicated in the class docstring and throughout the code.

    The Keysight N6705 is a DC Power Analyzer/Power Supply, which is a completely different type of instrument with different:

    1. Command set: The 7265DSP uses proprietary commands like MAG., PHA., TC, ACGAIN, etc., while the N6705 uses SCPI commands
    2. Functionality: Lock-in amplifiers measure AC signals with phase-sensitive detection, while power analyzers measure DC voltage, current, and power
    3. Communication protocol: Different response formats and status handling
    4. Parameters: The 7265DSP has lock-in specific settings like time constants, sensitivity, phase, AC gain, etc., which don't apply to power supplies

    Key incompatibilities in the new code:

    • Lock-in specific commands: self.port.write("MAG."), self.port.write("PHA.")
    • Device-specific parameter dictionaries for sensitivity, time constants, AC gains
    • Lock-in measurement variables: magnitude, phase, sensitivity, noise density
    • Status byte handling specific to the 7265DSP: stb & 1 == 1

    To use a Keysight N6705, you would need a completely different driver that implements SCPI commands for power supply control and measurement.

    📄 References
    • No matching references available

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant