Skip to content

Conversation

@ruediger
Copy link

An attempt at adding a parser for EIR.

Copy link
Owner

@laptou laptou left a comment

Choose a reason for hiding this comment

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

Hello, and thanks for the contribution :) I have some notes.

Comment on lines +199 to +200
data.advance(data.remaining());
buf = data.into_inner();
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose of these two lines?

Copy link
Author

Choose a reason for hiding this comment

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

I had to restore buf after moving it earlier. Otherwise I get the following error.

error[E0382]: borrow of moved value: `buf`
   --> src/eir.rs:105:11
    |
102 | pub fn parse_eir<T: Buf>(mut buf: T) -> Result<EIR, EIRError> {
    |                          ------- move occurs because `buf` has type `T`, which does not implement the `Copy` trait
...
105 |     while buf.has_remaining() {
    |           ^^^ value borrowed here after move
...
121 |         let mut data = buf.take((len - 1).into());
    |                        --- value moved here, in previous iteration of loop
    |
help: consider further restricting this bound
    |
102 | pub fn parse_eir<T: Buf + Copy>(mut buf: T) -> Result<EIR, EIRError> {
    |                         ^^^^^^

error: aborting due to previous error

I'm fairly new to Rust. So there might be an obvious way of doing this I'm missing.

Copy link
Owner

Choose a reason for hiding this comment

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

It's not really clear why you need to call buf.take(). Avoiding .take() would solve the problem, as .take() is the call that moves buf. You can still do all of the bounds checking that you do later in the loop using just len, and if you need to create a contiguous series of bytes then you can do buf.copy_to_bytes().

Note that copy_to_bytes() was added in bytes v0.6; you will need to rebase your branch against master.

Comment on lines +45 to +55
#[derive(Debug)]
pub struct EIR {
flags: Option<BitFlags<EIRFlags>>,
uuid16: Vec<u16>,
uuid32: Vec<u32>,
uuid128: Vec<u128>,
name: Option<EIRName>,
tx_power_level: Vec<i8>,
uri: Vec<String>,
manufacturer_specific_data: Vec<ManufacturerSpecificData>,
}
Copy link
Owner

@laptou laptou Nov 28, 2020

Choose a reason for hiding this comment

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

Consider making this EIR type represent a single EIR tag, and then return a Result<Vec<EIR>> from parse_eir.

Then you could write EIR as a tagged enum:

#[derive(Debug)]
#[non_exahustive]
pub enum EIR {
    Flags(BitFlags<EIRFlags>),
    Uuid16(Vec<u16>),
    Uuid32(Vec<u32>),
    // ...
}

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I was wondering what the best API would be here. Enums and Vec (or maybe HashSet?) might make it a bit harder to match. But it feels clunky having a struct full of optionals. I'll try with Enums and see how it goes.

Copy link
Author

Choose a reason for hiding this comment

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

An attempt at changing the return type to Vec: ruediger@cc91569 Please take a look if you prefer this style.

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good! Thank you, and please keep it up. One note: EIR::Name has more than one property, and it's not very obvious what the boolean is for, so maybe consider making the properties of that one named?

  Name { name: String, complete: bool },

Also, should the name be a String or a CString? I don't think the Bluetooth spec says device names have to be in UTF-8, so it might be good to give the end-user a chance to deal with this themselves.

@laptou laptou linked an issue Nov 28, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to get simple name from device advertisment

2 participants