search

uriyacovy / ESPHome_nuki_lock

ESPHome lock platform for Nuki Smartlock
MIT License
72 stars 21 forks source link

Feature: Several Basic and Advanced Settings, Event logs, Last Unlock User #59

Closed AzonInc closed 1 month ago

AzonInc commented 1 month ago

This PR adds several basic and advanced settings entites as well as event support.

Switch:

Text Sensor:

Select:

Number:

Events: Lock events are sent to Home Assistant with the esphome.nuki identifier by default.

event_type: esphome.nuki
data:
  device_id: 373c62d6788cf81d322763235513310e
  action: Unlatch
  authorizationId: "3902896895"
  authorizationName: Nuki ESPHome
  completionStatus: success
  index: "92"
  timeDay: "25"
  timeHour: "0"
  timeMinute: "46"
  timeMonth: "10"
  timeSecond: "11"
  timeYear: "2024"
  trigger: system
  type: LockAction
origin: LOCAL
time_fired: "2024-10-25T00:46:33.398284+00:00"
context:
  id: 01JB0J7AXPMS5DWHG188Y6XFCP
  parent_id: null
  user_id: null
AzonInc commented 1 month ago

I synced the base library with the main. I would have linked it directly to a specific release version but it does not seem like releases are following the code updates.

Using commit hashes works just fine. For example: https://github.com/I-Connect/NukiBleEsp32#93e7da927171c8973b7ef857c7fa644c174ed47d

Library Manager: Installing git+https://github.com/I-Connect/NukiBleEsp32#93e7da927171c8973b7ef857c7fa644c174ed47d
INFO Installing git+https://github.com/I-Connect/NukiBleEsp32#93e7da927171c8973b7ef857c7fa644c174ed47d
git version 2.39.5
Cloning into '/home/runner/.platformio/.cache/tmp/pkg-installing-2xroo738'...
HEAD is now at 93e7da9 Added MIT license
INFO NukiBleEsp@2.0.0+sha.93e7da9 has been installed!
Library Manager: NukiBleEsp@2.0.0+sha.93e7da9 has been installed!

And the dev builds are failing because the dev branch is still behind.

I changed the lib reference to the base repo and added the last commit hash as branch for the main build and in readme. The dev build uses the latest version of the base branch from I-Connect.

With this change you don't have to maintain two repos anymore and still have reference to a specific version of the code.

uriyacovy commented 1 month ago
  1. No problem using the hash in github actions, but it's less clear when normal users need to define it. That's why I forked the repo.
  2. After using the previous PR, I got some instability (disconnections) for long periods (an hour+). I'll retry now with this new version and update.
  3. I saw that you added delays of 5 sec. Why are they needed? Also, from my experience long delays cause issues with ESPHome and HA.
  4. I think it also worth updating the README with the latest features you've added.
AzonInc commented 1 month ago
  1. No problem using the hash in github actions, but it's less clear when normal users need to define it. That's why I forked the repo.

Ah okay, well... so lets do this for actions and still use yours for the Readme part.

  1. After using the previous PR, I got some instability (disconnections) for long periods (an hour+). I'll retry now with this new version and update.

Maybe thats caused by the more frequent request of the config after each status update. Do you know if it's only blocking the cycle or rebooting due to memory issues?

  1. I saw that you added delays of 5 sec. Why are they needed? Also, from my experience long delays cause issues with ESPHome and HA.

There is probably a better way to achieve this by waiting for a period of time and then check again. The results are not immediately available.

  1. I think it also worth updating the README with the latest features you've added.

I added all the nee Entites there and also mentioned the Events part. Or did u mean smth else?

uriyacovy commented 1 month ago
  1. After using the previous PR, I got some instability (disconnections) for long periods (an hour+). I'll retry now with this new version and update.

Maybe thats caused by the more frequent request of the config after each status update. Do you know if it's only blocking the cycle or rebooting due to memory issues?

I didn't save the logs so I can't tell for now. I follow and update.

  1. I saw that you added delays of 5 sec. Why are they needed? Also, from my experience long delays cause issues with ESPHome and HA.

There is probably a better way to achieve this by waiting for a period of time and then check again. The results are not immediately available.

AFAIK ESPHome is asynchronous, and to achieve this behavior you have to use set_timeout

  1. I think it also worth updating the README with the latest features you've added.

I added all the nee Entites there and also mentioned the Events part. Or did u mean smth else?

I think just how to see the logs is missing

AzonInc commented 1 month ago

I didn't save the logs so I can't tell for now. I follow and update.

Okay lets see what happens.

I think just how to see the logs is missing

Ahh okay uhm the logs are just sent to homeassistant as events. I thought thats covering the logs already. I'm not sure what more to say about it. Maybe an explanation how to go to the developer tools page.

AFAIK ESPHome is asynchronous, and to achieve this behavior you have to use set_timeout

I will check a few options to see if they solve the delay issue.

AzonInc commented 1 month ago

After using the previous PR, I got some instability (disconnections) for long periods (an hour+). I'll retry now with this new version and update.

Do you mean it disconnected for an hour or after a long period?

uriyacovy commented 1 month ago

After using the previous PR, I got some instability (disconnections) for long periods (an hour+). I'll retry now with this new version and update.

Do you mean it disconnected for an hour or after a long period?

Yes. It didn't discounted since, but anyway I think we should not use long delays.

AzonInc commented 1 month ago

I thought about setting a timeout period in the loop and executing it when the time is up. For auth data and event logs.

Also maybe change the way data is requested and not requesting on every status change but independently.

uriyacovy commented 1 month ago

I thought about setting a timeout period in the loop and executing it when the time is up. For auth data and event logs.

Why not to use set_timeout instead?

Also maybe change the way data is requested and not requesting on every status change but independently.

You can do that, but just make sure that the status is updated whenever it is changed from the app/manually.

AzonInc commented 1 month ago

Yea I just mean the config, advanced config and auth data + event logs because that's time consuming to request all of that after each other.

I never used set_timeout, how does it work?

uriyacovy commented 1 month ago

I don't have much knowledge about set_timeout, but in the discord there's some explanation: https://discord.com/channels/429907082951524364/1234784073071722567/1234784075823190027 https://discord.com/channels/429907082951524364/1204163134064168970/1204163136144408627

AzonInc commented 1 month ago

Do you think it's really necessary to request the auth data everytime or is every second time fine as well? Because now I'm requesting auth data and then logs wehenever there is a change. However auth data doesn't change as frequently as logs. So there could be smth like a bool (initially true) which is toggled on every update and whenever it is true auth data will be requested.