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

[BUG] Android SDK parse manual code fail #25477

Open
luobosu opened this issue Mar 6, 2023 · 20 comments
Open

[BUG] Android SDK parse manual code fail #25477

luobosu opened this issue Mar 6, 2023 · 20 comments
Labels

Comments

@luobosu
Copy link

luobosu commented Mar 6, 2023

Reproduction steps

1.scan a bar code and get the raw text :
MT:YZ7A0U.R02T.163EI00

2.get a setup payload by sdk method :
val payloadParser = SetupPayloadParser()
val payload = payloadParser.parseQrCode(“MT:YZ7A0U.R02T.163EI00”)

and I can see the attibutes on the payload:
version=0, vendorId=4488, productId=257, discriminator=1018, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[BLE], ipAddress=null

3.get a manul code of this payload:
String entryCode = payloadParser.getManualEntryCodeFromPayload(setupPayload)

4.get a setup payload by sdk method :
setupCodeParser.parseManualEntryCode(entryCode)
and I can see the attibutes on the payload:
version=0, vendorId=0, productId=0, discriminator=768, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[], ipAddress=null

this discriminator is not match to the discriminator on the step2 !!

Bug prevalence

always

GitHub hash of the SDK that was being used

TAG 1.0.0.2

Platform

android

Platform Version(s)

No response

Anything else?

No response

@bzbarsky-apple
Copy link
Contributor

1018 = 0x3fa and 768 = 0x300. So as far as that goes this seems correct: a manual code only contains the high 4 bits of the 12-bit discriminator. Now if the payload is not tracking whether it's got a short or long discriminator that's a problem, of course. But there is no way you can get 1018 as a discriminator out of a manual code; you can only get 3 (the high 4 bits).

@luobosu
Copy link
Author

luobosu commented Mar 7, 2023

1018 = 0x3fa and 768 = 0x300. So as far as that goes this seems correct: a manual code only contains the high 4 bits of the 12-bit discriminator. Now if the payload is not tracking whether it's got a short or long discriminator that's a problem, of course. But there is no way you can get 1018 as a discriminator out of a manual code; you can only get 3 (the high 4 bits).

