-
Notifications
You must be signed in to change notification settings - Fork 7
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
Native BLE transport plugin #487
Conversation
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 awesome. I'm excited to try it. There are a few clarification questions but it looks good to me!
) | ||
return | ||
|
||
parser = IOTileReportParser(report_callback=self._on_report, error_callback=self._on_report_error) |
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 does work because parser is indirectly referenced in the closure you create so it won't be garbage collected but I think it's cleaner to have an explicit reference to it around, so I would attach it to the context object for the connection.
|
||
if result['length'] > 0: | ||
# Register the payload notification callback | ||
self._register_notification_callback( |
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.
Are you sure there is not a race condition here? Depending on the loop structure inside bable python that could be the case but I just want to verify. Is there no way both of the header and payload notifications could be processed before this callback is triggered to register a listener on the second notification?
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 was wondering the same thing a few days ago and then I forgot to verify. It never happens during my tests but I am not sure enough to tell you that it is impossible. So, I will definitely register the payload callback before and remove it if it is not needed anymore. Would be better for sure :)
ArchManuID = 0x03C0 | ||
|
||
# GATT table | ||
BLEService = Service(uuid='1800', handle=0x0001, group_end_handle=0x0005) |
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.
Are these handles and group_end_handle
only used on the virtual interface? If so that's totally fine but if you key in on the handles for PODs as well that will break in the future as soon as we tweak a ble characteristic on the POD firmware and the handles are reassigned. In that case I wouldn't fix it now but file an issue to we remember to fix it in the future when we make such a change, which won't be too soon.
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 is just that currently bable_interface.Service object are not linked to bable_interface.Characteristic: there is no hierarchy because it was quite annoying to serialize/unserialize then. But it is definitely something to change in baBLE: add an add_characteristic
method to add a characteristic to a service and then, automatically calculate the group_end_handle
, and the Characteristics handle
,value_handle
, config_handle
.
This information is needed to register the GATT table so I need them only in the virtual interface.
So yes for now, we'll have to keep this data the same as the GATT table registered in the PODs. I'll create an issue
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.
After thinking about that, we'll always have to keep this updated when modifying the GATT table in the POD firmware, just like we have to in the BLED112 transport plugin (cf virtual_bled112.py#L50).
So even if adding the Service/Characteristic hierarchy would be great (cf issue here), it won't solve this issue.
Here it is! The new transport plugin using native BLE thanks to baBLE.
It basically works like any transport plugin, to use it simply run:
For now, it only works on Linux.
PS: to make the tests passing, although it only works on Linux, I had to modify the root
tox.ini
and thescripts/test.py
script to skip tests if on Mac or Windows.