-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support for Bluetooth on all platforms #33
base: master
Are you sure you want to change the base?
Conversation
…ecting to the Radiacode via USB
…th asyncio. Added a find-radiacode.py script to help find a Radiacode over bluetooth. Added a simple Logger class for nicer outputs
I refactored the main
Synchronous usage (unchanged):
Asynchronous usage:
In short, to use the library with This should remove the main concern I initially had that was to adjust existing apps to support asyncs. 🎉 |
…self and added a separate event loop in Bluetooth() to avoid the use of an asyncio.run()
…eptions in the backend
…library in both synchronous and asynchronous way
…itself. Added non-destructive integration tests
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.
Hey, I've finally found time to take a look here.
I think after we discuss and resolve the mentioned issues, I'll merge it into master. Then, after some time, I'll make a new release.
else: | ||
print('Connecting to Radiacode via USB') | ||
rc = RadiaCode() | ||
except DeviceNotFoundBT as e: |
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 think it's a bit overcomplicated with examples using mutiple except smth as e: print(e)
- probably we want to just use generic case Exception
everywhere or show all types of exceptions in only one example (basic.py?).
@@ -1,141 +1,197 @@ | |||
<!DOCTYPE html> | |||
<html> |
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.
Have you changed anything in this file? If it's only a format change, I would prefer not to do it in this MR as it's not related to "Bluetooth on all platforms"
def batch_read_vsfrs(self, vsfr_ids: List[VSFR]) -> List[int]: | ||
return self.loop.run_until_complete(self.async_batch_read_vsfrs(vsfr_ids)) | ||
|
||
def status(self) -> str: |
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 find it better to group async + sync methods:
async def async_status(self) -> str:
...
def status(self) -> str:
...
async def fw_signature() -> str:
...
def fw_signature() -> str:
...
Edit: check my last comment as with the last commit, no code changes are necessary and the library supports
async
as well.I'd like to request a review of my commits, the aim of these changes is to add bluetooth support for all platforms since the original version doesn't work on Mac OS. In order to achieve that I had to remove
bluepy
and replace it withbleak
.find-radiacode.py
to help users find their Radiacode via Bluetooth and show its MAC Address or UUID (depending on the platform).synchronous
use, checkbasic.py
in the examples folderasynchronous
use, checkwebserver.py
in the examples folderNote: I couldn't make the library work (not the
original
nor my fork) onWSL
. Bluetooth is not available on WSL and I couldn't make it work even over USB.I've tested the three scripts on the following platforms.
Windows (both Bluetooth and USB)
Apple M1 and Intel (both Bluetooth and USB)
Linux (Raspberry PI and Ubuntu, both Bluetooth and USB)
Further Testing & Feedback
Please let me know if others would be willing to test it to confirm that I haven't missed anything. I tried to be thorough but I had to work on a restricted schedule, so it's more than likely that I've missed something that didn't come up during my own tests.
I would really appreciate knowing if the community and the maintainer welcome these changes or if they prefer to keep this as a separate fork.
Documentation
I haven't gotten to the documentation but it would be great if the original maintainer could expand a bit on it. I'm happy to write the document but if they could share notes about the protocol that would be very helpful.
I have expanded significantly the
README
file to provide more guidelines.Logger
I've added a small
Logger()
class to print logs in a nicer way, it's really just cosmetic but it looks nicer on terminal.