-
-
Notifications
You must be signed in to change notification settings - Fork 558
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 the Xiaomi Induction Cooker #832
base: master
Are you sure you want to change the base?
Conversation
Hi basveeling, thanks for this works. for Build successfully; currently stuck on an issue with poetry/tox AttributeError: type object 'Callable' has no attribute '_abc_registry' <- I could use some advice here :-) : in my side build works. |
Any news on this issue? |
I'm willing to test it. It is working on my induction cooker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi and thanks for the PR! Looks like I forgot to submit this review looooong time ago for some reason, apologies for that... I hope that the inlined comments should help restructuring the code a bit, but it's currently just way too much to review piece-by-piece.
The biggest issue I have in its current form is that the code is too hard to read: there are too many magic numbers and direct index accesses (also outside the checksum calculation) that should be easier to define as contruct.Struct
s for maintainability.
Considering that at least part of the contents of those hex-encoded payloads are already known based on the code, it should not be too hard to convert them to use construct for parsing.
There is also #1018 (which aims to support only one model listed in your description). I think it would be great if you @basveeling and @sschirr could combine your efforts & knowledge on this topic :-) |
It seems this is a different device class (chunmi.cooker, whereas this PR concerns chunmi.ihcooker) and the multicooker seems closer to the rice cookers in operation. |
@rytilahti Thank you for the review! With a bit of delay on my side, I believe I have made most of the changes requested, outside of moving the bytestring building to Construct. I can take a look at that at a later point, I leave it up to you to decide if that is a blocking issue for this PR. In the mean time I'm happy to address any other issues, looking forward to hearing your thoughts! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @basveeling, no worries about delays, we are are volunteers here! :-) I added some more comments inline, my opinion is that it'd be better to convert to use construct for the payloads, as that will greatly simplify the code by avoiding plenty of conditionals & direct offset accesses.
Now, I'm not sure if there should be separate V1 and V2 structs (or a parameterized, shared one) defining the data structure, but as long as the access keys are shared among those, it should be easy to have a common interface for both.
@rytilahti Thank you for the feedback! A couple of changes in this new version.
|
Hi @basveeling and thanks for the update!
Great! Since #1160 we started to move integrations to their own packages, the idea being that contents of that directory do not spill over to other integrations. This will allow storing all code, auxiliary files and tests inside a single package. So I would suggest creating
Without reading the code yet, that sounds like a good approach 👍 If you don't mind reshuffling the parts prior I do a full review, that would be great! |
Hi @rytilahti, thanks for the suggestion. I've refactored the code to use the new dir structure. It's a large PR so I'd understand if it takes some time :-). Looking forward to your review! |
Hi Teemu / @rytilahti, I hope all is well. I see there's still a change requested status on this PR, but I think I've addressed all the comments. Just wanted to make sure it's clear this is ready for your review whenever you find the time :-). Enjoy the holiday period! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Bas, sorry, I completely forgot to submit some commentary I had written earlier... Thanks for the reminder! So there are just a couple of minor changes I'd like to see prior merging this, after those are done this is good to go 👍
edit: oh, also, do not forget to update README.md accordingly :-)
MODEL_EXP1: IHCooker, | ||
MODEL_FW: IHCooker, | ||
MODEL_TW1: IHCooker, | ||
MODEL_KOREA1: IHCooker, | ||
MODEL_HK1: IHCooker, | ||
MODEL_V1: IHCooker, | ||
MODEL_EG1: IHCooker, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these are correct for mdns? Either remove these completely for now, or check what format is being used in mdns on your device and just add them here as regular strings.
This information should be moved inside the integration itself at some point, but it'll require some more work. Having these here as strings makes it easy to copy them over if/when that time comes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They seem to be still there in the diff 👀
Hi! Following this issue since the start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move the directroy to be under cooker/chunmi/
(just in case some other "cooker" devices come around), and rebase on top of the current master? Otherwise this is good to go and should be a part of the next release.
MODEL_EXP1: IHCooker, | ||
MODEL_FW: IHCooker, | ||
MODEL_TW1: IHCooker, | ||
MODEL_KOREA1: IHCooker, | ||
MODEL_HK1: IHCooker, | ||
MODEL_V1: IHCooker, | ||
MODEL_EG1: IHCooker, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They seem to be still there in the diff 👀
I have chunmi.ihcooker.v2
|
@syssi where can I read about extracting react plugin from android (or jailbroken iPhone) device? |
what to do to reactivate this? |
@basveeling |
Any news on this merge for the cookers? |
This is a first attempt at supporting the Xiaomi Induction Cooker models (#491), which include models
chunmi.ihcooker.eg1 chunmi.ihcooker.exp1 chunmi.ihcooker.chefnic chunmi.ihcooker.hk1 chunmi.ihcooker.korea1 chunmi.ihcooker.tw1 chunmi.ihcooker.v1
Turns out the device is a bit more advanced than the app makes it appear. It supports 16-step recipes with each step having specific power, temperature, timer and next-phase settings. This enables building 'smart' recipes for e.g. automated rice cooking, kettle, pressure cooking, etc.
Supported features:
CookProfile
.To do:
AttributeError: type object 'Callable' has no attribute '_abc_registry'
<- I could use some advice here :-).Shout-out to @aquarat, @EUA and @hossF for initial analysis and @esgie for the plugin APK 👍
Here's a read-out of a rice-cooker program.