Replace USB-CCID (smartcard) by USB-HID #4203
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
macOS Installed |
| @@ -52,7 +52,7 @@ | |||
| #define USE_DEVICE_LEDGER 1 | |||
| #endif | |||
|
|
|||
| #if !defined(HAVE_PCSC) | |||
| #if !defined(HAVE_HIDAPI) | |||
| #undef USE_DEVICE_LEDGER | |||
| #define USE_DEVICE_LEDGER 0 | |||
| #endif | |||
There was a problem hiding this comment.
This seems to set USE_DEVICE_LEDGER to 0 or 1, but the code above uses #ifdef, not #if
|
Windows The log looks the same when the smartcard device is disabled in Device Manager. |
|
macOS Wallet creation, refresh works. Transfer is buggy. You have to time the accept fee press perfectly (not too fast and not too slow), or else it will display |
|
Windows Looks the same with 80c1fe9. |
|
Please move the lowlevel USB code out of the main device_ledger. Maybe you could provide the API (or similar) from #4267 in C? Basically your code provides all functionality. If you could encapsulate it and conditionally link the C code or not, we would both be happy :) Also, I have no issue removing all references to monerujo. (I can't use libusbhid in Android) |
|
@m2049r why does libhidapi not work on android? There even seems to be a makefile for it authored by Trezor developer Stick: https://github.com/signal11/hidapi/blob/master/android/jni/Android.mk . |
|
@TheCharlatan thanks for the pointer - it looks like it may work on android (although i doubt it since permissions don't work well for ndk). but why add an extra unnecessary dependency? imo, there is nothing in the code of this PR which cannot be handled by android usb support. and there is nothing wrong with encapsulating lowlevel functions like hardware communication even if we didn't care about android, ios, whatnot.. also, if ledger communication goes through the android usb stack provided by monerujo JNI, i can show the status of communications (as i can intercept it) - otherwise i cannot. and the user sits there wondering why nothing is happening for minutes... |
|
@m2049r I'm looking at your request. It seems legitimate. Also I agree with the lake of feedback on what happen. I has some draft code to give info about tx processing. I hope a new PR on this subject beginning of September. |
|
@m2049r do the last commit answer to your request? |
|
Are you planning on having this for 0.13 (that is, soon :)) ? |
|
Would be great, yes.
Basically the code is ok, but I have trouble with Windows. I will post a
minimal standalone code for asking help on debug.
Le 1 sept. 2018 6:21 PM, "moneromooo-monero" <notifications@github.com> a
écrit :
Are you planning on having this for 0.13 (that is, soon :)) ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4203 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFOX8-s_QIardYm_wi8tFyn_AT4U24Dnks5uWrP7gaJpZM4Vp_SS>
.
|
cslashm
force-pushed
the
NanoS-USBHID
branch
from
2197e31 to
5164798
Compare
m2049r
suggested changes
Sep 3, 2018
| BYTE buffer_recv[BUFFER_RECV_SIZE]; | ||
| unsigned int id; | ||
| //IO | ||
| hw::io::device_io_hid hw_device; |
|
|
||
| /* -------------------------------------------------------------- */ | ||
| device_ledger::device_ledger() { | ||
| device_ledger::device_ledger(): hw_device(0x0101, 0x05, 64, 10000) { |
There was a problem hiding this comment.
please construct hw_device explicitly so some else (me) can instantiate their own implementation of hw::io::device_io (with #ifdefs)
There was a problem hiding this comment.
do you suggest to declare:
hw::io::device_io *hw_device;
and do a:
hw_device = new hw::io::device_io_hid((0x0101, 0x05, 64, 10000);
or do you suggest the contructor take the pointer as parameter?
how to you plan to use that? I'm not sure it solve your pb
It is not clear for me what I zm suppose to mod :)
There was a problem hiding this comment.
sorry, i was over-thinking it - leave it like it is.
There was a problem hiding this comment.
hmm ...
Ok So I will
- leave as is for now,
- make some cleanup,
- squash all
- remove the "donot merge"
:)
| #include <PCSC/winscard.h> | ||
| #include <PCSC/wintypes.h> | ||
| #endif | ||
| #include "device_io_hid.hpp" |
There was a problem hiding this comment.
you just need device_io.hpp here
cslashm
force-pushed
the
NanoS-USBHID
branch
from
5164798 to
dd6888c
Compare
|
I built current master (fad88e1) with b932d0d and dd6888c cherry-picked on top. Generating the wallet on Windows works fine. I haven't tried making transactions. On Ubuntu 16.04, I get a segfault when trying to open an existing Ledger wallet or when creating a new one. Sorry, I don't have a core. |
cslashm
force-pushed
the
NanoS-USBHID
branch
from
dd6888c to
d1e0929
Compare
cslashm
changed the title
cslashm
force-pushed
the
NanoS-USBHID
branch
from
d1e0929 to
c926461
Compare
|
I tested c926461 on Windows 10 and on Ubuntu 16.04. On Windows I tested On two Linux boxes, both Ubuntu 16.04, I get segfaults when opening an existing wallet or creating a new one. /Edit: libhidapi-* version in Ubuntu 16.04 is |
cslashm
force-pushed
the
NanoS-USBHID
branch
from
c926461 to
93c4755
Compare
|
I think the segfault came from debug/trace message. I fixed it (I hope). |
|
The good news is it doesn't segfault any more. The bad news is Connecting to my Nano S works fine with v0.12.3.0-release (PCSC). |
|
PEBKAC After looking at the ldd output again, I realised the binaries link to libudev. Adding an udev rule solved my "Unable to open device" problem on Ubuntu. That's probably explained somewhere in Ledger documentation, but not in the commit or PR message. Creating a wallet now works fine for me on Linux and Windows. |
|
@iDunk5400 you need different udev rules than before? that doesn't sound right. what rule did you have to add? |
|
There was no "before" :) I didn't use my Nano S for anything other than Monero on my Ubuntu box, and Ledger used libpcsclite (smartcard mode) prior to this PR. |
|
udev rules are reported here: https://support.ledgerwallet.com/hc/en-us/articles/115005165269-Fix-connection-issues I should add this pointer in the user doc. @iDunk5400 does it mean there is no more trouble now ? \o/ |
Remove PCSC dependencies which is a bit hard (not user friendly) to install on linux and Mac Split Ledger logic and device IO
cslashm
force-pushed
the
NanoS-USBHID
branch
from
93c4755 to
bb6e3bb
Compare
fluffypony
added a commit
that referenced
this pull request
Sep 24, 2018
bb6e3bb Replace USB-CCID (smartcard) by USB-HID (cslashm)
|
what's with all the whitespace (lots of lines end with a random amount of blanks)? |
I think it would make sense if an rpc/public function is available, that returns the status of different devices. This would be useful for a whole bunch of applications, not just monerujo. From what I have read so far and the research we made at our company, it should be quite easy to make use of hidapi in an android application. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove PCSC dependencies which is a bit hard (not user friendly) to install on Linux and Mac
This code seems to work under Linux (more test incoming)
It needs to be tested under Mac and Windows
In order to compile it requires the following packages:
Linux:
Windows (msys2/64):
Mac: (edited)