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

Ensure access to characteristics happen on the main queue. #79

Merged
merged 5 commits into from
Feb 27, 2019

Conversation

gbrooker
Copy link
Contributor

Two small changes.

First change corrects the precondition on set characteristic value.

Secondly, the characteristic endpoint is currently running on a NIO queue, causing thread safety issues.

This PR performs the endpoint on the main queue, to ensure a single queue access the HAP characteristics. A future improvement could be to restructure the entire endpoint architecture to use promises, which would avoid a synchronous call across queues.

…least 1155 bytes. Add back the debug logging to root() to help fix configuration issues.
@gbrooker
Copy link
Contributor Author

Added workaround mentioned in #77 to ensure a minimum buffer size larger than 1155 for messages from HK.

Also added the debug message in root() to log the message sent to HomeKit to help fix issues in communications or configuration. The original version has a spelling mistake ("messagea") which was useful for locating it in the logs, so I preserved the spelling mistake !

@gbrooker
Copy link
Contributor Author

gbrooker commented Feb 26, 2019

The Tests were running on the main queue, which doesn't happen under normal operations. I've modified the characteristic tests to run from a separate queue, simulating a NIO thread.

Tests/HAPTests/EndpointTests.swift Show resolved Hide resolved
Sources/HAP/Endpoints/root().swift Outdated Show resolved Hide resolved
Sources/HAP/Base/Characteristic.swift Show resolved Hide resolved
@Bouke Bouke merged commit a86006f into Bouke:master Feb 27, 2019
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