suggestion: equal processing for both 16- and 128-bit Uuids // WIP #259
Conversation
| } | ||
|
|
||
| impl FromMeta for Uuid { | ||
| fn from_string(value: &str) -> darling::Result<Self> { | ||
| if let Ok(u) = uuid::Uuid::from_str(value) { | ||
| let mut bytes = u.as_bytes().to_owned(); | ||
| bytes.reverse(); // Little-endian, as per Bluetooth spec |
There was a problem hiding this comment.
Note that here, endianess conversion is non-conditional. This seems like a bug to me, but those would be removed lines in the PR.
There was a problem hiding this comment.
No, this is correct. The uuid crate stores UUIDs in big-endian order, but the bluetooth spec requires UUIDs to be transmitted in little-endian order.
host/src/types/uuid.rs
Outdated
| @@ -9,7 +9,7 @@ use crate::codec::{Decode, Encode, Error, Type}; | |||
| #[derive(Debug, PartialEq, Clone)] | |||
| pub enum Uuid { | |||
| /// 16-bit UUID | |||
| Uuid16([u8; 2]), | |||
| Uuid16([u8; 2]), // Q: why here as slice, when 'u16' in 'host-macros'? | |||
There was a problem hiding this comment.
Is there a reason why the inner contents are arrays here, but u16 in host-macros?
Should we make them both the same way?
There was a problem hiding this comment.
@lulf Would appreciate your comment on this.
There was a problem hiding this comment.
[u8; 2] should be preferred because it has align of 1 instead of align of 2.
host/src/types/uuid.rs
Outdated
| Self::Uuid128(val.to_le_bytes()) | ||
| } | ||
| // tbd. this is an API change | ||
| pub const fn new_long_from_arr(val: [u8; 16]) -> Self { |
There was a problem hiding this comment.
There are two places (below, in the tests) that use this function, but it's also public.
Is there a need for both; can tests use long hex constants (instead of strings)? How would building from arrays / strings be unified, perhaps using From, see #248
There was a problem hiding this comment.
u128s are fine in a const context but tend to be inefficient at runtime on embedded. I think both of these constructors can be useful, but I'd probably call them something like from_u128 and from_bytes.
| @@ -9,10 +9,10 @@ mod common; | |||
| const CONNECTIONS_MAX: usize = 1; | |||
| const L2CAP_CHANNELS_MAX: usize = 3; | |||
|
|
|||
| const SERVICE_UUID: Uuid = Uuid::new_long([ | |||
| const SERVICE_UUID: Uuid = Uuid::new_long_from_arr([ | |||
There was a problem hiding this comment.
Another approach is:
const SERVICE_UUID: Uuid = Uuid::new_long(
0x00001000b0cd11ec871fd45ddf138840
);If turning to this, care must be taken to the endianness. Should the value be as above, or reversed?
There was a problem hiding this comment.
To match current behavior it should be reversed.
|
Looking good, make sure you check that the conversions are equivalent (maybe a test temporarily to show that?) or better if you think we've got an endienness bug? |
|
@jamessizeland Do you know why the CI build is failing? I have no idea what these are: |
Related to #248 , I'd like to see both 16- and 128-bit Uuid processing logic to be alike. I think this is a good deed for maintainability and clarity.
They currently differ in a few places, and I'd like to discuss those within the PR. If this gets thumbs, could massage the PR to mergeable quality.