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

Try pairing when descriptor write fails #190

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dakhnod
Copy link
Contributor

@dakhnod dakhnod commented Apr 4, 2023

The code tries pairing with a BLE device when an initial characteristic read operation fails, and then tries to read the value again after pairing.

Yet, there might be a failing descriptor write operation first, or no characteristic read at all.
Hence, this code adds handling of such a case by buffering every write, issuing pairing if needed and re writing the descriptor.

This code is just a starting point. @shmuelzon How would you buffer the writes?

@dakhnod
Copy link
Contributor Author

dakhnod commented Sep 17, 2023

@shmuelzon any comment on this?

Copy link
Owner

@shmuelzon shmuelzon left a comment

Choose a reason for hiding this comment

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

Hey,

There was a reason why writing to a characteristic's descriptor has different logic than the rest.
I think the responses for it weren't consistent and the application would sometimes wait for a response that never came (causing us to hang and not dequeue the next operation).

See 156ce8b. It may have been a bug in ESP-IDF that was since fixed but I'm not even sure which peripherals had this issue so I can't test with them

Comment on lines +977 to +978
if (param->read.status == ESP_GATT_INSUF_AUTHENTICATION ||
param->read.status == ESP_GATT_INSUF_ENCRYPTION)
Copy link
Owner

Choose a reason for hiding this comment

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

These should probably check param->write.status.

Comment on lines +185 to +186
memcpy(write_buf, operation->value, operation->len);
write_buf_len = operation->len;
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need these static variables? They should be part of the operation and remain there until we decide to dequeue it (after it was successful).

@dakhnod
Copy link
Contributor Author

dakhnod commented Sep 17, 2023

@shmuelzon how about this:
Instead of focusing on single operations, we run the regular initialization routine as it used to be.
When we hit an insufficient-encryption or encryption we try bonding, and then re-run the regular routine from start.
That way, all possible types of read/write operations should be covered.

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

Successfully merging this pull request may close these issues.

None yet

2 participants