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

Add timers to xiaomi_miio vacuum #35417

Merged
merged 5 commits into from
Jun 12, 2020

Conversation

MarBra
Copy link
Contributor

@MarBra MarBra commented May 9, 2020

Proposed change

I like to add the list of configured timers as an attribute for a vacuum.
So that users can display them in the UI.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

  • Documentation added/updated for [www.home-assistant.io][docs-repository]

@homeassistant
Copy link
Contributor

Hi @MarBra,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link

This pull request needs to be manually signed off by @home-assistant/core before it can get merged.
(message by ReviewEnforcer)

@probot-home-assistant
Copy link

Hey there @rytilahti, @syssi, mind taking a look at this pull request as its been labeled with a integration (xiaomi_miio) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

Copy link
Member

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

The PR looks fine to me (without testing, as I don't use timers myself). This will introduce a new I/O req per update cycle, but I don't think that's a problem with these devices. When/if support for new vacuums will be introduced in the backend lib, this may need readjustments.

@MarBra MarBra force-pushed the mi_robot_timers branch from 02e87e4 to cf95051 Compare May 23, 2020 14:12
local_tz = timezone(self.timezone)

for timer in self._timers:
cron = croniter(timer.cron, start_time=local_tz.localize(datetime.now()))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to move this parsing to the backend library, do you agree? I think the data structure is fine, although you may want to expose the ID also, just in case if the API gets improved at some point to delete timers / toggle their state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree. Sounds good, I will create a pull request in python-miio library.

Copy link
Member

Choose a reason for hiding this comment

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

@MarBra I just release python-miio 0.5.1, feel free to continue on this PR! :)

@balloob balloob marked this pull request as draft June 3, 2020 16:46
Copy link
Member

@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.

Requesting changes to mark correct status. See above.

@MarBra MarBra force-pushed the mi_robot_timers branch 2 times, most recently from bda3411 to 4ca5462 Compare June 7, 2020 17:04
Copy link
Member

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Looks fine for me. I'm just wondering if the API is a bit "off" when/if homeassistant needs to import pytz simply to display this information?

Btw, the PR is still marked as a draft.

@MarBra MarBra force-pushed the mi_robot_timers branch from 635f5c9 to 26dfff7 Compare June 11, 2020 18:33
@MarBra MarBra marked this pull request as ready for review June 11, 2020 18:47
@MarBra MarBra requested a review from MartinHjelmare June 11, 2020 18:47
Copy link
Member

@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.

Looks good!

@lpfann
Copy link

lpfann commented Jul 14, 2020

This merge broke the integration for xiaomi devices for some people:
#37610

I looked into it am not sure if its an upstream bug or if this PR is buggy.
rytilahti/python-miio#759

Maybe @MarBra could look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants