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

Support for pronto codes on chuangmi.remote.h102a03 #501

Closed
wants to merge 6 commits into from

Conversation

yawor
Copy link
Contributor

@yawor yawor commented Apr 15, 2019

A separate device class for ChuangmiRemote. It subclasses ChuangmiIr and overrides pronto_to_raw class method to apply new learned signal encoding scheme.
It requires two new dependencies: cython and heatshrink.
There's a Python 3 compatibility issue with current version of heatshrink on PyPi, as johan-sports/pyheatshrink#15 waits to be merged for quite some time now. Because of that I've forked the library and applied the patch from PR and I've added dependency on the fork. For the library to install automatically from the git, a pip>=18.1 is required, as that version added support for PEP 508 dependency specification. I've added a check to setup.py to make sure at least that version is installed before adding cython and heatshrink to install_requires (it may still not work correctly if python setup.py install is used instead of pip). I couldn't find a better way until heatshrink author merges the PR.
I'm also waiting for confirmation in #495 to be sure that signals generated by the new implementation work correctly.

'cython',
'heatshrink @ git+https://github.com/yawor/pyheatshrink@py3fix',
]
except:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not use bare 'except'

@@ -6,6 +6,10 @@
Struct, Const, Rebuild, this, len_, Adapter, Computed,
Int16ul, Int32ul, Int16ub, Array, BitStruct, BitsInteger,
)
try:
import heatshrink
except:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not use bare 'except'

@coveralls
Copy link

coveralls commented Apr 15, 2019

Coverage Status

Coverage increased (+0.1%) to 71.756% when pulling 47caa3b on yawor:chuangi_remote into 09500e8 on rytilahti:master.

@@ -145,6 +149,18 @@ def play(self, command: str):
return play_method(command, *command_args)


class ChuangmiRemote(ChuangmiIr):

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a short docstring to explain what type of device this is, and why it requires overloading? For future readers :-)

pkg_resources.get_distribution('pip>=18.1')
heatshrink_requirement = [
'cython',
'heatshrink @ git+https://github.com/yawor/pyheatshrink@py3fix',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add these to extras_require? I hope that there will be a new pypi release for heatshrink with your fix, so that we need no git paths here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that's not my fix. It was done by a different GitHub user some time ago, but the PR still waits to be merged. I've only created this fork to be 100% sure it won't suddenly disappear over night.
The extras_require may be a good option, but pip 18.1 or newer is still required to install patched version directly from git until a new version is released. Another way would be to temporarily publish a patched version to pypi under a different package name.

@yawor
Copy link
Contributor Author

yawor commented May 6, 2019

There's a chance for a new Heatshrink release on pypi. The PR has been merged few weeks ago, so I've asked if they could publish the new version.

@rytilahti
Copy link
Owner

Great! This fine to be merged from my side after the new release can be added into the extras_require.

@rytilahti
Copy link
Owner

@yawor any updates on this?

@syssi syssi changed the title Chuangi remote Support for pronto codes on chuangmi.remote.h102a03 Jun 2, 2019
@kiwikern
Copy link

I would love to use remote.v2 with pronto codes. Is there any way we can revive this PR? I'd be happy to help

oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
All kudos to original work by @yawor's on PR rytilahti#501.

Fixes rytilahti#495, fixes rytilahti#619, fixes rytilahti#811
Closes rytilahti#501
Partially covers rytilahti#1020
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
All kudos to original work by @yawor's on PR rytilahti#501.

Fixes rytilahti#495, fixes rytilahti#619, fixes rytilahti#811
Closes rytilahti#501
Partially covers rytilahti#1020
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
All kudos to original work by @yawor's on PR rytilahti#501.

Fixes rytilahti#495, fixes rytilahti#619, fixes rytilahti#811.
Closes rytilahti#501.
Partially covers rytilahti#1020.
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 22, 2021
All kudos to original work by @yawor's on PR rytilahti#501.

Fixes rytilahti#495, fixes rytilahti#619, fixes rytilahti#811.
Closes rytilahti#501.
Partially covers rytilahti#1020.
@rytilahti
Copy link
Owner

Obsoleted by #1021 - thanks for the PR and groundwork on this @yawor! :-)

@rytilahti rytilahti closed this Apr 24, 2021
oblitum added a commit to oblitum/python-miio that referenced this pull request Apr 24, 2021
All kudos to original work by @yawor's on PR rytilahti#501.

Fixes rytilahti#495, fixes rytilahti#619, fixes rytilahti#811.
Closes rytilahti#501.
Partially covers rytilahti#1020.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants