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

Android add discover #10682

Merged
merged 21 commits into from
Nov 2, 2021
Merged

Conversation

joonhaengHeo
Copy link
Contributor

Problem

What is being fixed?

  • Android Test Application is not supported dns discover. (Python chip-ctrl-tool command is "discover -all")

Change overview

  • Add NsdManagerDiscoverService class.
  • Support discover function in AddressCommissioning activiry.

Testing

How was this tested? (at least one bullet point required)

  • Check to be able to discover in Android App.

@CLAassistant
Copy link

CLAassistant commented Oct 20, 2021

CLA assistant check
All committers have signed the CLA.

@Damian-Nordic
Copy link
Contributor

Would it make sense to implement the discovery as part of the existing NsdManagerServiceResolver class and ServiceResolver interface? That would be more aligned with other platforms which always perform the DNS-SD using the common native code.

Copy link
Contributor

@austinh0 austinh0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to what Damian said - if you are going to add NsdManagerServiceDiscover in the platform code, then you should also implement the platform DnssdImpl::ChipDnssdBrowse.

I can accept doing mDNS discovery purely in the app code (i.e. use NsdManager directly in the app), at least as a temporary solution, but we should not mix these two approaches.

Will review the PR in more detail after this is resolved.

@joonhaengHeo
Copy link
Contributor Author

@Damian-Nordic In the case of Android, the only way was to call ServiceResolver to know detailed information such as IP Address and discriminator. (I couldn't find another way.)
Also, it was confirmed that if it is called again before the callback of "NsdManager.resolveService", it will surely fail. Therefore, I thought that reusing the existing code would be helpful in solving complex cases in the future.

@joonhaengHeo
Copy link
Contributor Author

@austinh0 Does it mean that it should be implemented in a jni format similar to the implementation of Service Resolver?

Copy link
Contributor

@austinh0 austinh0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

Is there a reason we call discoverCommissionableNodes, wait, and the check getDiscoveredDevice instead of having some sort of callback method that is invoked when a device is discovered? I haven't looked at the native implementation in a while.

@todo
Copy link

todo bot commented Nov 1, 2021

(#7985): Annotate csrNonce as Nullable.

* TODO(#7985): Annotate csrNonce as Nullable.
*
* @param bleServer the BluetoothGatt representing the BLE connection to the
* device
* @param connId the BluetoothGatt Id representing the BLE connection to
* the device
* @param deviceId the node ID to assign to the device
* @param setupPincode the pincode for the device
* @param csrNonce the 32-byte CSR nonce to use, or null if we want to use
* an internally randomly generated CSR nonce.
*/


This comment was generated by todo based on a TODO comment in 4dad4bb in #10682. cc @joonhaengHeo.

@todo
Copy link

todo bot commented Nov 1, 2021

(#8443): This method could benefit from a ChipDevice abstraction to hide

* TODO(#8443): This method could benefit from a ChipDevice abstraction to hide
* the pointer passing.
*/
public void getConnectedDevicePointer(long nodeId, GetConnectedDeviceCallback callback) {
GetConnectedDeviceCallbackJni jniCallback = new GetConnectedDeviceCallbackJni(callback);


This comment was generated by todo based on a TODO comment in 4dad4bb in #10682. cc @joonhaengHeo.

@todo
Copy link

todo bot commented Nov 1, 2021

Find out if DNS-SD results for Android should contain interface ID

// TODO: Find out if DNS-SD results for Android should contain interface ID
chipMdnsCallback.handleServiceResolve(instanceName, serviceType, serviceInfo.getHost().getHostName(),
serviceInfo.getHost().getHostAddress(), serviceInfo.getPort(), serviceInfo.getAttributes(), callbackHandle,
contextHandle);
if (multicastLock.isHeld()) {
multicastLock.release();
}
mainThreadHandler.removeCallbacks(timeoutRunnable);
}
});


This comment was generated by todo based on a TODO comment in 4dad4bb in #10682. cc @joonhaengHeo.

@andy31415
Copy link
Contributor

/rebase

@andy31415 andy31415 merged commit 8ae5b65 into project-chip:master Nov 2, 2021
woody-apple added a commit that referenced this pull request Nov 2, 2021
andy31415 pushed a commit that referenced this pull request Nov 2, 2021
PSONALl pushed a commit to PSONALl/connectedhomeip that referenced this pull request Dec 3, 2021
* Add Android Discover

* Fix Build error

* Modify coding rule, string xml

* Modify coding style

* Restyled by google-java-format

* Restyled by gn

* Change browse using android platform layer

* Restyled by whitespace

* Restyled by google-java-format

* Restyled by clang-format

* Restyled by clang-format

* Modify after review

* Remove unused code

* Restyled by google-java-format

* Restyled by gn

* Modify some issue

* Fix some issue after reviewing

* restore file

* Restyled by clang-format

Co-authored-by: Restyled.io <commits@restyled.io>
PSONALl pushed a commit to PSONALl/connectedhomeip that referenced this pull request Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants