Refactoring: Split Device class into Device+Protocol by petrkotek · Pull Request #592 · rytilahti/python-miio

petrkotek · 2019-12-08T09:41:55Z

Before adding support for the first MIoT device (#585) I thought it would be good to do a little bit of refactoring (i.e. to make the #585 smaller).

This PR extracts command-sending functionality from Device class to new Protocol class.

Basically the send(), discover() and do_discover() methods were moved from Device to Protocol
Protocol is instantiated in Device constructor.
Protocol#send() is deprecated & forwards the commands to CommandSender.

Note: tried discovery and looks to be working fine:

$ python miio/vacuum_cli.py discover
INFO:miio.discovery:Discovering devices with mDNS, press any key to quit...
INFO:miio.discovery:Found a supported 'Vacuum' at 192.168.0.10 - token: ffffffffffffffffffffffffffffffff
/Users/petrkotek/Workspace/python-miio/miio/discovery.py:177: UserWarning: Please consider using python-yeelight for more complete support.
  dev = device_cls(ip=addr)
INFO:miio.discovery:Found a supported 'Yeelight' at 192.168.0.17 - token: 00000000000000000000000000000000
INFO:miio.discovery:Found a supported 'AirQualityMonitor' at 192.168.0.2 - token: bb835d94f8e75c74a231e445c9716616
INFO:miio.discovery:Found a supported 'Yeelight' at 192.168.0.9 - token: 00000000000000000000000000000000

Edit: renames CommandSender to Protocol

…dSender class to separate concerns

coveralls · 2019-12-08T10:23:47Z

Coverage increased (+0.08%) to 73.783% when pulling db0f53a on petrkotek:command-sender into 20413c2 on rytilahti:master.

petrkotek · 2019-12-11T06:22:44Z

@rytilahti Looking forward to hear your feedback when you have a bit of time to review this. Thank you!

petrkotek · 2019-12-16T03:24:16Z

@rytilahti friendly ping in case you missed it. If you're busy, that's ok :)

rytilahti

Hi and sorry for the delay, I added some comments, but my concerns are mostly:

Users of the device classes should have no need to know about implementation details, that's why the protocol handling should simply by wrapped and invisible to users. (concerns send() etc.).
For tests, simply pass the existing self.return_values to the dummy implementation directly afterwards. This will keep the history cleaner as there is no real reason to overwrite those parts from my understanding.

Ping @syssi

miio/command_sender.py

miio/device.py

miio/discovery.py

rytilahti · 2019-12-17T01:14:40Z

miio/tests/test_airconditioningcompanion.py

+                "send_cmd": lambda x: self._send_input_validation(x),
+                "set_power": lambda x: self._set_power(x),
+            }
+        )


Please keep the self.return_values for now, and simply pass it as a parameter to the implementation. This will keep the diff much more readable and there is no real need to modify these parts at the moment (imo).

I was thinking that we don't really need the self.return_values anymore and wanted to clean it up.

The diff is a bit smaller when ignoring whitespaces :badpokerface: https://github.com/rytilahti/python-miio/pull/592/files?w=1

Is that alright?

Honestly I'd prefer it that'd be done in a separate PR. This PR contains already so many lines to review, and as these are tests, not having return_values does not really bring much.

Fair enough. I'm initialising the DummyProtocol in DummyDevice now and passing in self so it can access return_values. That way the diff is indeed smaller.

Had to add call to the parent constructor for some dummy devices in tests and add inheritance from DummyDevice, but that should faily uncontroversial :)

miio/vacuum.py

Rename do_discover() to send_handshake() & create a proxy method send_handshake() in Device class

petrkotek

pushed additional commits to address most of the feedback (otherwise commented with a question)

miio/device.py

miio/command_sender.py

miio/device.py

miio/discovery.py

petrkotek · 2020-01-07T13:45:50Z

miio/tests/test_airconditioningcompanion.py

+                "send_cmd": lambda x: self._send_input_validation(x),
+                "set_power": lambda x: self._set_power(x),
+            }
+        )


I was thinking that we don't really need the self.return_values anymore and wanted to clean it up.

The diff is a bit smaller when ignoring whitespaces :badpokerface: https://github.com/rytilahti/python-miio/pull/592/files?w=1

Is that alright?

miio/vacuum.py

petrkotek · 2020-01-10T18:39:39Z

Actioned the 2nd round of review. Hope it looks better now.

Thank you for looking into this, @rytilahti!

rytilahti

This start to look good, thanks for your efforts! I'm unsure if cleaning up the DeviceException import should be a part of this, but it's not a big deal. See my comments inline, I haven't yet tested this locally on my devices (I'll do that later this weekend), but otherwise it's almost ready to be merged :-)

miio/device.py

rytilahti · 2020-01-10T20:06:06Z

