Simplify the V1 methods by using single re-usable base methods #125

GraemeDBlue · 2025-10-09T09:21:44Z

Summary

Simplify the V1 methods by using single re-usable base methods
Add Mix/SPH to V1 and while retaining legacy Shinephone backwards compatibility
Improve some code style

Checklist

I've made sure the PR does small incremental changes. (new code additions are dificult to review when e.g. the entire repository got improved codestyle in the same PR.)
I've added/updated the relevant docs for code changes i've made.

Cleanup

johanzander · 2025-10-09T15:05:19Z

Thanks for adding mix support! However, I have a concern about the API design:

Passing inverter_type as a parameter to every method call creates unnecessary coupling and repetition. As I wrote in previous feedback (even though AI helped me phrase it): Inverter type is a property of the inverter, not a parameter of each operation. It should be determined once and reused internally, not passed around repeatedly.

Here is how you could do it:

  class OpenApiV1(GrowattApi):
      def __init__(self, token):
          super().__init__(agent_identifier=self._create_user_agent())
          self.api_url = f"{self.server_url}v1/"
          self.session.headers.update({"token": token})
          self._device_type_cache = {}

      def device_list(self, plant_id):
          """Get devices and cache their types"""
          response = self.session.get(
              url=self._get_url("device/list"),
              params={"plant_id": plant_id, "page": "", "perpage": ""}
          )
          data = self._process_response(response.json(), "getting device list")

          # Cache device types (type field from API)
          for device in data.get('devices', []):
              self._device_type_cache[device['device_sn']] = device['type']

          return data

      def _get_device_type(self, device_sn):
          """Get cached device type"""
          if device_sn not in self._device_type_cache:
              raise GrowattParameterError(
                  f"Device type unknown for {device_sn}. Call device_list() first."
              )
          return self._device_type_cache[device_sn]

      def _map_type_to_enum(self, type_code):
          """Map numeric type to DeviceType enum"""
          # Based on device_list documentation (line 203-213)
          if type_code == 7:
              return DeviceType.MIN_TLX
          elif type_code in [2, 5, 6]:  # storage, sph, spa
              return DeviceType.MIX_SPH
          # ... other mappings
          else:
              raise GrowattParameterError(f"Unsupported device type: {type_code}")

      # PUBLIC API - user never passes device_type
      def device_detail(self, device_sn):
          """Get device details - automatically routes to correct endpoint"""
          type_code = self._get_device_type(device_sn)
          device_type = self._map_type_to_enum(type_code)
          return self._device_details(device_sn, device_type)

      def device_energy(self, device_sn):
          """Get device energy - automatically routes to correct endpoint"""
          type_code = self._get_device_type(device_sn)
          device_type = self._map_type_to_enum(type_code)
          return self._device_energy(device_sn, device_type)

      # INTERNAL methods (with underscore prefix) - these take device_type
      def _device_details(self, device_sn, device_type):
          """Internal: Get detailed data for a device"""
          if not isinstance(device_type, DeviceType):
              raise GrowattParameterError(f"Invalid device type: {device_type}")

          response = self.session.get(
              self._get_device_url(device_type, ApiDataType.BASIC_INFO),
              params={'device_sn': device_sn}
          )
          return self._process_response(response.json(), f"getting {device_type.name} 
  details")

      # BACKWARDS COMPATIBILITY - keep old method names
      def min_detail(self, device_sn):
          """Backwards compatible - prefer device_detail()"""
          return self._device_details(device_sn, DeviceType.MIN_TLX)

      def mix_detail(self, device_sn, plant_id=None):
          """Backwards compatible - prefer device_detail()"""
          return self._device_details(device_sn, DeviceType.MIX_SPH)

  Clean Usage

  # Get all devices (caches types automatically)
  devices = api.device_list(plant_id)

  # Clean, consistent API - works for ALL device types
  for device in devices['devices']:
      sn = device['device_sn']

      # User never thinks about type - just uses device_sn
      details = api.device_detail(sn)
      energy = api.device_energy(sn)
      settings = api.device_settings(sn)

  Key points:
  - Public methods (device_detail, device_energy) = no device_type parameter
  - Internal methods (_device_details, _device_energy) = have device_type parameter
  - Backwards compatibility kept with min_detail, mix_detail wrappers
  - Type determined once from device_list(), cached, and reused automatically

GraemeDBlue · 2025-10-09T15:49:53Z

Thanks for adding mix support! However, I have a concern about the API design:

Passing inverter_type as a parameter to every method call creates unnecessary coupling and repetition. As I wrote in previous feedback (even though AI helped me phrase it): Inverter type is a property of the inverter, not a parameter of each operation. It should be determined once and reused internally, not passed around repeatedly.

Here is how you could do it:

  class OpenApiV1(GrowattApi):
      def __init__(self, token):
          super().__init__(agent_identifier=self._create_user_agent())
          self.api_url = f"{self.server_url}v1/"
          self.session.headers.update({"token": token})
          self._device_type_cache = {}

      def device_list(self, plant_id):
          """Get devices and cache their types"""
          response = self.session.get(
              url=self._get_url("device/list"),
              params={"plant_id": plant_id, "page": "", "perpage": ""}
          )
          data = self._process_response(response.json(), "getting device list")

          # Cache device types (type field from API)
          for device in data.get('devices', []):
              self._device_type_cache[device['device_sn']] = device['type']

          return data

      def _get_device_type(self, device_sn):
          """Get cached device type"""
          if device_sn not in self._device_type_cache:
              raise GrowattParameterError(
                  f"Device type unknown for {device_sn}. Call device_list() first."
              )
          return self._device_type_cache[device_sn]

      def _map_type_to_enum(self, type_code):
          """Map numeric type to DeviceType enum"""
          # Based on device_list documentation (line 203-213)
          if type_code == 7:
              return DeviceType.MIN_TLX
          elif type_code in [2, 5, 6]:  # storage, sph, spa
              return DeviceType.MIX_SPH
          # ... other mappings
          else:
              raise GrowattParameterError(f"Unsupported device type: {type_code}")

      # PUBLIC API - user never passes device_type
      def device_detail(self, device_sn):
          """Get device details - automatically routes to correct endpoint"""
          type_code = self._get_device_type(device_sn)
          device_type = self._map_type_to_enum(type_code)
          return self._device_details(device_sn, device_type)

      def device_energy(self, device_sn):
          """Get device energy - automatically routes to correct endpoint"""
          type_code = self._get_device_type(device_sn)
          device_type = self._map_type_to_enum(type_code)
          return self._device_energy(device_sn, device_type)

      # INTERNAL methods (with underscore prefix) - these take device_type
      def _device_details(self, device_sn, device_type):
          """Internal: Get detailed data for a device"""
          if not isinstance(device_type, DeviceType):
              raise GrowattParameterError(f"Invalid device type: {device_type}")

          response = self.session.get(
              self._get_device_url(device_type, ApiDataType.BASIC_INFO),
              params={'device_sn': device_sn}
          )
          return self._process_response(response.json(), f"getting {device_type.name} 
  details")

      # BACKWARDS COMPATIBILITY - keep old method names
      def min_detail(self, device_sn):
          """Backwards compatible - prefer device_detail()"""
          return self._device_details(device_sn, DeviceType.MIN_TLX)

      def mix_detail(self, device_sn, plant_id=None):
          """Backwards compatible - prefer device_detail()"""
          return self._device_details(device_sn, DeviceType.MIX_SPH)

  Clean Usage

  # Get all devices (caches types automatically)
  devices = api.device_list(plant_id)

  # Clean, consistent API - works for ALL device types
  for device in devices['devices']:
      sn = device['device_sn']

      # User never thinks about type - just uses device_sn
      details = api.device_detail(sn)
      energy = api.device_energy(sn)
      settings = api.device_settings(sn)

  Key points:
  - Public methods (device_detail, device_energy) = no device_type parameter
  - Internal methods (_device_details, _device_energy) = have device_type parameter
  - Backwards compatibility kept with min_detail, mix_detail wrappers
  - Type determined once from device_list(), cached, and reused automatically

@johanzander thanks for your comments, but there is no need to pass the type at all did you see

   plants = api.plant_list()
    print(f"Plants: Found {plants['count']} plants")
    plant_id = plants['plants'][0]['plant_id']
    today = datetime.date.today()
    devices = api.get_devices(plant_id)
    for device in devices:
        # Works automatically for MIN, MIX, or any future device type!
        details = device.details()
        energy = device.energy()
        settings = device.settings()
        history = device.energy_history(start_date=today)

johanzander · 2025-10-09T21:10:58Z

examples/min_example_dashboard.py

+        raise Exception("No MIN_TLX device found to get energy data from.")

    # energy data does not contain epvToday for some reason, so we need to calculate it
    epv_today = energy_data["epv1Today"] + energy_data["epv2Today"]


great to add this to the library. But I have up to 4 individual PVs, so please use something more dynamic, taking into account the sum of up to 4 if present.

@johanzander are they all of the same type or do you have a mix? that might be a interesting scenario

added a sum

johanzander · 2025-10-09T21:12:39Z

but you have modified min_example.py to take device type parameter. is this a mistake?

johanzander · 2025-10-09T21:14:06Z

examples/min_example.py

-            inverter_data = api.min_detail(inverter_sn)
+            inverter_data = api.device_details(
+                device_sn=inverter_sn, 
+                device_type=device_type


passing device_type?

let me check , but I believe I have 3 different ways to call the same methods , device_details() is the base method with sn and device type https://github.com/indykoning/PyPi_GrowattServer/pull/125/files#diff-12865f9986e5f28194397aae0efed2b7abc9137d18c0a09197682e6c7e65cd01R484-R511, the details() https://github.com/indykoning/PyPi_GrowattServer/pull/125/files#diff-12865f9986e5f28194397aae0efed2b7abc9137d18c0a09197682e6c7e65cd01R1225-R1227 where the sn and device type are set on the object, so not needed as params , I will roll back my changes in examples/min_example.py to use a backwards compatible min_detail(sn) which will just call device_details(sn, device_type=7 )

…methods

revert changes in min_* example and add backwards compatible wrapper …

johanzander

Backward compatibility with MIN inverters using V1 seem to work...
Will test the new harmonized V1 APIs later...

johanzander · 2025-10-11T22:29:23Z

growattServer/open_api_v1.py

+        if DEBUG >= 1:
+            print(f"Saving {self.slugify(operation_name)}_data.json")  # noqa: T201
+            with open(f"{self.slugify(operation_name)}_data.json", "w") as f:  # noqa: PTH123
+                json.dump(response, f, indent=4, sort_keys=True)


Please remove debug code from prod version.
Im testing the library and I see these:
Saving getting_MIN_TLX_details_data.json
Saving getting_MIN_TLX_settings_data.json
Saving getting_MIN_TLX_energy_data_data.json

johanzander · 2025-10-12T20:06:53Z

growattServer/open_api_v1.py

+    """Enumeration of Growatt device types."""
+
+    MIX_SPH = 5  # MIX/SPH devices
+    MIN_TLX = 7  # MIN/TLX devices


For consistency, should you call the enum

MiX_SPH for SPH_MIX instead (<DEVICE_TYPE>)

consider adding all types in the enum:
INVERTER = 1
STORAGE = 2
OTHER = 3
MAX = 4
SPH_MIX = 5
SPA = 6
MIN_TLX = 7
PCS = 8
HPS = 9
PBD = 10

https://www.showdoc.com.cn/262556420217021/6117958613377445

The only thing consistent with their naming convention, is the inconsistency

GraemeDBlue · 2026-01-05T18:26:31Z

@indykoning I have pulled the upstream changes in and fixed as many ruff errors as possible

johanzander · 2026-01-05T21:38:20Z

Concerns with This PR

I really appreciate the effort to add MIX/SPH support, but I have concerns about the approach.

1. Breaking Changes Will Break Home Assistant Integration

The PR changes existing public method signatures:

Before (current):

api.min_read_parameter(device_sn, parameter_id, start_address=None, end_address=None)
api.min_write_time_segment(device_sn, segment_id, batt_mode, start_time, end_time, enabled=True)

After (this PR):

api.min_read_parameter(device_sn, params: dict)
api.min_write_time_segment(device_sn, params: TimeSegmentParams)

Impact: This will immediately break existing code including the Home Assistant Growatt integration which relies on this library. We need to maintain backward compatibility.

2. The "Generic" Methods Don't Actually Solve a Real Problem

The new common_read_parameter() and device_settings() methods aim to reduce duplication, but they're more verbose without adding value:

Current (clear and concise)

api.min_read_parameter(device_sn, 'discharge_power')

Proposed (verbose and exposes implementation details)

api.common_read_parameter(device_sn, DeviceType.MIN_TLX, 'discharge_power')

There's minimal code duplication being eliminated - just URL routing logic. MIN and SPH devices call completely different API endpoints (device/tlx/* vs device/mix/*, readMinParam vs readMixParam) with different parameters. The "generic" methods just add an abstraction layer that exposes device-type implementation details at every call.

3. Recommendation

To add SPH support while maintaining backward compatibility:

Keep existing min_* method signatures unchanged
Add parallel sph_* methods for SPH devices (following the official Growatt API naming for device type 5, or use mix_*):

# Settings
 def sph_settings(self, device_sn):
     """Get settings for an SPH inverter."""
     # SPH-specific implementation

 # Read/Write Parameters
 def sph_read_parameter(self, device_sn, parameter_id, start_address=None, end_address=None):
     """Read parameter from SPH inverter."""
     # SPH-specific implementation

 def sph_write_parameter(self, device_sn, parameter_id, parameter_values=None):
     """Write parameter to SPH inverter."""
     # SPH-specific implementation

 # Time Segments
 def sph_read_time_segments(self, device_sn, settings_data=None):
     """Read time segments from SPH inverter."""
     # SPH-specific implementation

 def sph_write_time_segment(self, device_sn, segment_id, batt_mode, start_time, end_time, enabled=True):
     """Write time segment to SPH inverter."""
     # SPH-specific implementation

This approach:

Maintains backward compatibility (doesn't break Home Assistant)
Provides clear, device-specific interfaces
Follows the existing min_* pattern users already understand
Uses official Growatt API naming (device type 5 = "sph")
Doesn't expose device-type implementation details at call sites
Covers all operations: settings, parameters, and time segments

The MIN and SPH implementations can share internal helper methods like _get_device_url() if needed, but the public API should remain device-specific and backward-compatible.

4. Alternative: If Truly Generic Methods Are Desired

If the goal is to have generic methods for parameter operations, a better approach would cache device types internally:

Usage:

 # Device types are auto-registered when calling device_list
 devices = api.device_list(plant_id)

 # Then use truly generic methods for parameters (no device_type parameter!)
 api.read_parameter(device_sn, 'discharge_power')
 api.write_parameter(device_sn, 'ac_charge', 1)

Implementation approach:

Add _device_type_cache dict that maps device_sn -> DeviceType
Auto-populate it in device_list() when devices are retrieved
Generic parameter methods look up device type from cache automatically
Optionally provide register_device(device_sn, device_type) for manual registration
Time segment methods remain device-specific since they may have different parameters

Benefits:

Truly generic API for common operations (no device_type at call sites)
Backward compatible (existing min_* methods unchanged)
Additive feature (doesn't break anything)
Cleaner than common_read_parameter(sn, device_type, param)
Device-specific methods remain available for operations with different signatures

This would provide a genuinely generic interface for parameter read/write operations while maintaining all existing functionality.

Would you be open to refactoring along these lines?

GraemeDBlue · 2026-01-05T21:55:08Z

@johanzander so why did you not say this months ago, when you first looked at this?

GraemeDBlue · 2026-01-05T22:06:57Z

@johanzander also saying it would break the HA integration, should not happen if the dependency is using correct SemVer dependency requirements right?

johanzander · 2026-01-05T22:11:53Z

@GraemeDBlue I did provide feedback months ago about the API design concerns - specifically about passing device_type as a parameter to every method call rather than caching it internally.

However, I'll be honest: I didn't anticipate that existing min_* method signatures would be changed in a way that breaks backward compatibility. I assumed we'd be adding new functionality rather than modifying existing public APIs that downstream projects like Home Assistant depend on.

Looking back at the PR history, it appears some of the breaking signature changes (like min_read_parameter and min_write_time_segment taking params objects instead of individual arguments) may have been introduced since my earlier comments.

My feedback remains focused on two main points:

Backward compatibility is critical - The existing min_* methods are in production use and changing their signatures will break existing code
API design - If we want generic methods, caching device types internally provides a cleaner interface than requiring device_type as a parameter at every call site

I'm happy to discuss how we can add SPH support in a way that doesn't break existing integrations. The work you've done on the underlying implementation is valuable - it's really just about how we expose it as a public API.

GraemeDBlue · 2026-01-05T22:15:10Z

@johanzander yeah I'm open to working together, but thought you had ghosted me, so stopped asking you for feedback.

johanzander · 2026-01-05T22:15:16Z

also saying it would break the HA integration, should not happen if the dependency is using correct SemVer dependency requirements right?

Yes, it wouldn't break immediately because of these changes were commited or even this library released, but if the HA integration would be updated from current 1.7.1 to a newer version it would break.

In theory one could update the library version and the method calls in HA at the same time, but HA project don't allow this - they bump libraries in separate PRs from code updates.

johanzander · 2026-01-05T22:20:53Z

@johanzander yeah I'm open to working together, but thought you had ghosted me, so stopped asking you for feedback.

No, I have been busy with the Home Assistant Growatt integration and finally all major changes I needed (reading / writing TOU settings, etc.) have been accepted and will be released in the 2026.1 update.

So now I will have more time to spend on this library update.

johanzander · 2026-01-06T09:00:15Z

@GraemeDBlue - I'd like to help get this merged. If you're open to it, I can collaborate on updating the PR to:

Restore the original min_* method signatures (backward compatibility)
Add the new sph_* methods (following the same pattern)
Keep your internal implementation work (which is solid)
Optionally add generic methods for read/write settings

Would you be open to that? I can push changes to your branch or create a collaborative branch - whatever works best for you.

GraemeDBlue · 2026-01-06T09:04:19Z

However, I'll be honest: I didn't anticipate that existing min_* method signatures would be changed in a way that breaks backward compatibility. I assumed we'd be adding new functionality rather than modifying existing public APIs that downstream projects like Home Assistant depend on.

Looking back at the PR history, it appears some of the breaking signature changes (like min_read_parameter and min_write_time_segment taking params objects instead of individual arguments) may have been introduced since my earlier comments.

@johanzander my idea had been to get the Growatt library to control and validate the params sent , rather than the HA implementation by calling specific library method like johanzander/growatt_server_upstream@master...GraemeDBlue:growatt_server_upstream:master#diff-814821c0de54a55c25b8b789f81ed1edcf1486fa245f99748de316999e503fc4R551

GraemeDBlue · 2026-01-06T09:11:09Z

@GraemeDBlue - I'd like to help get this merged. If you're open to it, I can collaborate on updating the PR to:

Restore the original min_* method signatures (backward compatibility)

Add the new sph_* methods (following the same pattern)

Keep your internal implementation work (which is solid)

Optionally add generic methods for read/write settings

Would you be open to that? I can push changes to your branch or create a collaborative branch - whatever works best for you.

@johanzander yeah I can add you as a collaborator on my branch

GraemeDBlue · 2026-01-06T09:13:34Z

@johanzander aah its not protected anyway so maybe we should just create a new branch and have some sort of plan rather than working cross purposes

GraemeDBlue and others added 17 commits September 24, 2025 12:47

Initial commit adding common methods for mix and min

c6703ba

Try tio simplify ,ethods and provide backwards compatible method names

6e487d6

normalise method names

8a07ba6

add Dashboard example for Mix V1

cf7d7b8

Fixes for dashboard

aa01035

Reworking

1d1e8e2

added docs

76b4548

add backwards compatible methods for min_*

52661fd

uncomment code

ce9afa5

Fixing up example

21f9581

fix example dashboard

fd98a51

debugging

7d4db09

minor tweaks

837c78c

Code quality

f042b63

fixes

0c03dfc

fix formatting and code quality

e4241e9

Merge pull request #1 from GraemeDBlue/cleanup

3f3790d

Cleanup

GraemeDBlue marked this pull request as draft October 9, 2025 09:48

johanzander reviewed Oct 9, 2025

View reviewed changes

GraemeDBlue and others added 3 commits October 10, 2025 12:05

revert changes in min_* example and add abckwards compatible wrapper …

9baba05

…methods

add missing renamed file for v1 example

f58d8b1

Merge pull request #2 from GraemeDBlue/cleanup

48a8fc6

revert changes in min_* example and add backwards compatible wrapper …

johanzander reviewed Oct 11, 2025

View reviewed changes

johanzander reviewed Oct 12, 2025

View reviewed changes

GraemeDBlue added 2 commits October 13, 2025 10:05

Added all constants

ae1f36e

Dynamically sum pev based on count of devices

4314600

GraemeDBlue and others added 16 commits January 5, 2026 14:29

fixes for ruff in older files

30e3150

ruff fixes

022a457

more fixes for ruff formaTTING

44f2cdd

more fixes for older files

0ee095c

fix formatting

e08d6ee

fix long line issues

8be12b0

fix long line issues

8f991ac

fix some more ruff issues

88ad246

fix some lazy code

97926a3

reformat for long lines

09cbdc5

fixes for formatting

50797c4

fix long line issue

b551c7f

Merge branch 'master' into master

8a645a1

apply ruff to pulled changes from master

8a192f5

rework some upstream changex to work with ruff

6604c5f

more fix for upstream changes

b782ea0

This was referenced Jan 8, 2026

Add support for SPH in V1 API #127

Closed

Add support for SPH inverters using V1 API GraemeDBlue/PyPi_GrowattServer#11

Open

Uh oh!

Simplify the V1 methods by using single re-usable base methods #125

Are you sure you want to change the base?

Simplify the V1 methods by using single re-usable base methods #125

Uh oh!

Conversation

GraemeDBlue commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johanzander commented Oct 9, 2025

Uh oh!

GraemeDBlue commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanzander commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanzander left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GraemeDBlue commented Jan 5, 2026

Uh oh!

johanzander commented Jan 5, 2026

Concerns with This PR

1. Breaking Changes Will Break Home Assistant Integration

2. The "Generic" Methods Don't Actually Solve a Real Problem

Current (clear and concise)

Proposed (verbose and exposes implementation details)

3. Recommendation

4. Alternative: If Truly Generic Methods Are Desired

Uh oh!

GraemeDBlue commented Jan 5, 2026

Uh oh!

GraemeDBlue commented Jan 5, 2026

Uh oh!

johanzander commented Jan 5, 2026

Uh oh!

GraemeDBlue commented Jan 5, 2026

Uh oh!

johanzander commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johanzander commented Jan 5, 2026

Uh oh!

johanzander commented Jan 6, 2026

Uh oh!

GraemeDBlue commented Jan 6, 2026

Uh oh!

GraemeDBlue commented Jan 6, 2026

Uh oh!

GraemeDBlue commented Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GraemeDBlue commented Oct 9, 2025 •

edited

Loading

GraemeDBlue commented Oct 9, 2025 •

edited

Loading

johanzander commented Jan 5, 2026 •

edited

Loading