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

November fixes #59

Closed
wants to merge 2 commits into from
Closed

November fixes #59

wants to merge 2 commits into from

Conversation

gbrooker
Copy link
Contributor

@gbrooker gbrooker commented Nov 7, 2018

This pull request combines a number of fixes and minor enhancements.
Fixes:

  • Fix race condition when calling Evergreen logger from Server queue
  • Fix for HomePod (add boolean type to possible Characteristic values). HomePod sends true or false for the "on" characteristic, whereas iPhones send 1 or 0.
  • Fix for accessory configuration on restart. Prior to this fix, restarting a bridge incremented the configuration count each time, causing HomeKit to forget room settings.
  • Fix for a race condition on the continueRunning flag in Server, and improve support to stop and restart the Server.

Enhancements:

  • Add didChangeAccessoryList to DeviceDelegate. Allow a container app to observe changes to accessories (for dynamic accessory discovery)
  • Add Device.unpairFromAllControllers(). Removes all pairings

@gbrooker
Copy link
Contributor Author

gbrooker commented Nov 7, 2018

Note the build check fails because travis can't find your new sonar-scanner checking tool.

I have looked at running sonar-scanner myself, but it seems that for swift support, one needs a paid Developer version of the SonarQube product. Is this really necessary for the GitHub pull request ?

@Bouke
Copy link
Owner

Bouke commented Nov 10, 2018

So SonarCloud doesn't (yet) support external PRs, so I'll mark SonarCloud optional (PR #60).

I see you've included the changes from your previous PR (#55), which I'm not yet sold on. In order to get this PR ready for merge, exclude those changes.

The OpenWeather looks like a nice example of how to implement a software-only sensor. I'd rather not include it with the core HAP package. You could setup a separate add-on package, which we could reference from the README.

The changes regarding the race conditions / locking can be resolved by calling that code from the main thread. The main server isn't thread-safe and should always be controlled from the same thread. Making it fully thread-safe is a lot more work than just asserting the code is called from the "correct" thread. That'll resolve programming errors and will be a lot safer. What do you think?

@gbrooker
Copy link
Contributor Author

So SonarCloud doesn't (yet) support external PRs, so I'll mark SonarCloud optional (PR #60).

Great, thanks, I'll rebase the two PR's

I see you've included the changes from your previous PR (#55), which I'm not yet sold on. In order to get this PR ready for merge, exclude those changes.

Not sure how you got that impression. I already backed out everything from the other PR

The OpenWeather looks like a nice example of how to implement a software-only sensor. I'd rather not include it with the core HAP package. You could setup a separate add-on package, which we could reference from the README.

I'll respond to this in PR #55

The changes regarding the race conditions / locking can be resolved by calling that code from the main thread. The main server isn't thread-safe and should always be controlled from the same thread. Making it fully thread-safe is a lot more work than just asserting the code is called from the "correct" thread. That'll resolve programming errors and will be a lot safer. What do you think?

The changes regarding race conditions are to fix an issue in the current codebase, no matter if the code is called by the main thread or not. The dispatchPrecondition is not a 'solution' but an aid to a developer to ensure there is not a threading error while testing their application.

I'll do some exercises in git to split out each of the changes here into separate PR's, so we can discuss each one individually.

…when an accessory changes it's value. Fix race condition when calling Evergreen logger from Server queue. Fix for HomePod (add boolean value type). Fix for accessory configuration on restart. Add Device.unpairFromAllControllers(). Fix for race condition on continueRunning flag in Server, and improve support to stop and restart.
@gbrooker
Copy link
Contributor Author

This PR has been split into PR #61 (Fix for HomePod) , PR #62 (Fix initial accessory configuration), PR #63 (Fix unpairing) and PR #64 (Fix Server race conditions), and so will now be closed.

@gbrooker gbrooker closed this Nov 11, 2018
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.

2 participants