fix: Duplicate Characteristic UUID overwrite each other #365
Conversation
src/droidplug/peripheral.rs
Outdated
| }; | ||
| // Only consider the first characteristic of each UUID | ||
| // This "should" be unique, but of course it's not enforced | ||
| if !characteristics.contains(&char) { |
There was a problem hiding this comment.
What if there is another characteristic which has the same UUID but is different in some other way?
There was a problem hiding this comment.
It's dropped. Ideally, this should never happen, but the reality turns out that there are devices out there that do this.
Right now, the first characteristic is overridden, so in that respect its no worse (this change just picks the first one rather than the last).
I did consider changing the characteristic structures to Map<Uuid, Vec> (please excuse the pseudocode), but given that all the other methods use the Uuid as the key for performing characteristic operations, that'd mean that they'd all have to be able to take a UUID and an index (99.9% of the time, there's only going to be 1 item in the array). The trade-off between being able to address all characteristics (even with duplicate keys) vs the overhead of the interface changes to support 1 device felt way off to me.
There was a problem hiding this comment.
How? This code seems to be comparing the characteristic as a whole, not just the UUID, unlike the other platform implementations.
There was a problem hiding this comment.
That's a good question... Sorry, I was answering for the change as a whole, not the Android diff specifically.
Hmm... the comparator does look like it applies to all members of the struct, so the contains check probably never returns true. I guess the properties and then descriptors would define the order that btleplug presents the characteristics, and I'm guessing that at a higher level they'll get uniqued by UUID. I see the UUID is the only identifier passed across the JNI boundary, and the characteristics are held in an ArrayList on that side, so I think making this set UUID unique (first characteristic wins) will mean that the presented properties match up the the characteristic being addressed.
There was a problem hiding this comment.
@qwandor I've re-written and re-tested the Android portion of the change
blackspherefollower
force-pushed
the
dev
branch
from
ac68037
to
9bb16f3
Compare
blackspherefollower
force-pushed
the
dev
branch
4 times, most recently
from
4c7309c
to
aaab333
Compare
This meant that if a device (like the LOOB) declares multiple characteristics on a service with the same UUID, the later is used not the first. Ideally we'd have access to them all, but since we can't tell them apart at the higher levels first wins seems better than last wins.
blackspherefollower
force-pushed
the
dev
branch
from
aaab333
to
bcbc23a
Compare
|
Rebased and building, but still testing: please don't merge (I swear there was a button for making PR's draft/WIP...) |
|
I converted it to draft (It's in the "reviewers" section for some reason) |
Apparently I don't have the required memberships to see the option... I've tested on Windows/Linux/Android and we're all good. |
|
@qdot Apparently the "not a draft any more" button was right at the bottom of the page... |
If a device (like the LOOB) declares multiple characteristics on a service with the same UUID, the last declared characteristic is used not the first (which is what the LOOB requires us to control).
Ideally we'd have access to them all, but since we can't tell them apart at the higher levels first wins seems better than last wins.
This has been tested on Windows, macOS, Android and Linux