search

dgibson / python-smadata2

Python code for communicating with SMA photovoltaic inverters within built in Bluetooth
GNU General Public License v2.0
23 stars 12 forks source link

Python3 fixes + some simplification using library functions #5

Closed Wilm0r closed 3 years ago

Wilm0r commented 5 years ago

Sadly there's no CRC library in Python by default, crcelk is an add-on library :-( But I like this more than including a reimplementation at least?

dgibson commented 5 years ago

Having these unrelated changes bundled together makes this patch a little harder to review than I'd like.

We already basically have Python3 support - I'm running the code with Python3 routinely now. Your added b'' looks correct (we're probably just getting away with that because of the bytearray() around it.

The encode('ascii') seems an odd way to do things. Since the password we're giving to the inverter is essentially a bytestring, not an ascii string, it seems to me we should pass in a bytestring to cmd_logon() rather than encoding it there.

I like some of the small cleanups you've made in smabluetooth.py, but I'd prefer them as a separate commit.

I'm dubious about using the external CRC library. Re-using things is good as a rule, but this adds an external dependency which makes the code trickier to deploy, so I'm not sure its worth it for a pretty modest code saving.