split polling can_read and reading from USB #4419
Conversation
|
TychoVrahe
force-pushed
the
tychovrahe/refactor/usb_split
branch
from
86f3140 to
e942f8e
Compare
|
Tangential: do we still use HID anywhere in Core? Maybe it is time to remove it (in different PR)? |
FIDO runs over HID |
| @@ -127,10 +127,25 @@ STATIC mp_obj_t mod_trezorio_WebUSB_write(mp_obj_t self, mp_obj_t msg) { | |||
| STATIC MP_DEFINE_CONST_FUN_OBJ_2(mod_trezorio_WebUSB_write_obj, | |||
| mod_trezorio_WebUSB_write); | |||
|
|
|||
| /// def read(self, buf: bytes) -> int: | |||
There was a problem hiding this comment.
we want the following signature:
| /// def read(self, buf: bytes) -> int: | |
| /// def read(self, buf: bytes, offset: int = 0, limit: int | None = None) -> int: |
TychoVrahe
requested review from
prusnak and
matejcik
and removed request for
prusnak
| } | ||
|
|
||
| ssize_t r = | ||
| usb_hid_read(o->info.iface_num, &((uint8_t *)buf.buf)[offset], limit); |
There was a problem hiding this comment.
we also need to check that offset and limit are valid for the buf
(and ideally raise a ValueError they exceed)
dtto webusb
There was a problem hiding this comment.
solved as part of 7d9a53c, see below
| msg_len = await read | ||
| report = bytearray(msg_len) | ||
| read_len = iface.read(report, 0, msg_len) | ||
| if read_len != msg_len: |
There was a problem hiding this comment.
these checks are kind of annoying... could we get rid of them somehow?
on usb we don't really need to know msg_len because it is fixed, so i'm guessing this is for BT with variable length packets?
perhaps read() should guarantee that it reads full limit (or buffer size) and exception out otherwise?
or, can we guarantee that if we got msg_len out of await read, the next read must return that many bytes? (i mean we could always do the full read into a buffer like we do on unix, i suppose...)
There was a problem hiding this comment.
how about this: 7d9a53c
- the guarantee that msg_len is equal to real read data is implemented
- exception if buffer space is insufficient
- exception if for any reason the underlying USB implementation reads message of different length, but modification of the buffer is allowed under such circumstances
- limit is removed, assumed that you always read the whole message of the msg_len size. (or do we ever want to allow reading partials, even if it means losing the rest of the data?)
This PR changes the way reading from USB works: the polling mechanism is only used to detect if there is a new message, the reading itself is done separately.
The emulator implementation is modified a bit, since there is a need to do the PING-PONG and it is undesirable to emit a CAN_READ event due to this- therefore the message is read into buffer during polling, and the read from the buffer upon read.
This should enable some optimized usage of the message buffer, but this is not done in this PR.