-
Notifications
You must be signed in to change notification settings - Fork 130
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
Fix smart task start action response dimmer attribute #614
Fix smart task start action response dimmer attribute #614
Conversation
7ec3564
to
fd84f1a
Compare
pytradfri/__main__.py
Outdated
if socket: | ||
print("> sockets") | ||
print("> socket.socket_control.sockets") | ||
print("> api(socket.socket_control.set_values({ const.ATTR_REPEAT_DAYS: 0b1111111 }))") |
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.
We don't use the const module above. I think we should be consistent.
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.
Do we need to change __main__.py
to fix this bug? I don't mind appending this file, as it makes the library easier to use, but perhaps in another PR?
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 can do it in another PR, yes. I have been iterating just trying to get the tests to pass, I didn't realize there was already activity in this PR. I've stopped updating so far, but happy to move it out.
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.
OK. I split out #615 from this. I will force push to reduce down to just the bugfix.
19e06d7
to
cbdf217
Compare
The linting errors in the 3.9 run seem unrelated to the changes made here. Should I resolve this in another PR @MartinHjelmare?
|
c8eb1d0
to
6b7e12c
Compare
6b7e12c
to
773aca7
Compare
OK, this and #615 are ready for review. Sorry for the noise, I was having difficulty getting the tests running locally, I was using GitHub Actions to run tests in my fork. |
"9040": 2, | ||
"9041": 127, | ||
"9042": {"5850": 1, "15013": [{"9003": 65553}]}, | ||
"9043": {"5850": 0, "15013": [{"9003": 65553}]}, |
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.
This is the response captured in the issue. Do we know what 9043 represents? We don't have that one in our const
module.
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'm unable to test using the app for the next few weeks unfortunately, I suspect it relates to controlling an outlet via smart task. Is there a way I can confirm this somehow, given my setup?
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 know, but if you have time later to dig in a bit, perhaps try removing the outlet device from the gateway and then reset the gateway and/or app. Then try adding a new smart task before adding the outlet device again and see if the response is different.
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!
Resolves #610
Also fleshed out some examples for how to interact with
SmartTask
s, happy to revert some or all of those if undesired.Thanks for this library!