python-miio

python-miio copied to clipboard

I've mostly rebased the work on #501, applied some formatting and adopted heatshrink2 as dependency over heatshrink, which has become dead.

I've tested it on chuangmi.remote.v2 and it worked for sending pronto codes to my Sony TV directly, which is great and will fulfill my intentions, but I still couldn't make it learn commands, and discovery doesn't seem to work either.

Fixes #495, fixes #619, fixes #811 Closes #501 Partially covers #1020

Not much idea how about fixing Tests PyPy Ubuntu.

Just a quick comment as I don't currently have time to do a proper review:

• that pypy error looks like some sort of version incompatibility with heatshrink2's cython impl, but I have no idea either what would be a fix for that. Maybe it is not python3.6 compatible?
• IMO, this new dependency (heathsrink2) should be marked as an optional dependency, and an exception should be raised run-time if it's needed and not available.
• it'd be great if you'd have used @yawor's PR as a baseline without squashing his commits (to give him credit).

• that pypy error looks like some sort of version incompatibility with heatshrink2's cython impl, but I have no idea either what would be a fix for that. Maybe it is not python3.6 compatible?

Related:

• https://foss.heptapod.net/pypy/pypy/-/issues/3039
• https://github.com/cython/cython/issues/3448

• IMO, this new dependency (heathsrink2) should be marked as an optional dependency, and an exception should be raised run-time if it's needed and not available.

OK, will do.

• it'd be great if you'd have used @yawor's PR as a baseline without squashing his commits (to give him credit).

A squashed rebase is much simpler for me to do. I'll add credits on commit message.

@rytilahti besides marking heatshrink2 = { version = "*", optional = true }, should it be put in a new "extras" entry under [tool.poetry.extras]?

Changing the dependency to optional, I ignore how to fix it missing on CI, as reported by failing tests.

besides marking heatshrink2 = { version = "*", optional = true }, should it be put in a new "extras" entry under [tool.poetry.extras]?

I don't think there's a need to create a new extras entry, as the exception will inform anyone trying to use it to install the package. The docs entry is there just to simplify installing the required packages for development.

re: pypy problems, from the looks of it there is no easy solution for that? If so, I think reporting the issue to the heatshrink2 developers, and disabling pypy checks (and marking the reason & including a link to the upstream issue) for this one test file is fine.

@rytilahti I will report it on heatshrink2 repo, but could you take the torch about applying remaining details on merging this? Because I'm no knowledgeable Python dev and it's just my first contact with "poetry" and such. I'd have to look up how to "disable pypy checks for this one test file", which I simply ignore, and I don't have much time available for learning these tidbits.

I'll try to find some time over the weekend to do a review, for the pypy checks in the test file, something like this near the beginning after the pytest import should mark the whole module to be skipped when run with pypy:

import platform

pytestmark = pytest.mark.skipif(platform.python_implementation() == "PyPy", reason="heatshrink2 does not support pypy")

Ah, looks like the heatshrink2 needs to be added to the extras group to easier install for the CI. Do you mind adding it to the docs group (and renaming the group to dev in both pyproject.toml and azure-pipelines.yml)?

Looks like simply renaming docs to dev on those two places and updating lock file didn't work. Any ideas?

That's really odd, I have no clue why it isn't installing the library using pip... I'll try to debug that locally to find the reason for it when I do the code review.

Almost there, only this addition needs to be toggled off for PyPy on CI. I dunno the syntax for that. The trivial approach would be to have PyPy Tests in its own job separated from the main Tests matrix.

~~And I think, if this is going to happen on .azure-pipelines.yml, then the pytest.mark.skipif check wouldn't be needed at all.~~

Ah, nevermind, it would, the test still runs on PyPy anyways.

Okay, the mark is not being set for some reason.. The comparison should be fine as I just tested it in pypy repl:

