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

BLE fixes #436

Merged
merged 10 commits into from
Aug 1, 2024
Merged

BLE fixes #436

merged 10 commits into from
Aug 1, 2024

Conversation

iranl
Copy link
Collaborator

@iranl iranl commented Jul 24, 2024

Description:

Fix BLE issues where Nuki Hub or Nuki device can get stuck.
Seems related to BLE connections between Nuki Hub and Nuki device not being closed correctly.

This PR aims to fix these issues.
Connecting also seems quicker in initial testing and Nuki Hub is more stable on the ESP32-C6 and now supports pairing

Possibly fixes #432
Fixes #438
Fixes #326

Needs I-Connect/NukiBleEsp32#71 to be merged first
Needs I-Connect/NukiBleEsp32#72 to be merged first
Needs I-Connect/NukiBleEsp32#73 to be merged first
Needs I-Connect/NukiBleEsp32#74 to be merged first

Checklist:

  • The pull request is done against the latest master branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works
  • I accept the CLA.

@iranl iranl added the enhancement New feature or request label Jul 24, 2024
@iranl iranl added this to the 9.00 milestone Jul 24, 2024
@iranl iranl marked this pull request as ready for review July 24, 2024 20:57
@iranl iranl marked this pull request as draft July 24, 2024 20:58
@iranl iranl marked this pull request as ready for review July 29, 2024 14:55
@iranl iranl requested a review from technyon July 29, 2024 15:08
@iranl iranl marked this pull request as draft July 29, 2024 15:31
@iranl
Copy link
Collaborator Author

iranl commented Jul 29, 2024

Might still have an issue with updating Nuki device config.

Edit: Fixed with I-Connect/NukiBleEsp32#74

@iranl iranl marked this pull request as ready for review July 31, 2024 11:21
@technyon
Copy link
Owner

technyon commented Aug 1, 2024

Hi,

Why are we shifting the nuki task to core 0? This could lead to race conditions between the nuki and network task.

@iranl
Copy link
Collaborator Author

iranl commented Aug 1, 2024

It is advised to run the BLE controller and tasks using BLE (esp-nimble-cpp/nuki_ble) on the same core.

Can't think of what (new) race conditions would/could unsue or in what situations this would cause an issue.
Any call to delay() will yield to other tasks, so that already allows both tasks to run pretty much concurrently in the current situation if I am not mistaken.

Have been running this build on multiple ESP devices without issue for a couple of days.

@technyon
Copy link
Owner

technyon commented Aug 1, 2024

I think there are some variables that could be accessed simultaneously from both tasks if running on different cores, this would require some kind of locking. But I'd need to go through the code in detail. Well, let's test it for now it's easy to revert.

@technyon technyon merged commit 9d09c43 into technyon:master Aug 1, 2024
10 checks passed
@iranl iranl deleted the ble-fixes branch August 4, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants