Skip to content
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

python-for-android backend #422

Merged
merged 43 commits into from
Oct 17, 2021
Merged

python-for-android backend #422

merged 43 commits into from
Oct 17, 2021

Conversation

xloem
Copy link
Contributor

@xloem xloem commented Jan 14, 2021

This is my past week or so of work on a python-for-android backend. Python-for-android is primarily used by kivy, a python gui toolkit.

This code is working for me, but I haven't tested features my test device doesn't use, such as pairing. There's value to merging this code, because I'm not aware of any other python-for-android bluetooth libraries, maybe with a comment to report to me any bugs encountered, with logcat output, or somesuch.

If this isn't a good addition to this project, the python-for-android project might consider it as a pull request, too, as part of their android interface library.

I'd appreciate criticism on my implementation choices. Most of my energy was spent just making it work.

My development history on this PR is at https://github.com/xloem/bleak/tree/p4a .

EDIT: lint failures have been addressed

and an example for kivy buildozer on android
@hbldh
Copy link
Owner

hbldh commented Jan 25, 2021

Good work! This is interesting, but it needs some additional handling first. It needs some documentation and some clear descriptions on how to use.

Other questions:

  • Is it only for python-for-android?
  • Does it (or at least, can it) work with Beeware's Android solution as well?
  • Does it only work with Kivy? Are there other ways of running it on Android?

@xloem
Copy link
Contributor Author

xloem commented Jan 25, 2021

  • It uses pyjnius which I've mostly seen used for python-for-android. I recently (after coding this up) discovered the proprietary android app pydroid 3 which does on-mobile python development and pyjnius/android also works. I haven't tested this PR with pydroid 3.
  • I haven't implemented beeware, as I'm less familiar with it and it uses different interfacing libraries for the android parts. Due to Add support for Java subclassing beeware/rubicon-java#25 the solution would presently require making a custom beeware template that includes the auxiliary java classes, and requiring apps build with that template instead of their normal ones. See also Android WebView lacks several features due to inability to subclass beeware/toga#1020 . My thought was to polish up p4a first, and then add beeware some other time.
  • I believe it is possible to use python-for-android without kivy or pydroid3. I haven't done this myself: you'd have to build or connect with a UI, for the user to see the application.

Is it pretty obvious what docs need updating here, for when I come back to commit changes?

EDITS to self:

  • note that making pyjnius work in beeware is not that helpful; what is missing the most is setting up a template with custom java classes; i don't know how hard that is.

@hbldh
Copy link
Owner

hbldh commented Jan 26, 2021

Thanks for the answers; I would very much like to make Bleak compatible with Beeware solutions eventually, so I wanted to hear your thoughts on that!

The docs that need updating are here and then adding api-references here. In combination with the example and README that you have already written, could you please include some links or references on where to start reading about how to get python-for-android onto the phone?

Also, pyjnius needs to be added to requirements.txt and to setup.py so that they will be installed if it is an android system, right?

@xloem
Copy link
Contributor Author

xloem commented Jan 27, 2021

@hbldh I threw something together for docs. The p4a backend uses the pyjnius and android packages, but python-for-android already installs both of these itself, and they are not needed on other platforms. python-for-android reads as 'linux' in sys.platform so it would be an extra manual check for environment variables in setup.py ... still something valuable?

@hbldh
Copy link
Owner

hbldh commented Mar 1, 2021

@xloem Sorry for taking time to merge this. I am interested in adding this, but there are several other things needing triage and subsequent release. I am planning on making a 0.11.0 release with a lot of potentially problematic code, and I want to wait with this until after that release to merge this to develop. Just so you know.

@jeffsf
Copy link

jeffsf commented Jun 3, 2021

I have only glanced at the code at this point, so I can't comment on its technical correctness.

Having worked struggled with BLE on the Android stack, one thing that is clear is that it is not robust and not "asynchronous" in the way one might hope. My own explorations have aligned well those reported elsewhere that the application layer needs to serially send commands and check for completion before sending another command. At least for that application, I needed to manage matching ACKs with requests explicitly to prevent BLE failures.

Here are some references I found (I was unable to confirm if it is one per CUUID, device, or stack before abandoning the third-party code base I was working with.)

Android seems to have a limit of one pending request
If we're lucky, it is per device

(2016) https://www.toothpasteandbubblegum.com/blog/queued-data-bluetooth-le-android
(2017) https://stackoverflow.com/questions/18011816/is-the-native-android-ble-implementation-synchronous-in-nature
(2016) https://github.com/NordicPlayground/puck-central-android
(2020) https://punchthrough.com/android-ble-guide/

  • Reaffirms that everything needs to be done serially
  • Confirms that failures need to be handled externally