we want to provide two ways to users to connect the the matter device : scan a barcode or input the 11/21 digits manual code . As you say , we can not provide the input-method to users , right ? (Maybe the 21 digits code can parse 1018? but I don't know how to get it)

@bzbarsky-apple
Copy link
Contributor

You cannot get "1018" out of any manual code. The 21-digit version includes a VID+PID, but still has a short discriminator.

Whatever code is doing discriminator matching needs to track whether it has a long or short discriminator. If it has a short one, it needs to only compare it to the top 4 bits of the device-advertised discriminator.

@luobosu
Copy link
Author

luobosu commented Mar 7, 2023

You cannot get "1018" out of any manual code. The 21-digit version includes a VID+PID, but still has a short discriminator.

Whatever code is doing discriminator matching needs to track whether it has a long or short discriminator. If it has a short one, it needs to only compare it to the top 4 bits of the device-advertised discriminator.

We need discriminator 1018 to pair the device with bluetooth . If we use discriminator 768 , it will connect fail .
It seems that 1018 is a long discriminator , 768 is a short discriminator.

So we can not parse a long discriminator which can be used on pairing with bluetooth from 11digits , right ?

@bzbarsky-apple
Copy link
Contributor

We need discriminator 1018 to pair the device with bluetooth .

The core code certainly does not need that; both chip-tool on Linux and Mac and the Darwin framework code can pair over bluetooth using short discriminators.

It sounds like the real problem here is that BLEManagerImpl::NewConnection on Android has this comment:

    // TODO: The API we have here does not handle short discriminators in any
    // sane way.

and indeed the code there makes not sense for short discriminators.

But that's a not a problem with parsing of the manual code. It's a bug in the BLE bits on Android.

So we can not parse a long discriminator which can be used on pairing with bluetooth from 11digits , right ?

You cannot parse a long discriminator from an 11-digit from numeric code.

You can pair with a short discriminator on non-buggy platforms, but it's broken on Android, apparently.

@joonhaengHeo
Copy link
Contributor

@luobosu
For Android, Bluetooth-related code exists in the Example Android Chip Tool. (SetupPayload Parsing is doing at Application Layer.)

private fun getServiceDataMask(isShortDiscriminator: Boolean): ByteArray {

@luobosu
Copy link
Author

luobosu commented Mar 7, 2023

How about iOS platform ? My iOS colleague say that he

We need discriminator 1018 to pair the device with bluetooth .

The core code certainly does not need that; both chip-tool on Linux and Mac and the Darwin framework code can pair over bluetooth using short discriminators.

It sounds like the real problem here is that BLEManagerImpl::NewConnection on Android has this comment:

    // TODO: The API we have here does not handle short discriminators in any
    // sane way.

and indeed the code there makes not sense for short discriminators.

But that's a not a problem with parsing of the manual code. It's a bug in the BLE bits on Android.

So we can not parse a long discriminator which can be used on pairing with bluetooth from 11digits , right ?

You cannot parse a long discriminator from an 11-digit from numeric code.

You can pair with a short discriminator on non-buggy platforms, but it's broken on Android, apparently.

revert_commit

How about iOS platform ? My iOS colleague say that he meet the same issue .
Will you try adapt it for android platform in the future ?

By the way , I want to try revert the commit on "Fix handling of short discriminator in Linux BLE scan. (#21187)"

@luobosu
Copy link
Author

luobosu commented Mar 7, 2023

@luobosu For Android, Bluetooth-related code exists in the Example Android Chip Tool. (SetupPayload Parsing is doing at Application Layer.)

private fun getServiceDataMask(isShortDiscriminator: Boolean): ByteArray {

I will try it for the short discriminator.
But how about setupPinCode?

version=0, vendorId=0, productId=0, discriminator=768, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[], ipAddress=null

the setupPinCode is wrong ,too

@joonhaengHeo
Copy link
Contributor

@luobosu
What do you mean differently mean? I think it's the same pinCode.

version=0, vendorId=4488, productId=257, discriminator=1018, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[BLE], ipAddress=null

version=0, vendorId=0, productId=0, discriminator=768, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[], ipAddress=null

@luobosu
Copy link
Author

luobosu commented Mar 7, 2023

@luobosu What do you mean differently mean? I think it's the same pinCode.

version=0, vendorId=4488, productId=257, discriminator=1018, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[BLE], ipAddress=null

version=0, vendorId=0, productId=0, discriminator=768, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[], ipAddress=null

oh ! Sorry , I was wrong , it is the same .
Let me try the getServiceDataMask method firtly , thanks!

@luobosu
Copy link
Author

luobosu commented Mar 7, 2023

It is OK now , thanks a lot ! @joonhaengHeo
By the way , from these information , I can not get a available vendor id now,:
version=0, vendorId=0, productId=0, discriminator=768, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[], ipAddress=null

when I init the chipdevicecontroller , it will crash . Should I replace the vendor id from 0 to VENDOR_ID = 0xFFF4 (default value on sample) ?

this is the crash information:

03-07 14:36:07.806  7835  7835 E AndroidRuntime: chip.devicecontroller.ChipDeviceControllerException: ../../src/controller/CHIPDeviceControllerFactory.cpp:310: CHIP Error 0x0000002F: Invalid argument
03-07 14:36:07.806  7835  7835 E AndroidRuntime: 	at chip.devicecontroller.ChipDeviceController.newDeviceController(Native Method)
03-07 14:36:07.806  7835  7835 E AndroidRuntime: 	at chip.devicecontroller.ChipDeviceController.<init>(ChipDeviceController.java:57)

@bzbarsky-apple
Copy link
Contributor

How about iOS platform ? My iOS colleague say that he meet the same issue.

This all works fine on iOS if you use the APIs correctly. MTRSetupPayload on iOS has a hasShortDiscriminator property, and the commissioner code knows how to use it correctly. If you're running into iOS problems, please file an issue with the details, including what exact APIs you are calling and how.

@joonhaengHeo
Copy link
Contributor

It is OK now , thanks a lot ! @joonhaengHeo By the way , from these information , I can not get a available vendor id now,: version=0, vendorId=0, productId=0, discriminator=768, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[], ipAddress=null

when I init the chipdevicecontroller , it will crash . Should I replace the vendor id from 0 to VENDOR_ID = 0xFFF4 (default value on sample) ?

this is the crash information:

03-07 14:36:07.806  7835  7835 E AndroidRuntime: chip.devicecontroller.ChipDeviceControllerException: ../../src/controller/CHIPDeviceControllerFactory.cpp:310: CHIP Error 0x0000002F: Invalid argument
03-07 14:36:07.806  7835  7835 E AndroidRuntime: 	at chip.devicecontroller.ChipDeviceController.newDeviceController(Native Method)
03-07 14:36:07.806  7835  7835 E AndroidRuntime: 	at chip.devicecontroller.ChipDeviceController.<init>(ChipDeviceController.java:57)

@luobosu
I think this issue is not related to Manual Pairing Code.
I don't know which Source Code you are using, but is the code below added to your code?

chipDeviceController = ChipDeviceController(ControllerParams.newBuilder().setControllerVendorId(VENDOR_ID).build())

This issue occurs because the Controller Vendor ID is not set.

@luobosu
Copy link
Author

luobosu commented Mar 8, 2023

How about iOS platform ? My iOS colleague say that he meet the same issue.

This all works fine on iOS if you use the APIs correctly. MTRSetupPayload on iOS has a hasShortDiscriminator property, and the commissioner code knows how to use it correctly. If you're running into iOS problems, please file an issue with the details, including what exact APIs you are calling and how.

ok , my colleague will follow it , thanks ~

@luobosu
Copy link
Author

luobosu commented Mar 8, 2023

@joonhaengHeo

yes , it is this code . But I can only get the verdorId=0 from the 11 digists manual pairing code . As you see :
version=0, vendorId=0, productId=0, discriminator=768, setupPinCode=22884930, commissioningFlow=0, optionalQrCodeInfoMap={}, discoveryCapabilities=[], ipAddress=null
When I call the method with vendorId = 0 , it will crash :
chipDeviceController = ChipDeviceController(ControllerParams.newBuilder().setControllerVendorId(0).build())

When I use the default vendorId = 0xFFF4 , it will work ok:
chipDeviceController = ChipDeviceController(ControllerParams.newBuilder().setControllerVendorId(0xFFF4).build())

My problem is that should I use 0xFFF4 to init the ChipDeviceController object when I can only get vendorId = 0 from the 11 digists?

@edWin-m
Copy link

edWin-m commented Mar 30, 2023

@luobosu I am facing the same Issue. Have you managed to resolve this for manual codes?

The manual code for an Esp32 device built off the esp-matter works with the android demo. However, another device supplied by another manufacturer doesn't. I have noticed that the discriminator obtained from the QR code (3414) is also different from that obtained from the manual pairing code(3328).
For the one used in the Esp32 demo, both payloads have the same discriminator.

@joonhaengHeo for the device with different discriminators, the device name for the device returned by

val device = bluetoothManager.getBluetoothDevice(requireContext(), deviceInfo.discriminator, deviceInfo.isShortDiscriminator)

has the name MATTER-3414 (even when using the discriminator 3328 ). Is it supposed to change in order to mirror the discriminator used?

@luobosu
Copy link
Author

luobosu commented Mar 30, 2023

@luobosu For Android, Bluetooth-related code exists in the Example Android Chip Tool. (SetupPayload Parsing is doing at Application Layer.)

private fun getServiceDataMask(isShortDiscriminator: Boolean): ByteArray {

@edWin-m I have resolved it using this code . But I do not know about Esp32 demo , sorry about that.

@edWin-m
Copy link

edWin-m commented Mar 30, 2023

@luobosu I am sure I have the same code, but somehow this issue still persists. Besides passing the generated service mask into the scan filter as shown below is there another way to use this mask?

  val serviceData = getServiceData(discriminator)
  val serviceDataMask = getServiceDataMask(isShortDiscriminator)

  val scanFilter =
      ScanFilter.Builder()
          .setServiceData(ParcelUuid(UUID.fromString(CHIP_UUID)), serviceData, serviceDataMask)
          .build()

@joonhaengHeo
Copy link
Contributor

@edWin-m
3414 of QRCode and 3328 of Manual Pairing Code are judged as the same discriminator.

3414 -> 0xD56
3328 -> 0xD00

In the case of Manual Pairing Code, since Short Discriminator is used, only 4 bits are valid.

@edWin-m
Copy link

edWin-m commented Apr 7, 2023

@joonhaengHeo thanks for the reply. However, this leads me right back to the beginning. If the issue with the short discriminator was fixed as you hinted below, why does the device still fail to commission when using the manual code? Is it something that I need to discuss with the manufacturer?

@luobosu For Android, Bluetooth-related code exists in the Example Android Chip Tool. (SetupPayload Parsing is doing at Application Layer.)

private fun getServiceDataMask(isShortDiscriminator: Boolean): ByteArray {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

No branches or pull requests

5 participants