Support external USB devices#1520
Merged
Merged
Conversation
Member
|
Looks sensible at first blush, will need more sleep to properly review ;). (Also, the uevent path is probably device-specific to some extent, but given how limited OTG support actually is OOB, it's probably fine for now ;)). |
NiLuJe
reviewed
Sep 21, 2022
Contributor
Author
|
Thanks NiLuJe. What's your opinion on the constants names? I aimed to match the current style. Another variant I thought of would be more descriptive but also more verbose: |
Member
That's indeed potentially less confusing, and verbosity's never bothered me ;o). |
Member
|
Random related thought, since I've never dealt with OTG, well, anywhere: does https://github.com/koreader/KoboUSBMS understand what's happening if you try to start an USBMS session w/ OTG enabled? |
Member
|
Same. |
Contributor
Author
|
When the OTG is enabled, USBMS fails after connecting to a computer with "The device is plugged into a plain power source, not a USB host!". I fixed that in the koreader PR - it restores the default USB role of "device" on exit now. |
Member
Good idea ;). I hope USBMS still fails when it's just a keyboard plugged in, though :D. |
Contributor
Author
|
Yeah, by the time USBMS is started, the USB role would be device. So, if the reader in OTG mode is capable of supplying power, the power would be off now and it would be as if nothing is connected. USBMS would wait for 30s and exit on timeout. Likely, only some readers with Android have powered OTG. If OTG does not supply power, a keyboard must be connected through a Y-shaped cable along with the power source. USBMS would detect the power source and fail with "The device is plugged into a plain power source, not a USB host!". |
NiLuJe
requested changes
Oct 14, 2022
NiLuJe
left a comment
Member
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lykahb)
input/input.c line 172 at r3 (raw file):
lua_settop(L, 0); // Pass the fd to Lua, we might need it for FFI ioctl shenanigans lua_pushinteger(L, inputfds[fd_idx]);
I'd fold the increment in there (i.e., inputfds[fd_idx++]).
input/input.c line 181 at r3 (raw file):
} static int closeInputDevice(size_t fd_idx_to_close)
ssize_t
input/input.c line 185 at r3 (raw file):
// Right now, we close everything, but, in the future, we may want to keep *some* slots open. // Note that doing that would (currently) require making sure those slots are at the start of the array, // in ascending fd number order, and that the array itself isn't sparse.
Duplicated comment ;).
Code quote:
// Right now, we close everything, but, in the future, we may want to keep *some* slots open.
// Note that doing that would (currently) require making sure those slots are at the start of the array,
// in ascending fd number order, and that the array itself isn't sparse.input/input.c line 200 at r3 (raw file):
inputfds[fd_idx - 1] = -1; fd_idx--;
In a similar vein, I'd fold the decrement in there. inputfds[--fd_idx] = -1;
Code quote:
inputfds[fd_idx - 1] = -1;
fd_idx--;input/input.c line 203 at r3 (raw file):
computeNfds(); printf("[ko-input] Closed input device with idx=%d, fd=%d\n", fd_idx_to_close, fd);
The format token for ssize_t is %zd ;).
Member
|
(Minor nits, but looks sounds!) |
Member
|
For archeological purposes, output of uevent feed(Excuse the chatty gaming keyboard, I was too lazy to remove the PS/2 adapter on my other, tamer spare keyboard ;p. Keycap LEDs work, though, which is probably another nice way of ruining battery life ;p). |
Member
|
Unplug (w/o a manual role swap): uevent feedThe role toggle stays put after unplug, FWIW. |
Member
|
Removal by role swap (LEDs stay on, which isn't the case when plugging in @ uevent feed |
Contributor
Author
|
Thanks for testing. The keyboard logic does nothing with LED's now. It is actually quite difficult to manipulate them. So, the difference in how their state changes must come from the keyboard firmware. Likely, if plugging it simply into a power bank would have the same effect as plugging into a device that has usb device mode. Yes, the role toggle does not change after unplugging. Otherwise, every time before plugging in the keyboard a user would have to enable OTG again. |
Should hopefully trigger less noise, because there's a lot of usb shenanigans going on ;). Also, it might just pickup BT keyboards if we ever handle those one day.
NiLuJe
approved these changes
Oct 28, 2022
NiLuJe
left a comment
Member
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @lykahb)
Member
|
20c88ad seems like a cleaner approach to me, but I haven't actually tested anything yet ;). Thoughts? |
Contributor
Author
|
Yeah, a check on subsystem is cleaner. I can check if it still works on Kobo Libra 2. |
In the fake event generator child, close any and all nonessential fds.
NiLuJe
approved these changes
Oct 28, 2022
NiLuJe
left a comment
Member
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status:
We don't do anything with it reight now, but it should ultimately allow us not to have to walk the entirely of /sys/class/input to find what was just plugged in ;). a.k.a, poor man's udev ;p.
Almost everything goes through with a PLATFORMSOC_DEVPATH prefix, whereas input subsystems events are few and far between ;).
NiLuJe
approved these changes
Oct 29, 2022
NiLuJe
left a comment
Member
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status:
NiLuJe
added a commit
to lykahb/koreader
that referenced
this pull request
Oct 29, 2022
NiLuJe
added a commit
to NiLuJe/koreader
that referenced
this pull request
Oct 29, 2022
NiLuJe
added a commit
to NiLuJe/koreader
that referenced
this pull request
Oct 29, 2022
NiLuJe
added a commit
to koreader/koreader
that referenced
this pull request
Oct 29, 2022
0xstillb
pushed a commit
to 0xstillb/koreader-thai
that referenced
this pull request
May 9, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is required for koreader/koreader#9540.
With this change a reader can have a USB role of either host or device. The existing event
CODE_FAKE_USB_PLUG_IN/OUThas been renamed toCODE_FAKE_USB_HOST_PLUG_IN/OUTto indicate that reader is plugged to a host. A new eventCODE_FAKE_USB_DEVICE_PLUG_IN/OUTis emitted when a device is connected to reader (which is in the host mode).I added the function to close a single device to input.c
closeInputDevice. I also extracted computation of nfds into a function to avoid duplicating the logic once more in thecloseInputDevice. Perhaps it makes sense to export it to lua, but it wouldn't have usage yet.The size of
inputfdsis increased from 4 to 8 to accommodate the external devices. On kobo there are already three default inputs. Connecting an external keyboard may bring in several inputs with keyboard capabilities.This change is