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 opentherm_gw services #17762

Merged
merged 4 commits into from Oct 31, 2018

Conversation

Projects
None yet
5 participants
@mvn23
Contributor

mvn23 commented Oct 24, 2018

Description:

Add services to the opentherm_gw component.

Related issue (if applicable): ref. #16670

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7059

Example entry for configuration.yaml (if applicable):

opentherm_gw:
  device: /dev/ttyUSB0

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • N/A
"""Set the clock on the OpenTherm Gateway."""
attr_date = call.data[ATTR_DATE]
attr_time = call.data[ATTR_TIME]
hass.async_add_job(gateway.set_clock, datetime.combine(attr_date,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 30, 2018

Member

Is gateway.set_clock a coroutine, an async safe callback or a sync method?

This comment has been minimized.

@mvn23

mvn23 Oct 30, 2018

Contributor

Like the other commands/services it's a coroutine that waits for a response from the gateway, but we don't need the return value here. That's why i awaited the others and added this one as an async job. Should I use another function instead of async_add_job?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 30, 2018

Member

If there's no reason to wait with scheduling it, just await the coroutine directly.

But yes, if you really want to schedule it on the queue, then use hass.async_create_task. But here we don't seem to need that.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 30, 2018

Member

Assuming there's no reason not to wait for the result I mean.

This comment has been minimized.

@mvn23

mvn23 Oct 30, 2018

Contributor

There is indeed no reason not to wait for the result as set_device_clock is already running in the event loop and it's the last statement in this function. I will change it to an await statement to avoid the overhead of async_add_job.

@MartinHjelmare

Looks good!

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 30, 2018

Please solve the merge conflict and we can merge.

@MartinHjelmare

MartinHjelmare requested changes Oct 30, 2018 edited

Now the git history of the old component was removed. It was better when we just moved the module to a package while keeping history. Is that impossible after the rebase?

if sensors:
await async_load_platform(hass, COMP_SENSOR, DOMAIN, sensors)
await async_load_platform(hass, COMP_SENSOR, DOMAIN, sensors, config)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 30, 2018

Member

We should create a task here too as above.

This comment has been minimized.

@mvn23

mvn23 Oct 30, 2018

Contributor

This was changed as per your request here in an earlier PR. I'll gladly change it back, just curious as to why?

@mvn23

This comment has been minimized.

Contributor

mvn23 commented Oct 30, 2018

Yeah I'm having a bit of a fight with git at the moment... Will solve it locally and push another commit later on.

@mvn23 mvn23 force-pushed the mvn23:opentherm_gw branch from 3ecae2f to 46e7907 Oct 30, 2018

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 30, 2018

Just create a task for the sensor platform loading and this should be good.

@MartinHjelmare

Great! Can be merged when build passes.

@mvn23

This comment has been minimized.

Contributor

mvn23 commented Oct 31, 2018

Looks like we're good to go! Thanks for the review.

@MartinHjelmare MartinHjelmare merged commit b12e79e into home-assistant:dev Oct 31, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 93.018%
Details

@wafflebot wafflebot bot removed the in progress label Oct 31, 2018

@mvn23 mvn23 deleted the mvn23:opentherm_gw branch Oct 31, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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