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

Fix smart task start action response dimmer attribute #614

Merged

Conversation

blast-hardcheese
Copy link
Contributor

Resolves #610

Also fleshed out some examples for how to interact with SmartTasks, happy to revert some or all of those if undesired.

Thanks for this library!

@MartinHjelmare MartinHjelmare changed the title #610: Not all SmartTasks have associated dimmers Fix smart task start action response dimmer attribute Jan 16, 2023
if socket:
print("> sockets")
print("> socket.socket_control.sockets")
print("> api(socket.socket_control.set_values({ const.ATTR_REPEAT_DAYS: 0b1111111 }))")
Copy link
Contributor

@MartinHjelmare MartinHjelmare Jan 16, 2023

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

.github/workflows/test.yml Outdated Show resolved Hide resolved
@ggravlingen
Copy link
Member

The linting errors in the 3.9 run seem unrelated to the changes made here. Should I resolve this in another PR @MartinHjelmare?

  lint: commands[2]> pylint examples pytradfri tests
  ************* Module example_async
  examples/example_async.py:26:0: I0021: Useless suppression of 'import-error' (useless-suppression)
  ************* Module example_cover_async
  examples/example_cover_async.py:26:0: I0021: Useless suppression of 'import-error' (useless-suppression)
  ************* Module debug_info
  examples/debug_info.py:28:0: I0021: Useless suppression of 'import-error' (useless-suppression)
  ************* Module example_pair
  examples/example_pair.py:31:0: I0021: Useless suppression of 'import-error' (useless-suppression)
  examples/example_pair.py:137:0: I0021: Useless suppression of 'unsupported-membership-test' (useless-suppression)
  ************* Module example_sync
  examples/example_sync.py:27:0: I0021: Useless suppression of 'import-error' (useless-suppression)
  ************* Module example_socket_async
  examples/example_socket_async.py:26:0: I0021: Useless suppression of 'import-error' (useless-suppression)
  ************* Module example_air_purifier_async
  examples/example_air_purifier_async.py:26:0: I0021: Useless suppression of 'import-error' (useless-suppression)
  ************* Module pytradfri.__main__
  pytradfri/__main__.py:9:0: W0611: Unused const imported from pytradfri (unused-import)
  pytradfri/__main__.py:9:0: I0021: Useless suppression of 'import-error' (useless-suppression)

@blast-hardcheese blast-hardcheese force-pushed the resolve-610 branch 2 times, most recently from c8eb1d0 to 6b7e12c Compare January 16, 2023 18:08
@blast-hardcheese
Copy link
Contributor Author

blast-hardcheese commented Jan 16, 2023

So, the underlying bug was resolved with 773aca7.

I think 6b7e12c fits in line with this library, and is the underlying task that I was trying to achieve with my project here, so I figured adding it made sense.

@blast-hardcheese
Copy link
Contributor Author

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}]},
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare MartinHjelmare merged commit ac1d750 into home-assistant-libs:master Jan 17, 2023
@blast-hardcheese blast-hardcheese deleted the resolve-610 branch January 18, 2023 05:42
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.

"1 validation error for SmartTaskResponse"
3 participants