Fix CHIPDeviceController when storage delegate is nullptr #4089
Conversation
pullapprove
bot
requested review from
andy31415,
BroderickCarlin,
bzbarsky-apple,
chrisdecenzo,
hawk248,
jelderton,
mspang,
rwalker-apple and
saurabhst
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.
@pan-apple Could you double-check this, please.
There was a problem hiding this comment.
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.
@pan-apple Please take a look here too?
There was a problem hiding this comment.
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.
Sure, I'll add TODO
|
|
||
| return false; | ||
| if (mDevice == nullptr) |
There was a problem hiding this comment.
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.
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.
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.
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.
Damian-Nordic
force-pushed
the
feature/controller-without-storage
branch
2 times, most recently
from
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.
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.
Damian-Nordic
force-pushed
the
feature/controller-without-storage
branch
from
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
GetIpAddresswork 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.