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

[testing needed] Android: implement usb-serial-for-android driver support #2647

Closed
wants to merge 11 commits into from
Closed

[testing needed] Android: implement usb-serial-for-android driver support #2647

wants to merge 11 commits into from

Conversation

charno
Copy link
Contributor

@charno charno commented Mar 6, 2020

This pull request allows to use a virtual "usb-serial-for-android" device to download dives from divecomputers with different USB-to-Serial chipsets.

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Code cleanup
  • Build system change
  • Documentation change
  • Language translation

Pull request long description:

This pull request allows to use a virtual "usb-serial-for-android" device to download dives from divecomputers with different USB-to-Serial chipsets. With this changes, FTDI-Based divecomputers also work again.

Currently I did only test this driver with a Mares Puck Pro. Please test this with other computers before merging!

The apk of this changes can be found at https://github.com/Subsurface-divelog/subsurface/suites/503855254/artifacts/2603870

Tested:

Changes made:

  1. added usb-serial-for-android dependency in build.gradle
  2. Added AndroidSerial.java as java abstraction layer
  3. added custom divecomputer serial_android_usb.c which uses JNI to call AndroidSerial methods
  4. support usb-serial-for-android as connection type for divecomputers
  5. Add more PID/VID pairs to devicefilter and support these also in usb-serial-for-android

Related issues:

Possibly Fixes #2108

Release note:

Mobile-android: Add usb-serial-for-android driver support

Documentation change:

Add reference to usb-serial-for-android driver support.

Mentions:

@glance-

@derek derek bot added the new-contributor label Mar 6, 2020
@derek
Copy link

derek bot commented Mar 6, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@derek derek bot added the no-dco label Mar 6, 2020
Signed-off-by: Christof Arnosti <charno@charno.ch>
Signed-off-by: Christof Arnosti <charno@charno.ch>
Signed-off-by: Christof Arnosti <charno@charno.ch>
Signed-off-by: Christof Arnosti <charno@charno.ch>
Signed-off-by: Christof Arnosti <charno@charno.ch>
Signed-off-by: Christof Arnosti <charno@charno.ch>
Signed-off-by: Christof Arnosti <charno@charno.ch>
@derek derek bot removed the no-dco label Mar 6, 2020
@charno charno marked this pull request as ready for review March 6, 2020 15:01
Copy link
Collaborator

@dirkhh dirkhh left a comment

Choose a reason for hiding this comment

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

First: awesome. So excited to see this
Generally I prefer more descriptive commit messages. That makes it much easier to go through the history and figure out what is what.
There are a number of whitespace issues (yeah, I know, everyone's a critic and it's the easiest thing to pick on - but after 8 years of trying to bring consistency my eyes are used to certain patterns... I'm happy to simply clean this up as part of manually merging your code)
I added a couple of questions and suggestions below.
But overall this looks very promising

scripts/android/before_install.sh Show resolved Hide resolved
scripts/android/travisbuild.sh Show resolved Hide resolved
private static final int DC_STATUS_TIMEOUT = -7;
private static final int DC_STATUS_PROTOCOL = -8;
private static final int DC_STATUS_DATAFORMAT = -9;
private static final int DC_STATUS_CANCELLED = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

that is of course rather fragile... would it be possible to auto-generate the code snippet here from libdivecomputer sources? Certainly not required to merge this, but might be a way to ensure that things don't break if Jef changes a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, maybe that's possible, but I wouldn't know how...

core/libdivecomputer.c Outdated Show resolved Hide resolved
subsurface-mobile-main.cpp Outdated Show resolved Hide resolved
subsurface-mobile-main.cpp Show resolved Hide resolved
Signed-off-by: Christof Arnosti <charno@charno.ch>
…roid" -> "usb-serial"

Signed-off-by: Christof Arnosti <charno@charno.ch>
@derek derek bot added the no-dco label Mar 7, 2020
@derek
Copy link

derek bot commented Mar 7, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

Signed-off-by: Christof Arnosti <charno@charno.ch>
Signed-off-by: Christof Arnosti <charno@charno.ch>
@derek derek bot removed the no-dco label Mar 7, 2020
@dirkhh
Copy link
Collaborator

dirkhh commented Mar 7, 2020

tested the Mares Icon HD. Works flawlessly (and crazy fast... wow)
I did the merge locally and will push this in a few minutes.
Feel free to look at how I edited the commits and some of the commit messages... I tried to keep a light touch, yet make it consistent with our best practices. And to combine commits where you fixed up earlier changes.

Excellent work @charno
Thank you so much.
I hope to have a Google Play beta based on this code relatively soon...

@dirkhh dirkhh mentioned this pull request Mar 7, 2020
12 tasks
@dirkhh
Copy link
Collaborator

dirkhh commented Mar 7, 2020

merged via #2650

@dirkhh dirkhh closed this Mar 7, 2020
@charno
Copy link
Contributor Author

charno commented Mar 7, 2020 via email

@dirkhh
Copy link
Collaborator

dirkhh commented Mar 7, 2020

I just got the notification on one of my older tablets and tested both the Mares and the OSTC 3 - and both worked perfectly. So yes, the beta is available in the Play Store.

I need to fix the code that tries to populate the download screen to auto-select usb-serial instead of FTDI. I'll try to get this done later today and push another beta.
Additionally, I want to merge an unrelated branch with some significant improvements to the dive list handling which will fix the issue where sometimes after a download the dive list is messed up... once that is merged we can have a much stronger new release of Subsurface-mobile 3.0 :-)

And for the record: the pleasure is all mine. This is a major contribution to an open source project. Something you should be really proud of.

@dirkhh
Copy link
Collaborator

dirkhh commented Mar 7, 2020

Oh, I should mention... the earlier tests were done with two newer Android phones running Android 10 and using a USB C - to - A connector.
The latest test was on a Nexus 10 running Android 5.1.1 using a USB-to-go cable... so we have covered pretty much the two extremes here :-)

@charno
Copy link
Contributor Author

charno commented Mar 7, 2020

Sounds good to me. Believe me, I am proud! :-)

One small thing I noticed while testing (and if you do some work in this area anyways, or I could also open a new Issue?) is this: When I sync from the computer, I get status messages on the right side of the buttons. If I abort and retry ("Wiederholen") the sync, the status does not update anymore.

@dirkhh
Copy link
Collaborator

dirkhh commented Mar 7, 2020

please file and issue, ideally with screen shots :-)

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

Successfully merging this pull request may close these issues.

Can't download dives from DC to mobile with FTDI
2 participants