-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix CHIPDeviceController when storage delegate is nullptr #4089
Fix CHIPDeviceController when storage delegate is nullptr #4089
Conversation
src/controller/CHIPDevice.cpp
Outdated
@@ -244,9 +244,9 @@ CHIP_ERROR Device::LoadSecureSessionParameters(ResetTransport resetNeeded) | |||
|
|||
bool Device::GetIpAddress(Inet::IPAddress & addr) const | |||
{ | |||
if (mState == ConnectionState::SecureConnected) | |||
if (mState != ConnectionState::NotConnected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pan-apple Could you double-check this, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would peronsally find this easier to read:
if (mState == ConnectionState::NotConnected) {
return false;
}
addr = mDeviceAddr;
return true;
this shows that 'return true' and 'set device addr' are really closely tied together.
@@ -608,7 +608,7 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status) | |||
mPairingDelegate->OnPairingComplete(status); | |||
} | |||
|
|||
if (mDeviceBeingPaired != kNumMaxActiveDevices) | |||
if (mDeviceBeingPaired != kNumMaxActiveDevices && mStorageDelegate != nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pan-apple Please take a look here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO here to remove this condition when all the controller apps have implemented the storage delegate? To support multiple device pairings, storage delegate cannot be optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add TODO
|
||
return false; | ||
if (mDevice == nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like an unusual sideffect - get ip addres may do a non-const init. Do we really need it?
or should we ignore since _deprecated should go away anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit it's not the most elegant solution, but I thought it didn't have to be since _deprecated will go away anyway ;). However, until it happens we should have code that works at least. Currently, it's impossible to test that Rendezvous completed successfully. Should I rename getIpAddress
to something not suggesting that it's a simple getter? retrieveIpAddress
mabe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just wondering about losing const.
Would if work if we return false if mDevice is nullptr?
That way we can stay const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, mDevice is only initialized when sending a message/command and getIpAddress
was introduced to automatically obtain IP address of a connected device before sending it a message to avoid typing long IPv6 addresses manually. The feature will probably be replaced with service discovery, but for now it's quite helpful I think.
d27582f
to
b0eec82
Compare
@@ -608,7 +608,7 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status) | |||
mPairingDelegate->OnPairingComplete(status); | |||
} | |||
|
|||
if (mDeviceBeingPaired != kNumMaxActiveDevices) | |||
if (mDeviceBeingPaired != kNumMaxActiveDevices && mStorageDelegate != nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO here to remove this condition when all the controller apps have implemented the storage delegate? To support multiple device pairings, storage delegate cannot be optional.
Another potential solution is
This will be more future proof, and will allow the current apps (that are using the deprecated controller) to continue to function. |
CHIPDeviceController releases results of the Rendezvous session assuming that they have been persisted. However some platforms, like Android, don't provide implementation of PersistentStorageDelegate yet.
1c575e6
to
041100c
Compare
Problem
CHIPDeviceController releases results of the Rendezvous session assuming that they have been persisted. However some platforms, like Android, don't provide implementation of PersistentStorageDelegate yet.
Summary of Changes
GetIpAddress
work again after recent refactoring.If it's desired that CHIPDeviceController rely on the storage delegate (or actually require it), all platforms should implement it first. I filed #4088 to fill the gap on Android side.