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

Support for Bluetooth on all platforms #33

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

RamonBeast
Copy link
Contributor

@RamonBeast RamonBeast commented May 9, 2024

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 with bleak.

  • I've added a new script find-radiacode.py to help users find their Radiacode via Bluetooth and show its MAC Address or UUID (depending on the platform).
  • For an example of synchronous use, check basic.py in the examples folder
  • For an example of asynchronous use, check webserver.py in the examples folder

Note: I couldn't make the library work (not the original nor my fork) on WSL. 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)

Radiacode-windows-bt

Apple M1 and Intel (both Bluetooth and USB)

Radiacode-M1-USB Radiacode-M1_BT

Linux (Raspberry PI and Ubuntu, both Bluetooth and USB)

Radiacode_Raspberry_USB Radiacode_Raspberry_BT

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.

@RamonBeast
Copy link
Contributor Author

I refactored the main Radiacode() class, I think I've found a solution that's not bad to allow the use of the library both in a synchronous and asynchronous way. With this last change I think we have gained quite some flexibility, I'll try to list everything here:

  • All examples seem to work correctly (I've only had time to test on Mac with both USB and BT)
  • For synchronous use, there is no need to change the code of existing apps, they should work out of the box (but we have new params in the constructor of the Radiacode() class that are useful)
  • Asynchronous apps are now fully supported as well and very easy to implement

Synchronous usage (unchanged):

rc = RadiaCode()
serial = rc.serial_number()

Asynchronous usage:

rc = await RadiaCode.async_init()
serial = await rc.async_serial_number()

In short, to use the library with async the main object must be created by calling RadiaCode.async_() (it supports the same exact parameters of the sync RadiaCode()) and all functions names are the same with a prefix async_ so spectrum() becomes async_spectrum() and so on. Of course those have to be awaited.

This should remove the main concern I initially had that was to adjust existing apps to support asyncs. 🎉

Copy link
Owner

@cdump cdump left a 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:
Copy link
Owner

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>
Copy link
Owner

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:
Copy link
Owner

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:
   ...

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.

None yet

2 participants