-
Notifications
You must be signed in to change notification settings - Fork 1.3k
nrf: Split the ble module into a shared part and the port implementation #1014
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
Conversation
This is very clean. I am thinking that maybe we want to make a class that lives under
etc. Later there could be other things that live under Some other examples of this kind of singleton class in CircuitPython are |
Yes, I wanted to use a property for enabled and address but had two hesitations: 1. would need a class, 2. would break the current API. Regarding having a class, I think best name would be |
Let me know if you want me to move the functions to a Adapter singleton. |
I vote to use a singleton. We'll break compatibility with upstream due to our use of properties anyway. We'll break with So, lets do what we think is best. Thanks! |
Woo, let the Bluetooth overhaul begin! Expect many PRs soon :D |
So we agree on |
Ya, |
|
|
3e4ee5c
to
b8ca21d
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.
This looks excellent to me! Just query and one minor change.
shared-bindings/bleio/Adapter.c
Outdated
STATIC mp_obj_t bleio_adapter_get_enabled(mp_obj_t self) { | ||
const bool enabled = common_hal_bleio_adapter_get_enabled(); | ||
|
||
return enabled ? mp_const_true : mp_const_false; |
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.
You can do return mp_obj_new_bool(common_hal_bleio_adapter_get_enabled());
. This is common in other shared-bindings
implementations.
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.
Didn't know that one, will fix!
#ifndef MICROPY_PY_BLE_NUS | ||
#define MICROPY_PY_BLE_NUS (0) | ||
#ifndef MICROPY_PY_UBLUEPY | ||
#define MICROPY_PY_UBLUEPY (0) |
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.
Q: are we going to get rid of ubluepy
at some point or does it serve some purpose in other nrf ports?
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.
Yes, next step is port ubluepy
classes to bleio
, I just like to keep things separated and do them one step at a 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.
OK, sounds good!
This allows other ports to implement these shared bindings.
b8ca21d
to
05c1384
Compare
@dhalbert Fixed! |
This allows other ports to implement these shared bindings.
This is one part of #995