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

add type hints and static type checking #76

Merged
merged 7 commits into from
Oct 30, 2023
Merged

Conversation

vchrisb
Copy link
Collaborator

@vchrisb vchrisb commented May 17, 2023

Type hinting is now available in all supported Python versions.

  • added type hints
  • added pyright static type checking
  • code refactoring
  • web connection is only using rscp via websocket
  • enhanced test scripts

This should help further improve code quality and make it more readable

@vchrisb vchrisb self-assigned this May 17, 2023
@vchrisb
Copy link
Collaborator Author

vchrisb commented May 19, 2023

@torbennehmer would you mind trying this PR out?
You're currently actively working with it. 🥇

@torbennehmer
Copy link
Contributor

@vchrisb Of course, will do, but it'll probably take until Sunday to get around to it.

@torbennehmer
Copy link
Contributor

torbennehmer commented May 24, 2023

@vchrisb I came around to give this a try, most things work, however there are at least to places, where I've got regressions. I didn't have time to analyse it deeper (I'm preparing for a business-trip, a large software rollout next week). I can look deeper into it if you need it (just say) afterwards. The problems I saw:

print(json.dumps(e3dc.poll_switches(), indent=2, default=str))

Traceback (most recent call last):
  File "/home/torben/src/python-e3dc/test.py", line 18, in <module>
    print(json.dumps(e3dc.poll_switches(), indent=2, default=str))
                     ^^^^^^^^^^^^^^^^^^^^
  File "/home/torben/src/python-e3dc/e3dc/_e3dc.py", line 448, in poll_switches
    descList = switchDesc[2]  # get the payload of the container
               ~~~~~~~~~~^^^
IndexError: list index out of range

The standard branch returns an empty list here ([]).

Similarily, this fails too:

print(json.dumps(e3dc.get_pvis_data(), indent=2, default=str))

  File "/home/torben/src/python-e3dc/test.py", line 19, in <module>
    print(json.dumps(e3dc.get_pvis_data(), indent=2, default=str))
                     ^^^^^^^^^^^^^^^^^^^^
  File "/home/torben/src/python-e3dc/e3dc/_e3dc.py", line 1789, in get_pvis_data
    self.get_pvi_data(
  File "/home/torben/src/python-e3dc/e3dc/_e3dc.py", line 1577, in get_pvi_data
    voltageMonitoring = rscpFindTag(req, "PVI_VOLTAGE_MONITORING")
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/torben/src/python-e3dc/e3dc/_rscpLib.py", line 66, in rscpFindTag
    raise KeyError("There is no Tag named {} in the message.".format(tag))
KeyError: 'There is no Tag named PVI_VOLTAGE_MONITORING in the message.'

The standard code lists this:

[
  {
    "acMaxApparentPower": 4000.0,
    "cosPhi": {
      "active": null,
      "value": null,
      "excited": null
    },
    "deviceState": {
      "connected": true,
      "working": true,
      "inService": false
    },
    "frequency": {
      "under": null,
      "over": null
    },
    "index": 0,
    "lastError": "3 0x0",
    "maxPhaseCount": 3,
    "maxStringCount": 2,
    "onGrid": true,
    "phases": {
      "0": {
        "power": 1321.0,
        "voltage": 231.1,
        "current": 5.71,
        "apparentPower": 1324.0,
        "reactivePower": 0.0,
        "energyAll": 22787022.0,
        "energyGridConsumption": 22257.0
      },
      "1": {
        "power": 0.0,
        "voltage": 231.7,
        "current": 0.0,
        "apparentPower": 0.0,
        "reactivePower": 0.0,
        "energyAll": 8551523.0,
        "energyGridConsumption": 850.0
      },
      "2": {
        "power": 0.0,
        "voltage": 234.2,
        "current": 0.0,
        "apparentPower": 0.0,
        "reactivePower": 0.0,
        "energyAll": 8674676.0,
        "energyGridConsumption": 953.0
      }
    },
    "powerMode": 1,
    "serialNumber": "E3<redacted>66",
    "state": "0xb32715",
    "strings": {
      "0": {
        "power": 22.0,
        "voltage": 244.0,
        "current": 0.09,
        "energyAll": 45005419.0
      }
    },
    "systemMode": 2,
    "temperature": {
      "max": 130.0,
      "min": -30.0,
      "values": [
        28.9,
        29.3,
        32.6,
        32.6
      ]
    },
    "type": 3,
    "version": " MAIN HW06 2.048",
    "voltageMonitoring": {
      "thresholdTop": null,
      "thresholdBottom": null,
      "slopeUp": null,
      "slopeDown": null
    }
  }
]

I have not yet tested (e.g. directly called):

  • get_db_data, get_db_data_timestamp
  • get_power_data
  • get_powermeter_data (get_powermeters_data works)
  • get_battery_data (get_batteries_data works)
  • get_pvi_data (get_pvis_data works)
  • get_system_info_static
  • get_system_status Correction, this one's working.
  • set_idle_periods
  • set_switch_onoff (don't have switches)

In addition:

  • everything apart the direct connection poll version, e.g. no web connection or no fallback connection w/o internet access
  • I haven't tried invalid authentication etc. so no error provoking on my side yet

The two known bugs above are no direct problem, as soon as I give my local test environment another run, I'll use your branch for further tests.

@vchrisb
Copy link
Collaborator Author

vchrisb commented Jun 9, 2023

Thank you for your effort.
Interesting that you don't get values for cosPhi, frequency, and voltageMonitoring
I would have expected these to be present in any e3dc system. If only there would be some documentation...

I missed testing poll_switches(), I'l look into it.

@torbennehmer
Copy link
Contributor

torbennehmer commented Jul 6, 2023

Hi @vchrisb,
so, finally I got some more time for HA:
poll_switches works with the latest version of the branch, I get an empty array as I have no switches paired to E3DC.Can't really say what happens with switches though.
get_pvis_data still fails as before, failing to find PVI_VOLTAGE_MONITORING.

Also as a further successful test, get_db_data_timestamp and get_db_data both work as well. And of course get_system_info_static, as this is called during init.

@vchrisb
Copy link
Collaborator Author

vchrisb commented Oct 26, 2023

@torbennehmer , @eikowagenknecht and @bullitt186 would you mind trying this PR? 🥇
There are quite a few changes, but behavior should not have changed!
Adding pyright also did surface a few bugs. 👍

  • added type hints
  • added pyright static type checking
  • code refactoring
  • removed legacy code for web connection
  • enhanced test scripts

@eikowagenknecht
Copy link
Contributor

running mypy against it shows some more typing issues:

e3dc\_e3dc_rscp_web.py:116: error: Incompatible types in assignment (expression has type "str", variable has type "bytes")  [assignment]
e3dc\_e3dc_rscp_web.py:118: error: Incompatible types in assignment (expression has type "str", variable has type "bytes")  [assignment]
e3dc\_e3dc_rscp_web.py:126: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
e3dc\_e3dc_rscp_web.py:131: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
e3dc\_e3dc_rscp_web.py:135: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
e3dc\_e3dc.py:134: error: Incompatible types in assignment (expression has type "E3DC_RSCP_web", variable has type "E3DC_RSCP_local")  [assignment]
e3dc\_e3dc.py:144: error: Incompatible types in assignment (expression has type "int", variable has type "None")  [assignment]
e3dc\_e3dc.py:147: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:149: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:150: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:152: error: "None" has no attribute "startswith"  [attr-defined]
e3dc\_e3dc.py:153: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:157: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:158: error: "None" has no attribute "startswith"  [attr-defined]
e3dc\_e3dc.py:159: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:163: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:164: error: "None" has no attribute "startswith"  [attr-defined]
e3dc\_e3dc.py:165: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:169: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:170: error: "None" has no attribute "startswith"  [attr-defined]
e3dc\_e3dc.py:171: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:175: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:176: error: "None" has no attribute "startswith"  [attr-defined]
e3dc\_e3dc.py:177: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:181: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:182: error: "None" has no attribute "startswith"  [attr-defined]
e3dc\_e3dc.py:183: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:187: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:188: error: "None" has no attribute "startswith"  [attr-defined]
e3dc\_e3dc.py:189: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:193: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:195: error: Incompatible types in assignment (expression has type "str", variable has type "None")  [assignment]
e3dc\_e3dc.py:326: error: Incompatible types in assignment (expression has type "dict[str, Any]", variable has type "None")  [assignment]
e3dc\_e3dc.py:327: error: Incompatible types in assignment (expression has type "float", variable has type "int")  [assignment]
e3dc\_e3dc.py:468: error: Need type annotation for "idlePeriods"  [var-annotated]
tools\tests.py:118: error: "str" not callable  [operator]

I only checked some of those, but they were valid.

@vchrisb
Copy link
Collaborator Author

vchrisb commented Oct 26, 2023

pyright and mypy don't have the same set of rules.
For this PR pyright is configured with the following:

typeCheckingMode = "strict"
reportUnknownVariableType = "warning"
reportUnknownArgumentType = "warning"
reportUnknownMemberType = "warning"
reportUnknownParameterType = "warning"

I would be interested, if there is any change in behavior of the API compared to latest release of pye3dc with your E3DC.

@eikowagenknecht
Copy link
Contributor

pyright and mypy don't have the same set of rules.

Yeah, I was thinking maybe mypy would catch some extra issues like it's done before in other projects. But sticking with pyright's feedback for now is of course a valid choice.

I would be interested, if there is any change in behavior of the API compared to latest release of pye3dc with your E3DC.

A quick run of tests.py and diff against master looked good, no changes to see.

@torbennehmer
Copy link
Contributor

@torbennehmer , @eikowagenknecht and @bullitt186 would you mind trying this PR? 🥇

Will do, but probably not before the start of next week, unfortunately it's rather hectic here at the moment.

@bullitt186
Copy link
Contributor

I did manual back-to-back testing of all the get_*() calls this morning comparing the output of this PR compared to 0.8.2 and could not find any differences / oddities / performance differences.
However I did not test the set_*() calls so far.
But for the first ones it looks good to me!

@vchrisb
Copy link
Collaborator Author

vchrisb commented Oct 27, 2023

I did clean the commit history

@vchrisb vchrisb force-pushed the pyright branch 4 times, most recently from 2831dc2 to ec26f77 Compare October 27, 2023 12:26
@vchrisb vchrisb merged commit 66a7a23 into fsantini:master Oct 30, 2023
5 checks passed
@vchrisb vchrisb deleted the pyright branch October 30, 2023 15:59
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.

4 participants