Multiple references at and linked from the recently updated https://www.stkent.com/2017/09/18/ble-on-android.html_

To help prevent others' frustrations, this should at least be documented.

@hbldh
Copy link
Owner

hbldh commented Jun 7, 2021

@jeffsf Thank you for your comments. I will take this under consideration; I have not tested this Android solution myself yet and need to confirm that it does what it needs to be included in Bleak.

@rocketstrong600
Copy link

i have been testing his commits and they seem to be working well i was able to write a app that talks over nordic ble uart

@dlech
Copy link
Collaborator

dlech commented Oct 8, 2021

Is there a way to get this up and running in 15 minutes or less so we can test it easily? I've looked a few times but I don't see a way to just run a simple command line python file on Android.

@xloem
Copy link
Contributor Author

xloem commented Oct 8, 2021

It takes longer than 15 minutes for python4android or buildozer to install. I might be able to find time to set up a docker image or something, do you need that? Otherwise, see https://github.com/hbldh/bleak/blob/7aa7532fe28f4341746762a4ff94980833f5baa0/examples/kivy/README

@dlech
Copy link
Collaborator

dlech commented Oct 8, 2021

I didn't realize that buildozer magically does most of the work for you. I think that should be good enough to get me started.

The arch argument of get_site_packages_dir() is no longer optional. Comes from changes in kivy/python-for-android#2467
@dlech
Copy link
Collaborator

dlech commented Oct 8, 2021

How do you get buildozer to rebuild bleak when changes are made?

@xloem
Copy link
Contributor Author

xloem commented Oct 8, 2021

I'm so sorry, I don't remember, you'd have to ask in python4android communities. But I think it's pretty reliable to delete the buildozer build folder, and I think there are some specific subfolders inside it you can delete instead to speed the rebuild up. I'm not sure what they are, I'm afraid.

Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a fairly good look at this now and it looks really good. I pushed a bunch of changes to bring things up to date with the develop branch and get it working again and fix a few bugs. I still have a few questions (see inline comments).

Also see the CI results for some issues with the docs.

examples/kivy/main.py Outdated Show resolved Hide resolved
bleak/backends/p4android/defs.py Outdated Show resolved Hide resolved
bleak/backends/p4android/defs.py Outdated Show resolved Hide resolved
bleak/backends/p4android/descriptor.py Outdated Show resolved Hide resolved
bleak/backends/p4android/scanner.py Outdated Show resolved Hide resolved
bleak/backends/p4android/client.py Outdated Show resolved Hide resolved
bleak/backends/p4android/client.py Outdated Show resolved Hide resolved
bleak/backends/p4android/client.py Outdated Show resolved Hide resolved
.github/workflows/build_and_test.yml Show resolved Hide resolved
examples/kivy/README.md Outdated Show resolved Hide resolved
Building from this directory as suggested in the README creates local folders that should be ignored.
This was converted from a method to a property in
hbldh#495
This change was made in other backends in 0f56dab.
This was checking disconnected instead of connected
This was implemented for other backends in 62f45b0.

Since Android doesn't automatically negotiate MTU, we have to do that when connecting to the device.
If there is a failure after connecting but before the connect method returns, we need to disconnect, otherwise we end up in a bad state.
returning a bool from the disconnect() method is for backwards compatilbity only and should always be True. If disconnecting failed, an exception should be raised.
asyncio.run() does the same thing, but probably better.
Using a context manager simplifes the code.
@xloem xloem marked this pull request as ready for review October 9, 2021 23:38
@xloem
Copy link
Contributor Author

xloem commented Oct 9, 2021

had a marathon today of addressing most of these things. haven't figured out the recipe building thing yet at this time.
EDIT: recipe thing addressed now.

Comment on lines 50 to 60
with:
path: |
~/.buildozer
examples/kivy/.buildozer
key: build-cache-buildozer
- name: Build Kivy example
working-directory: examples/kivy
run: buildozer android debug
- name: Clean bleak recipe for cache
working-directory: examples/kivy
run: buildozer android p4a -- clean_recipe_build --local-recipes $(pwd)/../../bleak/backends/p4android/recipes bleak
Copy link
Collaborator

@dlech dlech Oct 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the clean be before the build so that bleak is rebuilt? We can also add a check to avoid the clean if there is no cache.

