Add ble gatt server

[elotom]

I recently came across bluedroid GATT server implementation, which was done by @persello, and I believe it is a solid foundation for future improvements. With your permission, we would like to integrate the code with esp-idf-svc to make it more accessible to the wider community. This would enable more people to contribute to the project and help it evolve over time.

#55

[ivmarkov]

@svenstaro Asked me to summarize what still needs to be done in this PR, so here it comes:

In a nutshell, the following 4 things should be addressed:

• The singleton pattern for the BT server should be drastically simplified. Basically, no singleton is necessary. The server should just take as an input the modem peripheral from esp-idf-hal which is already a singleton. This way - and by extension - the server would be a singleton as well. This pattern is used for everything, including wifi and eth for example.
• Please remove any usage of STD stuff which is not necessary. esp-idf-* is actually no_std 99.9%. A lot of the collections are actually available in the alloc module too but even this should be done with care. A lot of time people reach out to those where a simple heapless Vec would do as well
• Remove any Arcs Mutexes and RwLocks from the API. Ideally these should be hidden and the API should be simple. Or these (being STD) should not be used at all, or if possible, the STD Mutex should be replaced with esp-idf-svc's own mutex. Using RwLock on ESP IDF does not give benefits. And so on.
• Please remove any Vecs from the public APIs too. These should take simple refs or simple ref muts.

Now, I get it that it is a lot of work, but:
a) All other code in the esp-idf-* crates follows these conventions and I feel making exceptions will bring more confusion for the users, especially given that our documentation sucks and
b) I don't know how the maintenance of this code will turn out in future; you guys are contributing a piece of code that was authored by another guy; is he willing to support it in esp-idf-* would you support it? I'm unsure; and given the lack of visibility, I would appreciate if this code looks as much like the rest of esp-idf-* as possible.

[persello]

Answering part of question B: I’m the author of the original library, and even if I had more development plans for it (new features and an architectural refactoring), unfortunately I don’t have time to maintain it due to some academic projects going on right now.

Code quality is extremely low in most parts of that crate, as it was my first Rust project, and I agree that a full refactoring might be necessary, and I don’t know whether it would be even worth it before creating a solid high-level API (or trait set).

[ivmarkov]

@persello Thanks for reaching out. Well, we do not necessarily need trait-based APIs (ala embedded-svc) to kickstart this. These might emerge later.

Also, you might be underestimating the quality of your code. I still believe that the changes I'm suggesting above might be a good (enough) set to get something merged. But they are also a requirement, i.e. I'm not willing to merge anything without this minimal cleanup. So I guess until/if somebody is willing to implement these changes, and commit some support behind this driver, this PR will rot. :)

[svenstaro]

We'll set someone to clean this up soon. Just gotta make a delivery first. :)

[MabezDev]

@svenstaro Any luck finding some time to work on this?

[svenstaro]

As per Matrix, we're currently out of capacity due to problems outside our control - We'll schedule someone onto this ASAP.

[ivmarkov]

Some updates:
I've started addressing some of the issues outlined above ^^^ in this branch.

I've only invested an hour or so, mainly in re-assembling the event callbacks which were scattered in many files, as well as moving some minor stuff from using type alises in std to the actual core impls.

The thing is the following: getting rid of the Arc<RwLock<Vec<Arc<RwLoc<... stuff will require a bit deeper changes.

These seem to come from the fact that the Bluedroid stack seems to spin its own event loop in a separate thread and user gets events via a user-provided callback on that separate thread (need to confirm this hypothesis but seems very likely).

While Arc-ing + Mutex-ing everything is a no-brainer strategy, it is way too heavyweight for an API, which just exposes the existing, raw, crude, callback-based Bluedroid API - albeit in safe wrappers. A better strategy that would fit the raw callback-based API which is exposed is to push into the Gatt Server a FnMut(...) + Send event callback which - obviously - would be "sent" to the event loop thread - possibly - with the registered profiles or bits and pieces of those, and from there on, everything would live on the Bluedroid event thread. As in the user would be responsible - in the event thread - to create and register the services for a profile, its characteristics and so on. But since these would be created on the event thread, and would live only there, there would be no need to Arc<Mutex<>> the world. If the user needs to communicate with stuff which lives on another thread - it should be up to the user to properly Mutex and maybe Arc his stuff, the BT wrapper framework should not get into this business.

So that's one big change. The other is that I'm not sure that splitting the giant gatt server event callback into sub-functions of on_this and on_that type has any benefit. What is the point? I would rather prefer a one giant, type-safe Event enum, on which I could type-safely match in Rust code, just like the C code matches (type-unsafely) on the raw C event enum. This also allows for a very simple first-pass at an async API (see below), in the form of a single pub async fn next_event(&mut server) -> Event method.

Once we have this in place, we can think about exposing more convenient API on top of the callback based one. An async API would be an obvious choice as this is the way to linearise in Rust a callback hell into something which uses regular control flow. Especially appealing if I find a way to schedule a piece of work (delayed execution) on the Bluedroid event thread. Usually, libraries which spin their own event loop (Matter C++ SDK; gstreamer / glib and so on) provide such facilities. It would be quite a blow if Bluedroid doesn't. That - in turn - would allow the implementation of a very trivial async executor that lives on the Bluedroid thread and would be the natural choice to run async work on that thread.

And sync can be further layered on top of async somehow too maybe.

With all of the above said, I can't help but give BIG kudos to @persello for properly documenting the code and properly instrumenting it - thanks so much! A joy to read and comprehend!

[ivmarkov]

Hmm, on a second thought, integrating a custom async executor into the event loop of Bluedroid might be more difficult than I thought. I was thinking of poll-ing the executor directly from the event callback, but that might not work due to backpressure to answer that particular event. Weird. Need to think if that's possible at all, and look into how Rust wasm does it with the JS event loop. Also, this other BT implementation of @pyaillet might be a source of inspiration as well, as its async-first.

[ivmarkov]

Yeah. Integrating async with the Bluedroid event loop won't be so easy. The rest of the simplifications still apply though, I think.

[ivmarkov]

@pyaillet got a much terser code by giving up (I think) on associating the Rust structures for Service, Profile Characteristics, Connection etc. with their corresponding handles returned by the Bluedroid stack. Sounds quite an appealing simplification to me, as that means getting rid of a lot of code and arcs and locks. Do we really need this association, and for what?

The other thing I noticed is that it is apparently OK to call Bluedroid functions on the main thread. It is just that you receive event callbacks in another thread.

@pyaillet, can you shed some light here? @persello ?

[ivmarkov]

Closing in favor of #290

If you guys want to work on adding BLE support, please work on the currently commented out module esp_idf_svc::bt::ble.