miio/device.py

        :param str command: Command to send
        :param dict parameters: Parameters to send"""
-        return self.send(command, parameters)
+        return self.protocol.send(command, parameters)


I'm starting to have second thoughts on deprecating the send. This code would generally be much cleaner if we'd keep it as it is, and simply make it wrap the call to the private protocol. Thoughts on this? What are the pros for deprecating it (and making all other methods more complicated)?

I guess the main reason why I decided to do that was to do less inheritance and more composition (to make things easier to follow). Keeping send, however, makes it more backwards compatible.

I've removed the deprecation.

miio/protocol.py

…od to keep everything compatible. Protocol: update docstring to reflect new code. Vacuum: expose raw_id from the Protocol.

petrkotek · 2020-01-11T11:54:31Z

Done addressing the latest feedback.

Tested on my Air Quality Monitor:

(python-miio) bash-3.2$ python miio/cli.py airqualitymonitor --ip $AIR_IP --token $TOKEN status
Power: on
USB power: True
Battery: 100
AQI: 4
Temperature: None
Humidity: None
CO2: None
CO2e: None
PM2.5: None
TVOC: None
Display clock: False
(python-miio) bash-3.2$ python miio/cli.py airqualitymonitor --ip $AIR_IP --token $TOKEN raw_command get_prop "['power', 'aqi', 'battery', 'usb_state', 'time_state', 'night_state', 'night_beg_time', 'night_end_time', 'sensor_state']"
Running command raw_command
['on', 3, 100, 'on', 'off', 'on', 79200, 25200, 'ready']

Discovery worked too:

(python-miio) bash-3.2$ python miio/vacuum_cli.py discover
INFO:miio.discovery:Discovering devices with mDNS, press any key to quit...
INFO:miio.discovery:Found a supported 'AirQualityMonitor' at 192.168.0.2 - token: bb835d94<REDACTED>

Vacuum (S50)

$ python miio/cli.py vacuum --ip $AIR_IP --token $TOKEN status
Running command status
<VacuumStatus state=Charging, error=No error bat=100%, fan=100% cleaned 14.585 m² in 0:14:25>

I.e. what I've got works fine, however, I don't have any other devices handy, so further testing will be appreciated!

rytilahti

I just tested this with the 1st gen vacuum and a powerstrip (zimi.powerstrip.v2). In my opinion this can be merged now, but I'd still like to have the MiIO protocol bits in a separate file already. This will avoid moving the pieces again shortly after introducing the support for miot and keeping the diffs more comprehensible for the future :-)

rytilahti · 2020-01-12T15:26:23Z

miio/protocol.py

 )
+
+
+class Protocol:


Could you move this to its own file (thus, avoid moving this again soonish and polluting the git logs), I'd say miioprotocol.py would be a good candidate.

✅ Moved Protocol to miioprotocol.py as MiIOProtocol

Great, thanks for the effort! 🎉

Extract command-sending functionality from Device class to new Comman…

818089f

…dSender class to separate concerns

petrkotek force-pushed the command-sender branch from 3d6c506 to 818089f Compare December 8, 2019 10:22

petrkotek marked this pull request as ready for review December 8, 2019 10:36

petrkotek mentioned this pull request Dec 16, 2019

Add basic support for Xiaomi Mi Air Purifier 3/3H #585

Closed

2 tasks

rytilahti requested changes Dec 17, 2019

View reviewed changes

rytilahti requested a review from syssi December 17, 2019 01:19

Petr Kotek added 2 commits January 8, 2020 01:38

Rename CommandSender class to Protocol to better describe what it is

41e4812

Rename do_discover() to send_handshake() & create a proxy method send_handshake() in Device class

Device: bring back ip and token

5cec45c

petrkotek commented Jan 7, 2020

View reviewed changes

petrkotek changed the title ~~Refactoring: Split Device class into Device+CommandSender~~ Refactoring: Split Device class into Device+Protocol Jan 9, 2020

Petr Kotek added 2 commits January 9, 2020 20:50

Merge branch 'master' into command-sender

78ed81b

airfresh_t2017.py: fix tests after merge

61afef5

petrkotek force-pushed the command-sender branch from dc84f3a to 61afef5 Compare January 9, 2020 09:58

petrkotek requested a review from rytilahti January 9, 2020 10:01

Petr Kotek added 2 commits January 11, 2020 05:30

tests: bring back return_values to simplify the diff

d8399a2

Device: update deprecation message

8509d4d

petrkotek force-pushed the command-sender branch from b382423 to 8509d4d Compare January 10, 2020 18:31

rytilahti reviewed Jan 10, 2020

View reviewed changes

Device: make protocol field private; don't deprecate Device.send meth…

968f8e1

…od to keep everything compatible. Protocol: update docstring to reflect new code. Vacuum: expose raw_id from the Protocol.

petrkotek force-pushed the command-sender branch from 394cb50 to 968f8e1 Compare January 11, 2020 11:42

rytilahti approved these changes Jan 12, 2020

View reviewed changes

Rename Protocol class to MiIOProtocol

db0f53a

rytilahti merged commit 9a7c6c4 into rytilahti:master Jan 13, 2020

petrkotek deleted the command-sender branch January 13, 2020 06:06

petrkotek mentioned this pull request Jan 13, 2020

Support for Xiaomi Air Purifier 3 (zhimi.airpurifier.ma4) #577

Closed

Uh oh!

Conversation

petrkotek commented Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrkotek commented Dec 11, 2019

Uh oh!

petrkotek commented Dec 16, 2019

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrkotek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrkotek commented Jan 10, 2020

Uh oh!

rytilahti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrkotek commented Jan 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

petrkotek commented Dec 8, 2019 •

edited

Loading

coveralls commented Dec 8, 2019 •

edited

Loading

petrkotek left a comment •

edited

Loading

rytilahti left a comment •

edited

Loading

petrkotek commented Jan 11, 2020 •

edited

Loading