Add support for chuangmi.remote.h102a03 and chuangmi.remote.v2

[oblitum]

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

[oblitum]

Not much idea how about fixing Tests PyPy Ubuntu.

[rytilahti]

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).

[oblitum]

• 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
• Cython makes assumptions about the internal structure of arrays preventing use with pypy cython/cython#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.

[oblitum]

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

[oblitum]

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

[rytilahti]

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.

[oblitum]

@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.

[rytilahti]

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")

[rytilahti]

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)?

[oblitum]

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

[rytilahti]

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.

[oblitum]

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.

[rytilahti]

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]

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():
    ...

[oblitum]

@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.

[rytilahti]

Considering the biggest change between these remotes is just the way they expect the payload to be encoded, I think it would be better to avoid creating separate classes, but also a parameter (model) like done on many other devices and choose the encoding function based on that.

What do you think?

[rytilahti]

Suggested change

	"""Class representing new type of Chuangmi IR Remote Controller identified by model
	"chuangmi-remote-h102a03_".
	"""Class representing Chuangmi IR Remote Controller (chuangmi-remote-h102a03_).

Is info() working fine for this device? If yes, could you also add that here. The long term goal is to separate the meta information to separate files to allow creating instances based on the model information.

[oblitum]

I can only confirm that it works for chuangmi-remote-v2, and it does. What should be added? I didn't get. Do you wish to add that info() works for the device in the class description?

[rytilahti]

The info will deliver the model of the device, could you check with miiocli device info what is it for your device & add that to the corresponding class?

The reason why I'm pushing towards a single class is that it makes it simpler for downstreams like homeassistant to support all supported remotes as long as 1) info is working or 2) the user provides the wanted model as a parameter so that the class can adjust itself accordingly.

[oblitum]

I get:

Model: chuangmi.remote.v2
Hardware version: esp32

Firmware version: 2.0.6_0006

The ChuangmiRemoteV2 child class already states to cover "chuangmi-remote-v2". Do you wish it changed to "chuangmi.remote.v2" according to the model output above? (The default hostname on the network doesn't use periods though, but the actual model name is with periods). I can't confirm anything for the h102a03 model.

[rytilahti]

Ah, yes, it's also in the ttile of the PR, sorry... The one reported by the info is the one to use for identification (as not all devices support the mDNS, which is used for the hostname).

[rytilahti]

Suggested change

	"""Takes a Pronto Hex encoded IR command and number of repeats and returns a
	tuple containing a string encoded IR signal accepted by controller and
	frequency. Supports only raw Pronto format, starting with 0000.
	"""Convert pronto to device-expected format.
	Takes a Pronto Hex encoded IR command and number of repeats and returns a
	tuple containing a string encoded IR signal accepted by controller and
	frequency. Supports only raw Pronto format, starting with 0000.

[rytilahti]

The docstring above mentions that only prontos starting with 0000 are supported. I think an exception should also be raised if the input is in invalid format.

[rytilahti]

Also, the another class expects that repeats >= 0, that should also be the case here?

[rytilahti]

See my comment above on simplifying the docstring & adding the model info.

[oblitum]

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.

[oblitum]

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]

@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
>>>> 

[oblitum]

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.

[oblitum]

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

[oblitum]

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

[rytilahti]

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.

[oblitum]

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.

[oblitum]

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.

[oblitum]

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.

[yawor]

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.

[yawor]

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.

[rytilahti]

@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)?

[yawor]

@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.

[yawor]

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

[rytilahti]

Nice work! 💯 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.

[yawor]

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.

[iboolka]

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: ***