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 java lib not decoding custom attribute correctly #30564

Closed
jonsmirl opened this issue Nov 19, 2023 · 8 comments · Fixed by #32285
Closed

[BUG] Android java lib not decoding custom attribute correctly #30564

jonsmirl opened this issue Nov 19, 2023 · 8 comments · Fixed by #32285
Assignees
Labels
android bug Something isn't working

Comments

@jonsmirl
Copy link
Contributor

Reproduction steps

Here it is in the report: @yunhanw-google
image
Here it is in the Kotlin report variable:
image

JSON and TLV both have the 2, but valueObject is null, it should be 2.

Example of correctly decoded standard attribute:
image

Bug prevalence

always

GitHub hash of the SDK that was being used

181b0cb

Platform

android

Platform Version(s)

No response

Anything else?

No response

@jonsmirl jonsmirl added bug Something isn't working needs triage labels Nov 19, 2023
@jonsmirl
Copy link
Contributor Author

Android lib built like this:

./scripts/build/build_examples.py \
  --target android-arm-chip-tool \
  --target android-arm64-chip-tool \
  --target android-x86-chip-tool \
  --target android-x64-chip-tool \
  build

@jonsmirl
Copy link
Contributor Author

I tried three attribute IDs (0xfff3003, 0x7fff0003, 0x333) and all fail. So it doesn't appear to be function of the attribute ID number, Note that I attached the custom attribute to the Bridged Device Basic Information Cluster (0x39). AFAIK that should legal to do. The attribute contains a small amount of data required by my app during setup.

As a work around I will try constructing a custom cluster. Doing that consumes a lot more RAM on the device.

@yunhanw-google yunhanw-google self-assigned this Nov 21, 2023
@yunhanw-google
Copy link
Contributor

yunhanw-google commented Nov 21, 2023

@jonsmirl custom attribute cannot be decoded via ValueObject, since knowledge for ValueObject is based upon known cluster/attribute...which are generated from zap xml...
https://github.com/project-chip/connectedhomeip/blob/master/src/controller/java/AndroidCallbacks.cpp#L309C13-L309C33

jobject DecodeAttributeValue(const app::ConcreteAttributePath & aPath, TLV::TLVReader & aReader, CHIP_ERROR * aError)

So you have to add this custom attribute in xml, and regenerate them and compile with java controller...

@jonsmirl
Copy link
Contributor Author

Does this work as a general solution? The TLV encodes the data type. The first '4' indicates byte, then the value is 2. So if the zap DecodeAttributeValue() fails, then you could fall back to tlv.get().

That has to happen in the library because I can't tell if the attribute is NULL because it is actually NULL, or if it is NULL due to DecodeAttributeValue() failing.

@yunhanw-google
Copy link
Contributor

yunhanw-google commented Nov 21, 2023

@jonsmirl Agree. I think we could do this fallback when custom attribute is not recognized, but if that is custom cluster, we cannot...

@jonsmirl
Copy link
Contributor Author

It should be possible to do the fall back on any primitive type even if it is on a custom cluster.

@joonhaengHeo
Copy link
Contributor

@jonsmirl
I think differently.
I think the correct flow for the DecodeAttributeValue function is not only to decode, but also to return an Error if the DataType is not the data type defined in the specification for each Attribute. In the case of Custom Cluster, I think you can parse the TLV Payload in AttributeState using the method in the kotlin tlv library.

@jonsmirl
Copy link
Contributor Author

In this case I added an extra, custom attribute to a standard cluster. I did not change the standard attributes in any way. So DecodeAttributeValue() is failing because the attribute is not part of the standard cluster definition. Adding custom attributes to standard clusters is allowed. I did it this way because I didn't want to build a full custom cluster just to pass one byte of setup info.

I can fix this my code. I have already done so by checking each report message to see if the ID is in the custom range. It would just be nice to have the library fallback to decoding the TLV for primitive types. Doing that would likely handle 90% of custom attributes without requiring the app writer to learn about TLV.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants