capsules: usb/cdc: Fixup the CDC control message logic #2082
Conversation
|
cc @gendx , just to confirm that always accepting the request won't be a problem for OpenSK. |
|
Why the change in the logic? I don't think my approach was particularly robust, but I do remember getting multiple of the various control messages. |
|
My Linux (5.7) kernel sends a value of 3, so with the current logic the CDC device doesn't work. |
|
On my mac with tockloader, I get two CONTROL_LINE_STATE messages, 0x02 and then 0x00. With miniterm I get one, just 0x03. So with this change tockloader doesn't work for me. Maybe we should use this change, but remove the disconnect case in the if statement? |
I didn't test the CDC capsule, but it shouldn't be a problem when that capsule is disabled. I didn't check the latest developments, but I don't think the current Tock implementation allows to have multiple USB capsules running at the same time anyway. |
|
I have removed the disconnect logic. |
|
My proposed change doesn't work either :(. I was hoping to avoid changing tockloader, but it seems like I will have to. There doesn't seem to be any way for the host to signal over CDC-ACM "ok, you may now send serial data to me". The serial library in python has this pesky line in it: https://github.com/pyserial/pyserial/blob/master/serial/serialposix.py#L352 which seems to be the issue for tockloader. Basically, anything received before all configuration is finished is just dropped. And since each serial tool has its own setup process, the device can't really know when it is safe to send. So, my updated approach is:
|
|
I couldn't push to this PR so I put my changes here: https://github.com/tock/tock/compare/pr/2082 |
|
And note, since miniterm uses pyserial, running |
Update the comment to use SET_CONTROL_LINE_STATE to match the pharse in the spec. As well as that accept the request no matter what the value is, but let's document what the options are. Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Instead of connecting and disconnecting with SET_CONTROL_LINE_STATE let's only ever start a connection. Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
alistair23
force-pushed
the
alistair/cdc-fixup
branch
from
429eabd
to
868326b
Compare
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
|
I have added your patches @bradjc and converted the values to enums. |
| } | ||
|
|
||
| #[derive(PartialEq)] | ||
| enum CDCCntrlMessage { |
There was a problem hiding this comment.
Nit pick: "ctrl" is a much more common abbreviation for "control". Otherwise, is saving two letters worth it over fully spelling "control"?
| @@ -211,6 +243,7 @@ impl<'a, U: hil::usb::UsbController<'a>> CdcAcm<'a, U> { | |||
| Buffer64::default(), | |||
| ], | |||
| state: Cell::new(State::Disabled), | |||
| ctrl_state: Cell::new(CtrlState::Idle), | |||
|
bors r+ |
Pull Request Overview
Update the comment to use SET_CONTROL_LINE_STATE to match the phrase in
the spec. As well as that accept the request no matter what the value
is, but let's document what the options are.
Testing Strategy
Test USB CDC on OpenTitan.
TODO or Help Wanted
N/A
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.