$ pypy3
Python 3.7.10 (51efa818fd9b24f625078c65e8e2f6a5ac24d572, Apr 11 2021, 13:25:22)
[PyPy 7.3.4 with GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>> import platform; print(platform.python_implementation())
PyPy
>>>> 

@rytilahti please check my updated PR and comments, the only remaining issue is on .azure-pipelines.

The proper way is to find out why the pytestmark is not working, as that allows local testing, too. I can look into that later to figure out why it isn't working, the worst case is that every separate test function has to be decorated with the skipif instead of having a single one.

skip_pypy = pytest.mark.skipif(...)

@skip_pypy
def test_function():
    ...

@rytilahti both changes are needed, the pytestmark to skip the test on PyPy because heatshrink2 can't not be present anyways, and the Azure Pipelines condition to just not try installing heatshrink2 on PyPy, and installing it for the rest.

I did not manage to write the azure expression correctly yet:

condition: eq(variables.python.version, 'pypy3') results in:

Evaluating: eq(variables['python']['version'], 'pypy3')
Expanded: eq(Null, 'pypy3')
Result: False

I'm looking for the proper way to refer to $(python.version) in the condition expression.

What do you think?

Should be great but I'm not willing to do it, as the current code works for me and I don't have time for a rewrite. TBH, this azure condition fix is my last contribution attempt.

@rytilahti both changes are needed, the pytestmark to skip the test on PyPy because heatshrink2 can't not be present anyways, and the Azure Pipelines condition to just not try installing heatshrink2 on PyPy, and installing it for the rest

The heatshrink2 installs just fine here locally, so as long as those tests are not executed it should work just fine, no?

$ pypy3 -m venv testenv

$ . testenv/bin/activate

$ python --version
Python 3.7.10 (51efa818fd9b24f625078c65e8e2f6a5ac24d572, Apr 11 2021, 13:25:22)
[PyPy 7.3.4 with GCC 10.2.0]

$ pip --version
pip 20.1.1 from /home/tpr/testenv/site-packages/pip (python 3.7)

$ pip install heatshrink2
Collecting heatshrink2
  Downloading heatshrink2-0.10.0.tar.gz (108 kB)
     |████████████████████████████████| 108 kB 3.3 MB/s 
Using legacy setup.py install for heatshrink2, since package 'wheel' is not installed.
Installing collected packages: heatshrink2
    Running setup.py install for heatshrink2 ... done
Successfully installed heatshrink2-0.10.0
WARNING: You are using pip version 20.1.1; however, version 21.1 is available.
You should consider upgrading via the '/home/tpr/testenv/bin/pypy3 -m pip install --upgrade pip' command.

$ python
Python 3.7.10 (51efa818fd9b24f625078c65e8e2f6a5ac24d572, Apr 11 2021, 13:25:22)
[PyPy 7.3.4 with GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>> import heatshrink2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tpr/testenv/site-packages/heatshrink2/__init__.py", line 5, in <module>
    from . import core
  File "heatshrink2/core.pyx", line 1, in init heatshrink2.core
ValueError: array.array size changed, may indicate binary incompatibility. Expected 72 from C header, got 24 from PyObject
>>>> 

It didn't work on Azure, poetry install --extras dev fails because of heatshrink2 install. As commented above, my test is not right and is returning false because the variable reference is null, which executes poetry install --extras dev too, instead of just the poetry install step.

Hmm, actually sorry, on Azure at the moment it is failing when the tests are actually running. Checking it up..

Well, seems like the import is failing in other tests, a catch all try/except may fix it.

Looking at the CI logs for the most recent build, the exception is E ImportError: cannot import name 'encode' while the catch is for ModuleNotFoundError. There might be some differences between python releases (https://stackoverflow.com/q/62854761), so I'd just use except (ModuleNotFoundError, ImportError), and remove the version checks for azure-pipelines.

Should be great but I'm not willing to do it, as the current code works for me and I don't have time for a rewrite. TBH, this azure condition fix is my last contribution attempt.

Fair enough, I appreciate your contribution & honesty, hopefully someone will hop in to do the last steps to get it merged :-) If you don't mind allow modifications to the PR from maintainers, I might do that at some point.

The ImportError happens after a ValueError on traceback, while my ModuleNotFoundError was to cover solely when heatshrink2 is not installed, not when the import blows up for whatever reason. Adding a catch all (or those two) can avoid the issue, but, doesn't seems clear... I'm unsure.

You are free to change the PR in whatever way you wish. My desire is just to have support for these devices merged in any way and get a new release with it so to easily import it in my projects. I commented above on one of the suggested additions asking what exactly you wish me to do, but you can simply reword it yourself as you please, seems easier.

I'm wondering whether having changed the docs group to dev instead of creating a new one was worth it. I'm using poetry export to have the project exported to the requirements.txt format to install it on containers from sources but poetry export --extras dev --without-hashes adds the doc deps, which I don't need, just the extra runtime deps instead. So to avoid this I'm actually using poetry export --without-hashes output and adding heatshrink2 on top.

Hi all, It's been quite some time. I'd like to let you know that I'm currently working on a pure Python implementation of HeatShrink, which is going to be fully compatible with pyheatshrink2 library. Changing the import is all that's needed. Right now I already have a working decoder, which passes tests in the pyheatshrink2 library.

I've finished encoder too. I still have a FAIL on one test, but it is strange. The compression of a test file with default parameters produces a binary file slightly different than what the test expects, but the file decompresses properly and the decompressed content is identical to the original one.

@oblitum yeah, you are right, the optional dependencies (currently android_backup and then heatshrink2 as new) should belong to their own group (sphinx & co are really useless for end users), my bad.

@yawor hi there! Are you planning to publish that in pypi, too? So that we could simply swap the implementation later (or maybe even for this PR already)?

@rytilahti yes, that's my plan. I've never done this though. I'll publish it on a github in a moment. I've used the benchmark script from pyheatshrink2 and pure Python implementation is significantly slower: Results from pyheatshrink2 with native extension:

==================================================
Encode benchmarks
==================================================
*** Writing 10,000 bytes ***
==> 0.001150369644165039 seconds elapsed
*** Writing 100,000 bytes ***
==> 0.014354228973388672 seconds elapsed
*** Writing 1,000,000 bytes ***
==> 0.13378429412841797 seconds elapsed
*** Writing rest of the file ***
==> 0.73799729347229 seconds elapsed
==> Wrote 6488666 bytes
==================================================
Decode benchmarks
==================================================
*** Reading 10,000 bytes ***
==> 0.0004725456237792969 seconds elapsed
*** Reading 100,000 bytes ***
==> 0.002451658248901367 seconds elapsed
*** Reading 1,000,000 bytes ***
==> 0.024286985397338867 seconds elapsed
*** Reading rest of the file ***
==> 0.12644195556640625 seconds elapsed
<== Read 6488666 bytes

Results from my implementation:

==================================================
Encode benchmarks
==================================================
*** Writing 10,000 bytes ***
==> 0.08463716506958008 seconds elapsed
*** Writing 100,000 bytes ***
==> 1.0897622108459473 seconds elapsed
*** Writing 1,000,000 bytes ***
==> 10.651371479034424 seconds elapsed
*** Writing rest of the file ***
==> 57.530882835388184 seconds elapsed
==> Wrote 6488666 bytes
==================================================
Decode benchmarks
==================================================
*** Reading 10,000 bytes ***
==> 0.039415836334228516 seconds elapsed
*** Reading 100,000 bytes ***
==> 0.2732369899749756 seconds elapsed
*** Reading 1,000,000 bytes ***
==> 2.6301050186157227 seconds elapsed
*** Reading rest of the file ***
==> 13.844971418380737 seconds elapsed
<== Read 6488666 bytes

The difference is quite big, but on the other hand IR payloads are rather small so the impact shouldn't be that big. The library can be imported conditionally. If the pyheatshrink2 is available, then use it. If not, then use pure Python version instead.

Here's a link to my implementation: https://github.com/yawor/heatshrinkpy https://pypi.org/project/heatshrinkpy/

I think the best way to use it would be:

try:
    import heatshrink2
except ModuleNotFoundError:
    import heatshrinkpy as heatshrink2

Nice work! :100: Did you try to benchmark on pypy, too? It'll probably be faster, but in this case it does not really matter (due to payload sizes as you said, and no one is going to blast enough commands to notice the difference due to I/O anyway), so I think it's it's better to depend on heatshrinkpy directly without any fallbacks.

Here's a benchmark on pypy 7.3.4 (Python 3.7.10 equivalent):

==================================================
Encode benchmarks
==================================================
*** Writing 10,000 bytes ***
==> 0.08823657035827637 seconds elapsed
*** Writing 100,000 bytes ***
==> 0.0941777229309082 seconds elapsed
*** Writing 1,000,000 bytes ***
==> 0.6658759117126465 seconds elapsed
*** Writing rest of the file ***
==> 4.66175651550293 seconds elapsed
==> Wrote 6488666 bytes
==================================================
Decode benchmarks
==================================================
*** Reading 10,000 bytes ***
==> 0.05748915672302246 seconds elapsed
*** Reading 100,000 bytes ***
==> 0.03234267234802246 seconds elapsed
*** Reading 1,000,000 bytes ***
==> 0.14438748359680176 seconds elapsed
*** Reading rest of the file ***
==> 0.726499080657959 seconds elapsed
<== Read 6488666 bytes

I'm actually surprised to see that much of a difference.

chuangmi.remote.h102a03 warning, but device works fine.

Logger: miio.device Source: /usr/local/lib/python3.10/site-packages/miio/device.py:158 First occurred: 4:18:40 PM (1 occurrences) Last logged: 4:18:40 PM

Found an unsupported model 'chuangmi.remote.h102a03' for class 'ChuangmiIr'. If this is working for you, please open an issue at https://github.com/rytilahti/python-miio/

About HA: Version | core-2022.7.6 Python Version | 3.10.5 Operating System Version | 5.10.0-16-amd64 Host Operating System | Debian GNU/Linux 11 (bullseye) Update Channel | stable Supervisor Version | supervisor-2022.07.0

Remote:

• platform: xiaomi_miio name: "livingroom remote" hidden: true host: *** token: ***