-
Notifications
You must be signed in to change notification settings - Fork 52
Improve documentation of BLE types used in API #195
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
Comments
ps. Also, I'd be happy to get any guidance on where the randomness matters. I understand BLE but haven't read full specs. |
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 |
As for generating random addresses, have a look at the |
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:
i.e. the audience would be not BLE-professionals but hobbyists wanting to ride its wave. 🏄♀️ |
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. |
..so it would actually come from the build system, not the source code needing to generate anything random. Think I got it! |
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. |
In the repo I'm working on I try to make it easy for makers to enter the 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 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.
EDIT: Using |
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
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. |
I agree! If you don't mind, let's keep this issue around and change the title |
Address::stable_for_testing
?
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. |
A random observation about the |
@chrissuozzo Another way to fix that is by adding the "Service Changed" characteristic (see #180 for a discussion about it) to the 0x1801 service. Relevant part in spec: https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/Core-54/out/en/host/generic-attribute-profile--gatt-.html section 2.5.2 We could probably make this easier to adopt via some flag in the GapConfig or similar. |
Basic docs up now, so we can add anything not part of rustdoc to that https://embassy.dev/trouble/ |
Great to see a document for using Trouble - this will help newcomers. I spotted a few typos etc. (happy to provide the list, separately), but would like to ask about the focus / aim for this document. The reader likely knows about Trouble (the library), since they came here. It says:
I think this space could be used to even more detailed introduce both for the reader and authors the intended scope of the document. Bluetooth and BLE are complex technologies, and there would be a temptation to keep adding things to the document. It's always easier to add, than to reject or remove. Thus, I suggest that there should be a sentence or two about the scope and purpose of this book. It should be enough to create an application (if knowing Rust), but not a tutorial on BLE, itself. This would help maintaining of the document, since one could test:
A list of "Resources" at the end could help in finding more details. |
@lure23 Very good suggestions. I agree with you that the scope should not be to cover that much BLE, but I added the concepts page to explain some of the terms that is used in the docs. Other than that, it should focus on trouble, how it works, how to configure and how to tune. Also I'm careful not to provide too much code examples since they tend to get outdated pretty fast, so we should point to examples that get built (and some of them get automatically tested!) as much as we can. Feel free to add to the docs though, it's all here: https://github.com/embassy-rs/trouble/tree/main/docs - the structure should be pretty self explanatory (and asciidoc is pretty similar to markdown). |
This comment has been minimized.
This comment has been minimized.
I notice I've taken the Issue in wrong direction. This was about a very narrowly defined problem of finding something about the Currently, the "book" (what GitHub calls documentation) mentions this, which is helpful:
However, this is different from the API docs (what GitHub refers to as "rust-docs"):
Sorry for pointing out problems ahead of their time, but.. I already find it confusing that there is "documentation" and "rust-docs". What do Rust projects normally call these? "Book" and "API docs" is what I used above. I'll keep this Issue for the topic mentioned in its title. |
Usually the API docs are specific towards the types and source code you see, and shown by your IDE etc. The "book" is more higher level that you can't really document in the rustdoc because it doesn't really belong to a particular piece of code. |
@lulf Sure. I'm just trying to help you keep things under control, unfortuntely not being able to contribute directly. Should we close the issue - what's your liking? |
There's been one detail that caught my eye early on, looking at
trouble
: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 currentrandom
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 usingset_random_address
(and other notions on randomness), when the address in the examples actually isn't that random.The text was updated successfully, but these errors were encountered: