Use a single CHIPDeviceController for all the views in the iOS app. #1004
Conversation
bzbarsky-apple
requested review from
anush-apple,
BroderickCarlin,
chrisdecenzo,
gerickson,
hawk248,
jelderton,
robszewczyk,
saurabhst and
woody-apple
as code owners
| @property (readonly) ControllerOnMessageBlock onMessageHandler; | ||
| @property (readonly) ControllerOnErrorBlock onErrorHandler; | ||
| @property dispatch_queue_t appCallbackQueue; | ||
| @property ControllerOnMessageBlock onMessageHandler; |
There was a problem hiding this comment.
I would suggest being explicit about the property attributes.
Helps people who are new to objc from having to remember the default attributes.
| * | ||
| * @param[in] onError the block to call when there is a network error. | ||
| */ | ||
| - (void)registerCallbacks:(dispatch_queue_t)appCallbackQueue |
There was a problem hiding this comment.
Considering this is a sharedController the expectation is that there will be multiple clients for this class.
However, the registerCallbacks seems to allow only one client at a time as a new client registering with CHIPDeviceController will overwrite any existing callbacks.
I would change this API to take in an error block and a completion block as part of the sendMessage API.
Internally this class will maintain a map of message ID to these blocks. And then call the appropriate block when the message sends or errors out.
Essentially move this API to a model where we take send and error callbacks in the message send API.
There was a problem hiding this comment.
I think the update to address @sagar-apple's comments plus #1007 more or less addresses this.
We don't want to be reinitializing the CHIP stack as the user navigates through the app. This also fixes issues where we'd have multiple CHIPDeviceController instances live at the same time, which would bind to the same low-level socket and confuse the networking layer into delivering response notifications to the wrong CHIPDeviceController. The dealloc block for CHIPDeviceController was removed because now we always keep it alive in a static, so it would never run anyway.
|
LGTM |
We don't want to be reinitializing the CHIP stack as the user
navigates through the app.
This also fixes issues where we'd have multiple CHIPDeviceController
instances live at the same time, which would bind to the same
low-level socket and confuse the networking layer into delivering
response notifications to the wrong CHIPDeviceController.
The dealloc block for CHIPDeviceController was removed because now we
always keep it alive in a static, so it would never run anyway.
Problem
We create a new CHIPDeviceController every time we switch views in the iOS app.
Summary of Changes
Have a single, static, CHIPDeviceController that all the views share.
fixes #984