Fix unpair, to only change state when no pairings remain#63
Fix unpair, to only change state when no pairings remain#63Bouke merged 3 commits intoBouke:masterfrom
Conversation
…romAllControllers() function
|
|
||
| // Remove all the pairings made with this Device | ||
| // Can be used in the event of a stale configuration file | ||
| public func unpairFromAllControllers() { |
There was a problem hiding this comment.
We don't notify the other party of the removal, so is it really unpairing or removing the pairing?
| public func unpairFromAllControllers() { | |
| public func removeAllPairings() { |
There was a problem hiding this comment.
Yes, the HAP protocol doesn't seem to allow for an accessory to request to unpair. The only way to do it is equivalent to a hard reset on a hardware device, i.e to simply close all open connections and forget the pairings. I'll change the function name to your suggestion.
| logger.debug("Before unpair") | ||
| logger.debug(self.configuration) | ||
| logger.debug(self.config) | ||
| server?.stop() |
There was a problem hiding this comment.
Why do we stop the server? It's also not clear from the method name that this happens.
There was a problem hiding this comment.
Because we need to close all open connections, to force the unpairing.
There was a problem hiding this comment.
So what's the exact use-case of this method? I'm guessing it would be some software equivalent of a hardware device's "hard reset". And so, I would expect it to also call server?.start(). The documentation would have to be more explicit about what this method does. E.g. should it start() when it wasn't already started on entry, or would that be an invalid entry state?
There was a problem hiding this comment.
Yes, the use case is part of a local reset. I have removed the server.stop(), as you are right that it is misplaced here. The management of the server state should be performed by the caller.
| // Remove all the pairings made with this Device | ||
| // Can be used in the event of a stale configuration file | ||
| public func unpairFromAllControllers() { | ||
| logger.debug("Before unpair") |
There was a problem hiding this comment.
Do we need all this logging including the before and after state? And if so: can we condense these six lines into a single line?
There was a problem hiding this comment.
The logging is not needed at all. I can will remove them.
| logger.debug("Before unpair") | ||
| logger.debug(self.configuration) | ||
| logger.debug(self.config) | ||
| server?.stop() |
There was a problem hiding this comment.
So what's the exact use-case of this method? I'm guessing it would be some software equivalent of a hardware device's "hard reset". And so, I would expect it to also call server?.start(). The documentation would have to be more explicit about what this method does. E.g. should it start() when it wasn't already started on entry, or would that be an invalid entry state?
2 participants
Split out from PR #59
When removing pairing, only change sate when no more pairings remain.
Add unpairFromAllControllers() function to remove all pairings.