-
Notifications
You must be signed in to change notification settings - Fork 79
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
GATT server support for Rust #454
base: main
Are you sure you want to change the base?
Conversation
2a6aedd
to
e823482
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much motivation I have to fix the build with Python 3.8 and 3.9 given that 3.10 is almost 3 years old...
attribute: Attribute, | ||
value: Optional[bytes] = None, | ||
force: bool = False, | ||
) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added type annotations based on poking around at the callers, but since of course the type info isn't pervasive I'm not sure these are right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. We want to eventually have everything type, incrementally.
|
||
# Dev tools | ||
file-header = { version = "0.1.2", optional = true } | ||
globset = { version = "0.4.13", optional = true } | ||
|
||
# CLI | ||
anyhow = { version = "1.0.71", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer optional due to wanting a "don't care" error for callbacks, etc, that wasn't the Python-specific PyError
|
||
let mut builder = AdvertisementDataBuilder::new(); | ||
builder.append(CommonDataType::CompleteLocalName, "Bumble Battery")?; | ||
builder.append( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fairly manual, but given the use cases for Bumble that might want to do somewhat exciting things vis-a-vis spec compliance, I'm not sure it makes sense to make a more OS-style API that doesn't require you to assemble the advertisement and prevents you from doing things wrong...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a set of reusable AdvertismementData pre-set builders would be nice, for users who what just basic common stuff. That's something I've considered adding to the Python lib but haven't found the time to yet.
/// | ||
/// This is a stop-gap until rootcanal's build is improved to the point that a rootcanal-sys crate, | ||
/// and using rootcanal as a library rather than a separate process, becomes feasible. | ||
pub(crate) async fn run_with_rootcanal<O, F>(closure: F) -> anyhow::Result<()> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hchataing forgive me for these rootcanal crimes. Let me know if there is a less bad way to do this while I try to get the rootcanal build working on more systems.
/// Execute `closure` with a simple directory lock held. | ||
/// | ||
/// Assumes that directory creation is atomic, which it is on most OS's. | ||
async fn with_dir_lock<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed since tests run concurrently, and I don't want to sacrifice that as integration-level ones are rather slow
use pyo3::types::PyBytes; | ||
use pyo3::{intern, PyAny, PyResult, Python}; | ||
|
||
impl TryToPy for AttributeUuid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure how it'll work out, but I'm experimenting with having Try[To|From]Py
in the wrapper
module distinct from the relatively pure Rust structs in crate::internal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 this makes sense to me. If it causes confusion later, we can move things around.
e823482
to
bf9eb05
Compare
|
||
// Needed for Python 3.8-3.9, in which the Semaphore object, when constructed, calls | ||
// `get_event_loop`. | ||
wrap_python_async(py, device_ctor)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to sprinkle this on Device and Peer invocations that end up instantiating an asyncio.Semaphore
}) | ||
} | ||
|
||
/// Returns the path to a rootcanal binary, downloading it from GitHub if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a little odd to me that this is downloading artifacts at runtime, would it make sense to move this step into a build.rs and setting this up at build time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't enjoy the current approach, but I don't see a great way of doing it with build.rs
either that would work nicely with doing dirty tracking. I hope either way it's short lived, though -- with n
additional hours I think I can get rootcanal
building as a library, at which point a rootcanal-sys
crate becomes reasonable, and this whole spawn-a-process thing can go away.
bf9eb05
to
e521e68
Compare
use pyo3::types::PyBytes; | ||
use pyo3::{intern, PyAny, PyResult, Python}; | ||
|
||
impl TryToPy for AttributeUuid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 this makes sense to me. If it causes confusion later, we can move things around.
Also, although we've not quite kept up-to-date with the Changelog up to this point, could we at least make note of these changes in the Changelog now, so they're there for the next release? |
c8cb4d9
to
e3f3cfc
Compare
With a whole lot of support to get there: - Device::add_service(), with some complexity around how to represent the mutable Python `Service` being used under the covers - AttributePermission, CharacteristicProperty, and other supporting GATT-related stuff - Ability to handle the different data types used by various Python `Characteristic` subclasses - Support for running tests in the context of a background `rootcanal` process - `TryToPy` and `TryFromPy` traits to help make the lifecycle more clear for structs that have a Python equivalent, but are also meaningful standalone - Add example with a minimal battery service - Rearrange known service IDs to make it possible to refer to a service UUID in a meaningful way - Make Address hold its own state to avoid propagating PyResult so much, and adopt the new TryToPy and TryFromPy traits - `AdvertisingDataValue` trait so users don't have to remember as many ADU encoding details at a call site - Sprinkle in more wrap_python_async to work on Python 3.8 and 3.9
e3f3cfc
to
19dc37d
Compare
With a whole lot of support to get there:
Service
being used under the coversCharacteristic
subclassesrootcanal
processTryToPy
andTryFromPy
traits to help make the lifecycle more clear for structs that have a Python equivalent, but are also meaningful standaloneAdvertisingDataValue
trait so users don't have to remember as many ADU encoding details at a call site