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

Non-root android support #1150

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AlexandreRouma
Copy link

@AlexandreRouma AlexandreRouma commented Sep 2, 2022

This PR contains the following modifications:

  1. Fixed the cmake to allow compilation with the Android NDK
  2. Disabled libusb enumeration when compiling on android since it is not supported (As explained in the libusb documentation line 92)
  3. Add the hackrf_open_by_fd function that allows to open a hackrf device using a file descriptor. This is the only possible way to open a usb device on a non-rooted android device.

This code has been tested and confirmed working on my serveral android device but I still recommend testing it further.
It should not impact any other operating system.
image

@martinling
Copy link
Member

martinling commented Sep 3, 2022

Hi @AlexandreRouma, thanks for the PR!

The changes look straightforward, but as you may have seen there's a build failure on the CI - this is on Appveyor, which runs a Windows build. Since Windows doesn't provide an fd for USB devices I suggest we just wrap hackrf_open_by_fd with #ifndef _WIN32.

My other thought is about the use of LIBUSB_OPTION_NO_DEVICE_DISCOVERY due to discovery not currently working on Android. There's a libusb branch aiming to fix that problem which has been getting a lot of traction: libusb/libusb#874, later updated at libusb/libusb#1164. It would be good to avoid making changes in libhackrf that would prevent discovery from working if the libusb issue is fixed. Can we make this conditional somehow?

Finally, what is the Android app in your screenshot? Could you point us to some instructions for building this with libhackrf so that we can test it?

@AlexandreRouma
Copy link
Author

Hi,

I'll add the windows ifdef in a sec, didn't realize that libusb completely removes the function instead of simply returning LIBUSB_ERROR_NOSUPPORTED like basically all other functions. I should bring this up to the libusb team for more consistency.

The branches that aim to fix the enumeration support do exists but I have very high doubts that they will ever work. Still, it can indeed be made conditional.

The app in the screenshot is an android build of my SDR++ project. It is currently very hard to build for android and still a bit buggy (not the fault of libhackrf). So I don't think it's an ideal way to test libhackrf on android.

@AlexandreRouma
Copy link
Author

not exactly sure why the CI's complaining about code style when it perfectly matches the rest of the file

@straithe
Copy link
Member

straithe commented Sep 3, 2022

Did you start your work on this PR from the latest version in the repository?

@AlexandreRouma
Copy link
Author

yes, it was pulled yesterday a few hours before the PR

@straithe
Copy link
Member

straithe commented Sep 3, 2022

Thank you for the update. I'll check this again next week when I'm back in the office (if no one else gets to you before then).

@martinling
Copy link
Member

We have a clang-format config in the repository which is checked on the CI. If you have clang-format installed, you can run ./tools/reformat-source.sh to make the code match.

@martinling
Copy link
Member

Do you have any recommended process for building and testing libhackrf on android? I've done a little work with the NDK and Android dev tools in the past but it was some years ago.

@AlexandreRouma
Copy link
Author

Building for the NDK is quite involved because the dependencies must be compiles as well, so, so I'd suggest following the scripts I wrote for my android-sdr-kit project that builds all of these libraries for android (or just run the container).

Then, to test, you need to write an NDK app and link as usual. All you have to do is, in the java part, enumerate devices and locate the hackrf using its vid/pid combo, then request permission, and when you've got permission, send the file descriptor android gives you. After using the new hackrf_open_by_fd function, you can use everything as normal.

@AlexandreRouma
Copy link
Author

Hi,
Is there any update on this?

@mossmann
Copy link
Member

Hi, Is there any update on this?

We've been very busy preparing for a release for Opera Cake support which will be out very soon. We will give this a proper review after the release and hope to incorporate it into the next release. Thank you for your patience!

Copy link
Member

@miek miek left a comment

Choose a reason for hiding this comment

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

This generally looks good to me, thanks!

I haven't tested on Android, but I tried it out on Linux with a quick patch to hackrf_transfer:

$ git diff
diff --git a/host/hackrf-tools/src/hackrf_transfer.c b/host/hackrf-tools/src/hackrf_transfer.c
index a2ad9ccc..d6261295 100644
--- a/host/hackrf-tools/src/hackrf_transfer.c
+++ b/host/hackrf-tools/src/hackrf_transfer.c
@@ -1123,7 +1123,8 @@ int main(int argc, char** argv)
                return EXIT_FAILURE;
        }
 
-       result = hackrf_open_by_serial(serial_number, &device);
+       int fd = open(serial_number, 'r');
+       result = hackrf_open_by_fd(fd, &device);
        if (result != HACKRF_SUCCESS) {
                fprintf(stderr,
                        "hackrf_open() failed: %s (%d)\n",

$ hackrf-tools/src/hackrf_transfer -r /dev/null -d /dev/bus/usb/003/010
call hackrf_set_sample_rate(10000000 Hz/10.000 MHz)
call hackrf_set_hw_sync_mode(0)
call hackrf_set_freq(900000000 Hz/900.000 MHz)
Stop with Ctrl-C
^CCaught signal 2
17.3 MiB / 0.875 sec = 19.8 MiB/second, average power -32.5 dBfs

Exiting...
Total time: 0.87522 s
hackrf_stop_rx() done
hackrf_close() done
hackrf_exit() done
fclose() done
exit

I've added some comments for minor changes inline.

Also, the bugfix commits should be squashed down into a set of descriptive commits and any clang-format issues should be fixed before merging. I've done that here if you want to use that: https://github.com/miek/hackrf/commits/android

@@ -754,6 +759,36 @@ int ADDCALL hackrf_open_by_serial(
return hackrf_open_setup(usb_device, device);
}

int ADDCALL hackrf_open_by_fd(
int fd,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should take intptr_t sys_dev here instead and pass it straight through to libusb. Looking back at at the PR for the libusb function (libusb/libusb#242 (comment)), they made that choice to make it more general across different platforms so I think we should do the same and leave it up to the user to do any platform-specific conversion/checking.

host/libhackrf/CMakeLists.txt Outdated Show resolved Hide resolved
return HACKRF_ERROR_INVALID_PARAM;
}

int err = libusb_wrap_sys_device(g_libusb_context, (intptr_t)fd, &usb_device);
Copy link
Member

Choose a reason for hiding this comment

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

This function was added fairly recently (in libusb 1.0.23) so I think we should add an ifdef on LIBUSB_API_VERSION and return unsupported otherwise, to allow for building with older versions.

@straithe
Copy link
Member

straithe commented Dec 6, 2022

@AlexandreRouma I'm checking in to see if you've seen the requested changes. Please let me know!

@AlexandreRouma
Copy link
Author

Hi,

Sorry, had some stuff come up IRL, didn't have as much time for coding and completely forgot about the PR.
Will try to implement this weekend if I get the time.

@straithe
Copy link
Member

straithe commented Dec 7, 2022

Hope you are doing ok! I'll watch out for an update.

@CRD716
Copy link

CRD716 commented Feb 29, 2024

@AlexandreRouma Any plans to complete this?

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.

6 participants