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

Air Conditioning Companion: Rewrite a captured command before replay #317

Merged

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented May 10, 2018

@coveralls
Copy link

coveralls commented May 10, 2018

Coverage Status

Coverage increased (+0.1%) to 72.384% when pulling 8b13cb8 on syssi:feature/airconditioningcompanion-ir-codes into 551687b on rytilahti:master.

@syssi syssi requested a review from rytilahti May 10, 2018 16:42
default_output=format_output("Sending the supplied infrared command")
)
def send_ir_code(self, command: str):
def send_ir_code(self, model: str, code: str):
Copy link
Contributor

@yawor yawor May 10, 2018

Choose a reason for hiding this comment

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

I'm not a fan of working on a hex string. I think it would be better to convert both model and code strings to bytearrays at the beginning and work on them. Then at the end, convert it back to a hex string.

code[26:28] + code[28:32] + '27'

checksum = sum([int(command[i:i + 2], 16) for i in range(0, len(command), 2)])
checksum = "{:02X}".format(checksum % 256)
Copy link
Contributor

Choose a reason for hiding this comment

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

checksum & 0xFF produces the same value as checksum % 256, but should be much faster (bit-wise operation vs arithmetic one).

@@ -131,7 +131,7 @@ def test_learn_stop(self):
assert self.device.learn_stop() is True

def test_send_ir_code(self):
assert self.device.send_ir_code('0000000') is True
assert self.device.send_ir_code(bytes.fromhex('010500978022222102'), bytes.fromhex('00')) is True

Choose a reason for hiding this comment

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

line too long (105 > 100 characters)

slot = bytes([121 + slot])

# FE + 0487 + 00007145 + 9470 + 1FFF + 7F + FF + 06 + 0042 + 27 + 4E + 0025002D008500AC01...
command = code[0:1] + model[2:8] + b'\x94\x70\x1F\xFF' + slot + b'\xFF' + code[13:16] + b'\x27'

Choose a reason for hiding this comment

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

line too long (103 > 100 characters)

@@ -131,7 +131,7 @@ def test_learn_stop(self):
assert self.device.learn_stop() is True

def test_send_ir_code(self):
assert self.device.send_ir_code('0000000') is True
assert self.device.send_ir_code(bytes.fromhex('010500978022222102'), bytes.fromhex('00')) is True

Choose a reason for hiding this comment

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

line too long (105 > 100 characters)

slot = bytes([121 + slot])

# FE + 0487 + 00007145 + 9470 + 1FFF + 7F + FF + 06 + 0042 + 27 + 4E + 0025002D008500AC01...
command = code[0:1] + model[2:8] + b'\x94\x70\x1F\xFF' + slot + b'\xFF' + code[13:16] + b'\x27'

Choose a reason for hiding this comment

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

line too long (103 > 100 characters)

@@ -86,7 +86,8 @@ def test_status(self):

assert self.is_on() is False
assert self.state().load_power == 2
assert self.state().air_condition_model == '010500978022222102'
assert self.state().air_condition_model == \
bytes.fromhex('010500978022222102')

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent


# FE + 0487 + 00007145 + 9470 + 1FFF + 7F + FF + 06 + 0042 + 27 + 4E + 0025002D008500AC01...
command = code[0:1] + model[2:8] + b'\x94\x70\x1F\xFF' + \
slot + b'\xFF' + code[13:16] + b'\x27'

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

@@ -131,7 +132,8 @@ def test_learn_stop(self):
assert self.device.learn_stop() is True

def test_send_ir_code(self):
assert self.device.send_ir_code('0000000') is True
assert self.device.send_ir_code(bytes.fromhex('010500978022222102'),
bytes.fromhex('00')) is True

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

@syssi
Copy link
Collaborator Author

syssi commented May 10, 2018

@yawor The bytearray variant is a bit hard to read now. I'm doing it correct?

default_output=format_output("Sending the supplied infrared command")
)
def send_ir_code(self, model: str, code: str, slot: int=0):
def send_ir_code(self, model: bytes, code: bytes, slot: int=0):
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking about keeping strings here as args but converting them to bytearray/bytes right away. Having bytes as arg type for command line is probably not very good idea :). Maybe there should be two methods? Main one taking bytes like this one, but the @command should be applied to a wrapper method, which takes hex strings, converts them and calls the bytes one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Optionally (maybe even better solution) would be to add converting functions to click arguments definition to convert hex strings from command line to bytes.

@yawor
Copy link
Contributor

yawor commented May 11, 2018

@syssi seems to be OK. I would only do something about click args, because it'll probably be hard to enter raw bytes on command line. Unless click is smart enough to do this conversion automatically for bytes type.

@syssi syssi force-pushed the feature/airconditioningcompanion-ir-codes branch from 16bb6e4 to c3ce22c Compare May 13, 2018 13:03
@@ -1,4 +1,7 @@
import base64

Choose a reason for hiding this comment

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

'base64' imported but unused


try:
code = bytes.fromhex(code)
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'

"""
try:
model = bytes.fromhex(model)
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'

else:
return d

with open(os.path.join(os.path.dirname(__file__),

Choose a reason for hiding this comment

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

expected 2 blank lines after class or function definition, found 1

@syssi syssi merged commit 395fcb6 into rytilahti:master May 19, 2018
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.

4 participants