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

Improve Nuki lock #22888

Merged
merged 1 commit into from Jul 16, 2019

Conversation

@franfos
Copy link
Contributor

commented Apr 8, 2019

Breaking Change:

The lock.nuki_unlatch service has been removed. It has been replaced by the lock.open service. The lock.nuki_lock_n_go service has been renamed to nuki.lock_n_go. Users that are using the removed or renamed services in automations need to replace them with the new services.

Description:

Nuki lock component improvements:

  • Bride port: The port parameter was not forwarded on bridge component initialization.
  • The default timeout has been changed from 5s to 20s.
  • available attribute: Implemented entity available attribute. It checks if the communication HA <-> bridge and bridge <-> lock is fine.
  • Check connection service: If, by any reason, the connection between the bridge and the lock was lost, there was no way to detect it. There is now a new service to force the communication between the bridge and the lock, instead of retrieving the last know state. If that communication fails, the attribute lock_reachable will become false.
  • Open service implemented (previous nuki_unlatch)

Docs PR:

home-assistant/home-assistant.io#9841

Example entry for configuration.yaml:

lock:
  - platform: nuki
    host: 192.168.0.85
    port: 8080
    token: [token]

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

  • Documentation added/updated in home-assistant.io(waiting to be approved this PR to change the documentation)

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

  • The manifest file has all fields filled out correctly (example).
  • New dependencies have been added to requirements in the manifest (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:

  • Tests have been added to verify that the new code works.
@github-actions

This comment has been minimized.

Copy link

commented Apr 8, 2019

Hey there @pschmitt, mind taking a look at this pull request as its been labeled with a integration (nuki) you are listed as a codeowner for? Thanks!

@robbiet480

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@franfos Was there a reason you closed that other PR in favor of this one?

@franfos

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@franfos Was there a reason you closed that other PR in favor of this one?

Because of the failed checks. There was some syntax failures and one missing modified file (manifest)

@franfos

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Hi,
It still has some syntax issues (extra empty lines), but before solve them, I would like to have some feedback about the PR. @pschmitt, could you comment if you see any problem/change/improvement?
Thanks in advance.

@codecov

This comment was marked as off-topic.

Copy link

commented May 3, 2019

Codecov Report

Merging #22888 into dev will decrease coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22888      +/-   ##
==========================================
- Coverage   94.07%   93.83%   -0.25%     
==========================================
  Files         536      448      -88     
  Lines       41045    36528    -4517     
==========================================
- Hits        38614    34275    -4339     
+ Misses       2431     2253     -178
Impacted Files Coverage Δ
homeassistant/components/owntracks/config_flow.py 44.82% <0%> (-41.66%) ⬇️
homeassistant/components/mqtt/fan.py 74.33% <0%> (-23.64%) ⬇️
homeassistant/bootstrap.py 58.08% <0%> (-22.67%) ⬇️
homeassistant/components/mqtt/light/schema_json.py 73.06% <0%> (-20.65%) ⬇️
...omeassistant/components/locative/device_tracker.py 87.5% <0%> (-10.18%) ⬇️
homeassistant/components/config/script.py 90% <0%> (-10%) ⬇️
...omeassistant/components/geofency/device_tracker.py 86.66% <0%> (-9.11%) ⬇️
homeassistant/components/mqtt/lock.py 91.83% <0%> (-8.17%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
homeassistant/components/hassio/ingress.py 71.3% <0%> (-6.75%) ⬇️
... and 333 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60fe4c9...3c380f9. Read the comment docs.

@franfos

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Hi,
all the conflicts have been solved. What's the next step? Is it someone monitoring this PR?
Thanks.

@robbiet480

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@franfos Someone (preferably with experience developing for Nuki) needs to approve this PR. You already alerted @pschmitt, hopefully they respond soon, otherwise someone else will come along to review it soon.

@franfos

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@franfos Someone (preferably with experience developing for Nuki) needs to approve this PR. You already alerted @pschmitt, hopefully they respond soon, otherwise someone else will come along to review it soon.

Thanks!

@nexxoc

This comment has been minimized.

Copy link

commented Jun 4, 2019

@franfos : Can't help you with coding (yet) but please keep on going what you're doing here. I also need the timeout config for my nuki to work with hassio (5s is too little).

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

For the future, different features or fixes should go in different PRs. Multiple small PRs are always preferred over one bigger. With this PR a comment and discussion about one feature will hold up all the other features.

@franfos

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

For the future, different features or fixes should go in different PRs. Multiple small PRs are always preferred over one bigger. With this PR a comment and discussion about one feature will hold up all the other features.

Ok, understood

homeassistant/components/nuki/lock.py Outdated Show resolved Hide resolved
homeassistant/components/nuki/lock.py Outdated Show resolved Hide resolved
homeassistant/components/nuki/lock.py Outdated Show resolved Hide resolved
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Please rebase on latest dev branch to let the build pass.

@MartinHjelmare
Copy link
Member

left a comment

Great!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Can be merged when build passes.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@franfos please check if our docs need updating.

@franfos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

@franfos please check if our docs need updating.

Ok

@franfos franfos referenced this pull request Jul 12, 2019
2 of 2 tasks complete
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

Please rebase on latest dev branch to let the build pass.

20s default timeout
Using port on bridge initialization
Service: check_connection
Attribute: available
Updated requeriments_all.txt
Change unlatch service for open service

Removed extra info

nuki_lock_n_go renamed to lock_n_go
nuki_check_connection renamed to check_connection

@franfos franfos force-pushed the franfos:nuki_lock_improvements2 branch from 7579c20 to 3678dd4 Jul 14, 2019

@MartinHjelmare MartinHjelmare changed the title Nuki lock improvements Improve Nuki lock Jul 16, 2019

@MartinHjelmare MartinHjelmare merged commit 4afc19f into home-assistant:dev Jul 16, 2019

9 checks passed

CI Build #20190714.27 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pytlint) FullCheck Pytlint succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python35) Tests PyTest Python35 succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA

KJonline added a commit to Rendili/home-assistant that referenced this pull request Jul 17, 2019

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant: (156 commits)
  Add HmIP-PCBS2, HmIP-PCBS-BAT to Homematic IP Cloud (home-assistant#25201)
  Simplify cache restore (home-assistant#25186)
  Only include target temp if has right support flag (home-assistant#25193)
  Fix issue home-assistant#24495 (home-assistant#25199)
  Handle somfy expired token (home-assistant#25195)
  Add login_method config option to fix login issue with RouterOS Version > 6.43 (home-assistant#25194)
  Add HMIP-FCI / HMIP-FBL / HmIP-BBL (home-assistant#25188)
  [climate] Tweak evohome migration (home-assistant#25187)
  Fix device name customization on ZHA add devices page (home-assistant#25180)
  Upgrade mypy to 0.720, turn on unreachability warnings (home-assistant#25157)
  Use MockConfigEntry (home-assistant#25190)
  Add severe weather sensor to Dark Sky (home-assistant#22701)
  Fix typo in azure-pipelines-ci.yml
  Raise not ready when no data from API is retrieved (home-assistant#25182)
  Improve Nuki lock (home-assistant#22888)
  Delete config.yml (home-assistant#25181)
  Upgrade luftdaten to 0.6.2 (home-assistant#25177)
  Upgrade Mastodon.py to 1.4.5 (home-assistant#25176)
  Upgrade ruamel.yaml to 0.15.99 (home-assistant#25175)
  Upgrade discord.py to 1.2.3 (home-assistant#25174)
  ...

KJonline added a commit to Rendili/home-assistant that referenced this pull request Jul 17, 2019

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into hive_water_heater

* 'dev' of https://github.com/home-assistant/home-assistant: (49 commits)
  Pin Docker to Debain Stretch (home-assistant#25206)
  Updated frontend to 20190717.0
  Add HmIP-PCBS2, HmIP-PCBS-BAT to Homematic IP Cloud (home-assistant#25201)
  Simplify cache restore (home-assistant#25186)
  Only include target temp if has right support flag (home-assistant#25193)
  Fix issue home-assistant#24495 (home-assistant#25199)
  Handle somfy expired token (home-assistant#25195)
  Add login_method config option to fix login issue with RouterOS Version > 6.43 (home-assistant#25194)
  Add HMIP-FCI / HMIP-FBL / HmIP-BBL (home-assistant#25188)
  [climate] Tweak evohome migration (home-assistant#25187)
  Fix device name customization on ZHA add devices page (home-assistant#25180)
  Upgrade mypy to 0.720, turn on unreachability warnings (home-assistant#25157)
  Use MockConfigEntry (home-assistant#25190)
  Add severe weather sensor to Dark Sky (home-assistant#22701)
  Fix typo in azure-pipelines-ci.yml
  Raise not ready when no data from API is retrieved (home-assistant#25182)
  Improve Nuki lock (home-assistant#22888)
  Delete config.yml (home-assistant#25181)
  Upgrade luftdaten to 0.6.2 (home-assistant#25177)
  Upgrade Mastodon.py to 1.4.5 (home-assistant#25176)
  ...

# Conflicts:
#	homeassistant/components/hive/water_heater.py
@balloob balloob referenced this pull request Aug 7, 2019
@Jalleronline

This comment has been minimized.

Copy link

commented Aug 8, 2019

Nice to have the timeout to 20s but I am having problems with the nuki.check_connection service. After a few uses the lock appears as unavailable and can't use it after a full HA restart; of course that the lock is available and I can access it from the internal REST API.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Aug 8, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.