Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/string inverters #86

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

Pho3niX90
Copy link
Owner

@Pho3niX90 Pho3niX90 commented Oct 30, 2024

User description

closes #84 #58 #79


PR Type

enhancement, bug fix


Description

  • Added support for string inverters, allowing configuration and sensor setup based on inverter type.
  • Introduced new sensor definitions for both hybrid and string inverters, enhancing the modularity of the code.
  • Improved error handling in the Modbus controller by logging errors and returning None instead of raising exceptions.
  • Refactored sensor, switch, and time setup to conditionally load based on inverter type, ensuring compatibility with different inverter models.

Changes walkthrough 📝

Relevant files
Enhancement
config_flow.py
Add support for string inverters in config flow                   

custom_components/solis_modbus/config_flow.py

  • Added support for string inverters in configuration validation.
  • Introduced a new configuration option for inverter type.
  • Adjusted register reading based on inverter type.
  • +13/-1   
    hybrid_sensors.py
    Define hybrid sensors for Solis inverters                               

    custom_components/solis_modbus/data/hybrid_sensors.py

  • Added a new file defining hybrid sensors.
  • Defined multiple sensor entities with specific registers and
    attributes.
  • +633/-0 
    string_sensors.py
    Define string sensors for Solis inverters                               

    custom_components/solis_modbus/data/string_sensors.py

  • Added a new file defining string sensors.
  • Defined sensor entities specific to string inverters.
  • +147/-0 
    number.py
    Skip number setup for string inverters                                     

    custom_components/solis_modbus/number.py

  • Added condition to skip setup for string inverters.
  • Checked inverter type before proceeding with setup.
  • +5/-0     
    sensor.py
    Refactor sensor setup for hybrid and string inverters       

    custom_components/solis_modbus/sensor.py

  • Refactored to load sensors based on inverter type.
  • Removed hardcoded sensor definitions and imported from new modules.
  • +14/-633
    switch.py
    Skip switch setup for string inverters                                     

    custom_components/solis_modbus/switch.py

  • Added condition to skip switch setup for string inverters.
  • Checked inverter type before proceeding with setup.
  • +5/-0     
    time.py
    Skip time setup for string inverters                                         

    custom_components/solis_modbus/time.py

  • Added condition to skip time setup for string inverters.
  • Checked inverter type before proceeding with setup.
  • +5/-0     
    Bug fix
    modbus_controller.py
    Enhance error handling in Modbus controller                           

    custom_components/solis_modbus/modbus_controller.py

  • Improved error handling by logging errors and returning None.
  • Changed log level from error to debug for connection issues.
  • +9/-5     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    … methods
    
    Modified `async_read_input_register`, `async_read_holding_register`,
    `async_write_holding_register`, and `async_write_holding_registers` to log errors
    and return None instead of raising exceptions, ensuring smoother error handling
    without interruptions in execution.
    @Pho3niX90 Pho3niX90 linked an issue Oct 30, 2024 that may be closed by this pull request
    @Pho3niX90 Pho3niX90 closed this Oct 30, 2024
    @Pho3niX90 Pho3niX90 reopened this Oct 30, 2024
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    84 - Fully compliant

    Fully compliant requirements:

    • Update support for Solis S6-GR1P4K model
    • Ensure correct Modbus addresses for the new inverter type

    58 - Fully compliant

    Fully compliant requirements:

    • Add support for non-hybrid inverters
    • Ensure error-free setup for non-hybrid inverters
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Refactoring Required
    The refactoring of sensor setup based on inverter type could be improved for maintainability and scalability. Consider abstracting the sensor setup into a separate method or class that handles different inverter types more cleanly.

    Connection Validation
    The connection validation logic in the config flow might be improved by handling different inverter types more robustly, ensuring that the correct registers are read for each type.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add logging for connection attempts to enhance system monitoring and troubleshooting

    Implement logging for successful and unsuccessful connection attempts to aid in
    debugging and monitoring the system's behavior.

    custom_components/solis_modbus/config_flow.py [40-41]

    -await modbus_controller.connect()
    -return True
    +try:
    +    await modbus_controller.connect()
    +    logging.info("Connection successful")
    +    return True
    +except ConnectionError as e:
    +    logging.error(f"Connection failed: {str(e)}")
    +    return False
    Suggestion importance[1-10]: 9

    Why: Implementing logging for connection attempts provides valuable insights for debugging and monitoring, making it easier to track successful and unsuccessful connections, which is crucial for system reliability and maintenance.

    9
    Add validation for the 'type' field to prevent processing of unexpected values

    Validate the type field in user_input to ensure it contains only expected values
    before processing to avoid potential errors or unexpected behavior.

    custom_components/solis_modbus/config_flow.py [35-38]

    -if user_input["type"] == "string":
    -    await modbus_controller.async_read_input_register(3262)
    +if user_input.get("type") in ["string", "hybrid"]:
    +    if user_input["type"] == "string":
    +        await modbus_controller.async_read_input_register(3262)
    +    else:
    +        await modbus_controller.async_read_input_register(33263)
     else:
    -    await modbus_controller.async_read_input_register(33263)
    +    # Handle unexpected type value
    +    return False
    Suggestion importance[1-10]: 7

    Why: Validating the type field in user_input ensures that only expected values are processed, reducing the risk of errors or unexpected behavior. This is a valuable enhancement for input validation.

    7
    Possible bug
    Improve error handling for register read operations to enhance robustness

    Add error handling for the async_read_input_register method to manage potential
    exceptions that might occur during the register read operation.

    custom_components/solis_modbus/config_flow.py [35-38]

    -if user_input["type"] == "string":
    -    await modbus_controller.async_read_input_register(3262)
    -else:
    -    await modbus_controller.async_read_input_register(33263)
    +try:
    +    if user_input["type"] == "string":
    +        await modbus_controller.async_read_input_register(3262)
    +    else:
    +        await modbus_controller.async_read_input_register(33263)
    +except Exception as e:
    +    # Handle specific exceptions or log error
    +    return False
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the async_read_input_register method is a significant improvement, as it enhances the robustness of the code by managing potential exceptions during register read operations, which could prevent unexpected crashes.

    8
    Ensure all register addresses are valid and correct any errors

    Validate that all register fields contain valid register addresses and correct any
    typographical errors or incorrect register references.

    custom_components/solis_modbus/data/string_sensors.py [141]

    -{"type": "SDS", "name": "Solis Battery Charge Power", "unique": "solis_modbus_inverter_battery_charge_power", "device_class": SensorDeviceClass.POWER, "multiplier": 0.1, "unit_of_measurement": UnitOfPower.WATT, "state_class": SensorStateClass.MEASUREMENT, "register": ['33149', '33150', '33135', '0']},
    +{"type": "SDS", "name": "Solis Battery Charge Power", "unique": "solis_modbus_inverter_battery_charge_power", "device_class": SensorDeviceClass.POWER, "multiplier": 0.1, "unit_of_measurement": UnitOfPower.WATT, "state_class": SensorStateClass.MEASUREMENT, "register": ['33149', '33150', '33135']},  # Corrected register addresses
    Suggestion importance[1-10]: 6

    Why: The suggestion to validate register addresses is reasonable, as incorrect addresses can lead to data retrieval errors. However, without specific evidence of errors in the existing code, the impact is moderate. The removal of '0' from the register list seems arbitrary without further context.

    6
    Performance
    Adjust the scan interval to prevent continuous polling and potential performance issues

    Avoid setting scan_interval to 0 as it can cause continuous polling without any
    delay, potentially leading to performance issues or overloading the device.

    custom_components/solis_modbus/data/string_sensors.py [8]

    -"scan_interval": 0,
    +"scan_interval": 60,  # Adjusted to a reasonable interval
    Suggestion importance[1-10]: 8

    Why: Setting the scan interval to 0 can indeed cause performance issues due to continuous polling. Adjusting it to a reasonable interval is a valid suggestion to prevent potential overloads and ensure efficient operation.

    8
    Improve performance by caching static data schema

    Consider caching the result of _get_user_schema if it does not change often, to
    improve performance by reducing redundant computations.

    custom_components/solis_modbus/config_flow.py [26]

    -data_schema=self._get_user_schema()
    +data_schema=self.cached_user_schema if hasattr(self, 'cached_user_schema') else self._get_user_schema()
    Suggestion importance[1-10]: 5

    Why: Caching the result of _get_user_schema can improve performance by reducing redundant computations if the schema does not change often. However, the impact is moderate as it depends on how frequently the schema is accessed.

    5
    Possible issue
    Remove duplicate sensor entries to maintain data integrity

    Remove duplicate sensor entries to prevent data conflicts and ensure unique
    identifiers for each sensor.

    custom_components/solis_modbus/data/string_sensors.py [49-52]

    -{"type": "SS", "name": "Solis Total Generation Energy", "unique": "solis_modbus_inverter_total_generation_energy", "register": ['36050', '36051'], "device_class": SensorDeviceClass.ENERGY, "multiplier": 0.01, "unit_of_measurement": UnitOfEnergy.KILO_WATT_HOUR, "state_class": SensorStateClass.TOTAL_INCREASING},
    +...  # Ensure each sensor entry is unique
    Suggestion importance[1-10]: 7

    Why: The suggestion highlights a potential issue with duplicate sensor entries, which can lead to data conflicts. Ensuring unique identifiers for each sensor is important for maintaining data integrity.

    7
    Add handling for the False return value to ensure consistent function behavior

    Ensure that the function returns a consistent type or handles the False return value
    explicitly if inverter_type is 'string' to prevent potential issues in the calling
    code.

    custom_components/solis_modbus/switch.py [22-23]

     if inverter_type == 'string':
         return False
    +# Ensure the rest of the function or the calling code handles the False return appropriately
    Suggestion importance[1-10]: 5

    Why: The suggestion highlights the need to handle the False return value, which is important for consistent function behavior. However, it lacks specific implementation details on how to handle the return value, making it less actionable.

    5
    Maintainability
    Replace repetitive conditional imports with a dictionary mapping to streamline the code and enhance maintainability

    Consider using a dictionary to map inverter types to their respective modules and
    sensor data to avoid repetitive conditional statements and improve code
    maintainability.

    custom_components/solis_modbus/sensor.py [31-36]

    -if inverter_type == "string":
    -    from .data.string_sensors import string_sensors as sensors
    -    from .data.string_sensors import string_sensors_derived as sensors_derived
    -else:
    -    from .data.hybrid_sensors import hybrid_sensors as sensors
    -    from .data.hybrid_sensors import hybrid_sensors_derived as sensors_derived
    +sensor_modules = {
    +    "string": (".data.string_sensors", "string_sensors", "string_sensors_derived"),
    +    "hybrid": (".data.hybrid_sensors", "hybrid_sensors", "hybrid_sensors_derived")
    +}
    +module, sensors, sensors_derived = sensor_modules.get(inverter_type, sensor_modules["hybrid"])
    +sensors = __import__(module, fromlist=[sensors]).__dict__[sensors]
    +sensors_derived = __import__(module, fromlist=[sensors_derived]).__dict__[sensors_derived]
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a dictionary for mapping inverter types to their respective modules reduces repetitive code and enhances maintainability. However, the use of __import__ and __dict__ can be less readable and may introduce complexity, which slightly reduces the score.

    7

    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.

    Solis S6-GR1P4K support Support for regular string inverters
    1 participant