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

Improve documentation of BLE types used in API #195

Open
lure23 opened this issue Dec 14, 2024 · 11 comments
Open

Improve documentation of BLE types used in API #195

lure23 opened this issue Dec 14, 2024 · 11 comments

Comments

@lure23
Copy link
Contributor

lure23 commented Dec 14, 2024

There's been one detail that caught my eye early on, looking at trouble:

    // Using a fixed seed means the "random" address will be the same every time the program runs,
    // which can be useful for testing. If truly random addresses are required, a different,
    // dynamically generated seed should be used.
    let address = Address::random([0x41, 0x5A, 0xE3, 0x1E, 0x83, 0xE7]);
    info!("Our address = {:?}", address);

This is likely the longest comment in the source base I've found. Instead of misusing ::random I would suggest:

  • separate Address::random() that generates an address, without needing to be fed a seed.

    My problem, as a newcomer to trouble, is that the example only shows the "non-random" code path. How should I generate a truly random address? Likely the answer is to generate a random seed of [u8;6]. But that's... a bit clumsy and verbose.

    Is there a reason why I, as a developer, would like to be in cahrge of the seed used for address generation, other than the testing case mentioned in the above comment?

  • separate Address::stable_for_testing() that would either give always the same address, or work as current random and require a seed to be provided.

Also, since these approaches are clearly either-or, perhaps there could be a stable_for_testing feature that decides, which path is built. Even then, I would prefer them to have different names. A big part of the confusion currently is using set_random_address (and other notions on randomness), when the address in the examples actually isn't that random.

@lure23
Copy link
Contributor Author

lure23 commented Dec 14, 2024

ps. Also, I'd be happy to get any guidance on where the randomness matters. I understand BLE but haven't read full specs.

@lulf
Copy link
Member

lulf commented Dec 15, 2024

I don't think that comment is entirely accurate and should probably be updated.

It is named random because that's the term the BLE specification uses for these kind of addresses where you don't have an official global assigned one. You're right it isn't random in the sense that it is generated randomly. It's random in the sense that it can be 'anything'. I believe the correct term to use here would be 'static random' address.

Trying to found the best authorative source, there is some more explanation about public vs random addresses here https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/CSS_v11/out/en/supplement-to-the-bluetooth-core-specification/data-types-specification.html

@lulf
Copy link
Member

lulf commented Dec 15, 2024

As for generating random addresses, have a look at the rand crate https://docs.rs/rand/0.8.5/rand/ - Maybe some helper function to ensure it meets the official requirements from BLE spec should be placed in trouble. But I don't think we should require them to be random, as these addresses usually come from some external source (like stored in device flash).

@lure23
Copy link
Contributor Author

lure23 commented Dec 15, 2024

For my repo, I would like to show both code paths - generated and fixed. For the actual device I work on, "static random" is way enough.

Thanks for explaining the background. If the comments can be reworded, that would be great! BLE is lot to learn, and I expect people to take similar route as me:

  1. general ~1h webinar/video
  2. read the source code

i.e. the audience would be not BLE-professionals but hobbyists wanting to ride its wave. 🏄‍♀️

@jamessizeland
Copy link
Collaborator

Yeah this is a comment I added in to try to disambiguate what this address was doing as I was initially confused that multiple projects I was trying working on were showing up as the same item in nrf_connect, even though I'd named them differently.

For testing a specific SKU I supposed you'd want this address to be static, and then unique for each provisioned device after that.

@lure23
Copy link
Contributor Author

lure23 commented Dec 15, 2024

..so it would actually come from the build system, not the source code needing to generate anything random.

Think I got it!

@lulf
Copy link
Member

lulf commented Dec 15, 2024

Yeah, having them static means that the ble_bas_peripheral example works out of the box with ble_bas_central without needing to do a scan and name lookup, it's more accurate in some ways.

In actual 'production' usage, you can read a register like FICR (on nrf) to derive a unique 'random' address.

@lure23
Copy link
Contributor Author

lure23 commented Dec 16, 2024

In the repo I'm working on I try to make it easy for makers to enter the esp-hal ecosystem and be productive, focusing on their solutions - not the ways to get there.

BLE is one of the areas the repo represents. What I'd like to reach is a production-grade sample, and this is where this discussion comes in. For trouble itself, it's enough to prove that the library works; and you guys have this "gray domain" know-how about how such keys should be generated, on the field. A newcomer (like me) to BLE lacks that knowledge.

Thus, I'd like my repo to work also as a training ground, showing ways that are "good enough" - and avoiding misguiding the person using it as their spring board to their own, awesome creations.

Currently, I chose to take the address bytes from [env], kind of showing they would come from outside the code base. I should really do what @lulf mentioned:

In actual 'production' usage, you can read a register like FICR (on nrf) to derive a unique 'random' address.

..but perhaps some other day?


EDIT: Using Address::random(esp_hal::efuse::Efuse::mac_address());

@lure23
Copy link
Contributor Author

lure23 commented Dec 19, 2024

In summary - and while closing this issue:

I think the real issue is in educating people not (wanting to be) deep in the BLE terminology, about the options - and what options not to take. See the AddressKind:

impl AddrKind {
    pub const PUBLIC: AddrKind = AddrKind(0);
    pub const RANDOM: AddrKind = AddrKind(1);
    pub const RESOLVABLE_PRIVATE_OR_PUBLIC: AddrKind = AddrKind(2);
    pub const RESOLVABLE_PRIVATE_OR_RANDOM: AddrKind = AddrKind(3);
    pub const ANONYMOUS_ADV: AddrKind = AddrKind(0xff);
}

These give me 0 understanding as to when to use them. Which means I am next reading specs - not really for this, but for other reasons.

TrouBLE could be in the nice position to both implement and educate users, for example by a tutorial, or code docs about where and how to use each thing. Even better, there could be a full library (that's just 1-to-1 API complete with the specs) and a limited set (the 20% things you actually need) that filters out unnecessary clutter, enhancing the IDE "fill it for me" experience.

@lure23 lure23 closed this as completed Dec 19, 2024
@lulf
Copy link
Member

lulf commented Dec 19, 2024

I agree! If you don't mind, let's keep this issue around and change the title

@lulf lulf reopened this Dec 19, 2024
@lulf lulf changed the title FR: Address::stable_for_testing? Improve documentation of BLE types used in API Dec 19, 2024
@jamessizeland
Copy link
Collaborator

This is a good point. I've been learning BLE as part of my contribution to this project, and trying to improve what I like to call the "route 1" user experience. Thanks for pointing this bit out! We want this to be accessible to newcomers.

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

No branches or pull requests

3 participants