Suggested change
with:
path: |
~/.buildozer
examples/kivy/.buildozer
key: build-cache-buildozer
- name: Build Kivy example
working-directory: examples/kivy
run: buildozer android debug
- name: Clean bleak recipe for cache
working-directory: examples/kivy
run: buildozer android p4a -- clean_recipe_build --local-recipes $(pwd)/../../bleak/backends/p4android/recipes bleak
id: buildozer-cache
with:
path: |
~/.buildozer
examples/kivy/.buildozer
key: build-cache-buildozer
- name: Clean bleak recipe for cache
if: steps.buildozer-cache.outputs.cache-hit == 'true'
working-directory: examples/kivy
run: buildozer android p4a -- clean-recipe-build --local-recipes $(pwd)/../../bleak/backends/p4android/recipes bleak
- name: Build Kivy example
working-directory: examples/kivy
run: buildozer android debug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, but it does make the cache slightly bigger which slightly increases the time to download it. I imagine the difference is negligible. The end result is the same.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean. Fine to leave it as-is then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: move recipe cleaning to after compilation, so that the recipe is not included in the cache. Toggle the condition so it only runs if the cache was missed.

bleak/backends/p4android/characteristic.py Outdated Show resolved Hide resolved
bleak/backends/p4android/scanner.py Outdated Show resolved Hide resolved
… PR recommendation

Co-authored-by: David Lechner <david@lechnology.com>
Comment on lines 31 to 36
SCAN_FAILED_NAMES = {
ScanCallback.SCAN_FAILED_ALREADY_STARTED: "SCAN_FAILED_ALREADY_STARTED",
ScanCallback.SCAN_FAILED_APPLICATION_REGISTRATION_FAILED: "SCAN_FAILED_APPLICATION_REGISTRATION_FAILED",
ScanCallback.SCAN_FAILED_FEATURE_UNSUPPORTED: "SCAN_FAILED_FEATURE_UNSUPPORTED",
ScanCallback.SCAN_FAILED_INTERNAL_ERROR: "SCAN_FAILED_INTERNAL_ERROR",
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize before that the java code doesn't have enums. So instead of the string lookup like this, another option could be to make a Python enum. Then we can just cast the values returned from java code to this type. Then the enum value can be passed directly to format strings and magically be converted to a string without having to manually do the conversion.

Suggested change
SCAN_FAILED_NAMES = {
ScanCallback.SCAN_FAILED_ALREADY_STARTED: "SCAN_FAILED_ALREADY_STARTED",
ScanCallback.SCAN_FAILED_APPLICATION_REGISTRATION_FAILED: "SCAN_FAILED_APPLICATION_REGISTRATION_FAILED",
ScanCallback.SCAN_FAILED_FEATURE_UNSUPPORTED: "SCAN_FAILED_FEATURE_UNSUPPORTED",
ScanCallback.SCAN_FAILED_INTERNAL_ERROR: "SCAN_FAILED_INTERNAL_ERROR",
}
class ScanFailed(IntEnum):
ALREADY_STARTED = ScanCallback.SCAN_FAILED_ALREADY_STARTED
APPLICATION_REGISTRATION_FAILED = ScanCallback.SCAN_FAILED_APPLICATION_REGISTRATION_FAILED
FEATURE_UNSUPPORTED = ScanCallback.SCAN_FAILED_FEATURE_UNSUPPORTED
INTERNAL_ERROR = ScanCallback.SCAN_FAILED_INTERNAL_ERROR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: convert this to an enum. the other change to suppor tthe suggestion is changing the dictionary lookup in SCAN_FAILED_NAMES to a constructor call to ScanFailed in bleak/backends/p4android/scanner.py
TODO: it might make sense to bring back the other name dictionary and turn it into an enum, to keep code maintenance more parallel and improve debuggability, but it is not necessary

xloem and others added 9 commits October 10, 2021 15:37
The values in CONTROLLER_ERROR_CODES overlap with PROTOCOL_ERROR_CODES
so we will need a separate lookup table if these error codes are
actually needed.
To be like other backends, use bytes instead of bytearray for service
and manufacturer data. Also simplify code a bit while we are touching
this.
@dlech dlech merged commit 87773f2 into hbldh:develop Oct 17, 2021
@dlech
Copy link
Collaborator

dlech commented Oct 17, 2021

Merged! Thanks again for all of the amazing work!

@dlech
Copy link
Collaborator

dlech commented Oct 17, 2021

I also forgot to say that you should feel free to add yourself to the AUTHORS file if you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants