Ice40usbtrace support

[asdfuser]

As discussed in #157 this PR replaces the cynthion support with support for the ice40-usbtrace. I renamed/moved/reused some structs that were used in the cynthion backend, but are useful/needed for the ice40-usbtrace as well.

• InterfaceSelection
• CynthionUsability -> DeviceUsability
• CynthionStop -> BackendStop
• handle_thread_panic
• Speed

Speed is used in ui.rs and other backends, but the repr(u8) and mask() methods are specific to the cynthion so that's not ideal right now.

It probably needs to be split up to keep the details in the (cynthion) backend.

[martinling]

Thank you! I've made a couple of minor suggestions below.

I'll work on the necessary abstractions to get this integrated, as discussed in #157. It looks like that should mostly be quite straightforward as the interface is similar.

The main parts I'm concerned about are the cases where parse_packet currently calls println! or assert! with some error. These each need some other kind of handling.

From reading the code this is my current understanding of those errors:

1. PID code zero (aka TsOverflow), OK flag zero: There was a packet on the wire with a bad PID, but it's not possible to get the bytes of that packet.
2. PID code Data0 | Data1, OK flag zero: There was a data packet on the wire with bad CRC16, but we do have its bytes.
3. PID code Sof | Setup | In | Out, OK flag zero: There was a token packet on the wire with bad CRC5. We have its bytes except for the bad CRC5 value. We can invent our own bad CRC5 to use as a dummy with the same effect.
4. PID code Ack | Nak | Stall, OK flag zero: This shouldn't happen.
5. An unrecognised PID code in the header. This shouldn't happen.

Thoughts/questions:

• In case 1, is it true that we can't get those bytes? It seems like it should be possible for the device to return a header with pid: 0, ok: 0, dat: N, along with the N bytes of the bad packet - does it, or could it be made to do that? We don't currently have a way to represent "there was a packet here, but we don't have its bytes".
• In case 2, can you confirm if the bytes captured include the bad CRC16 value?
• The invented bad CRC in case 3 is a reasonable solution for now, but it could be misleading to someone trying to debug their implementation. I'll have to think about how we can warn about this. Also: what happens in the case of a token packet with trailing bytes?
• Can case 4 happen as a result of an ACK/NAK/STALL packet with trailing bytes after the PID? And if not, then what does the ice40-usbtrace report for that case?
• I think case 5 should result in stopping the capture and displaying an error to the user, because it indicates a failure of the capture device. Unless there's some reason that this can happen in normal operation and should be ignored?

[smunaut]

Hi Martin,

I designed the ice40usbtrace hardware so let me answer some of the questions.

1. PID code zero (aka TsOverflow), OK flag zero: There was a packet on the wire with a bad PID, but it's not possible to get the bytes of that packet.

Yes, basically we saw a valid SYNC but then nothing after that made any sense, so it's just reporting that.
The core in the hardware that interprets incoming data is originally written for a normal "usb device" core and meant to be tiny (to fit in ice40) so as soon as the packet doesn't make sense, it "aborts", that's why some of the error case are the way they are.

2. PID code Data0 | Data1, OK flag zero: There was a data packet on the wire with bad CRC16, but we do have its bytes.

Yes. And the CRC16 is included in the data bytes reported to the host.
Technically this can also get triggered by a lower level error ( like invalid bit stuffing which will abort processing of the data packet )

3. PID code Sof | Setup | In | Out, OK flag zero: There was a token packet on the wire with bad CRC5. We have its bytes except for the bad CRC5 value. We can invent our own bad CRC5 to use as a dummy with the same effect.

As you guessed the OK flag zero can also be triggered by extraneous data ( i.e. we didn't get EOP when we expected it ).
Any extraneous data isn't reported to the host.

4. PID code Ack | Nak | Stall, OK flag zero: This shouldn't happen.

This can happen.
As you theorized, if we get a valid sync and PID byte for handshake packet but then we don't get EOP right after, then this is what happens. The hardware doesn't send the extraneous data.

5. An unrecognised PID code in the header. This shouldn't happen.

As you stated I think this should probably be handled by stopping the capture and resetting everything because something went very wrong.

[martinling]

Thanks @smunaut, that all makes sense.

My concern is with how we map each of these cases into:

• what is stored in Packetry's capture database
• what is shown in the Packetry UI
• what is saved in the Pcap and PcapNG file formats

And I see three approaches to that:

1. Drop all invalid packets, and warn/document that this backend will do so. This is by far the simplest approach, and is the easiest to communicate to users. In the vast majority of USB analysis scenarios, the low level protocol is working correctly so this is not an issue. And in the case where users are debugging low-level failures, they're really going to need a different capture device to do that effectively anyway.

2. Invent some bytes in order to map each case into the existing data model. Specifically, this could be done by:

   • For case 1, store a packet with a single zero byte. Packetry will identify this as an invalid packet, which is the right outcome even if what is shown is not exactly what was seen on the wire.
   • For case 2, store the packet as normal as per the current PR.
   • For case 3, store the packet with the inverse of the correct CRC, as per the current PR.
   • For case 4, store a two-byte packet, with the PID followed by a zero byte. Packetry will interpret this as having a correct PID but excess bytes, which is the right outcome.

3. Modify the data model so that we can accurately represent exactly what is and isn't known about the packet in each of these cases.

I don't really see a good solution for option 3, when you factor in the need to persist this information to capture files.

I would be fine with either option 1 or option 2, but I think it should be one or the other. Right now the PR code is doing a mix of both, and I think that's a recipe for confusion.

[smunaut]

I think option 2 is the best one.

I'd avoid option 1 simply because it's very useful to know that something has happened and that you may need to resort to another tool to know what's going on. ( There is actually an alternate bitstream that can run on the same hw that turns it into a 2 bit 96 Msps logic analyzer and feeds it to sigrok and it's usb-fs decoder for very low lever analysis ... )

And I agree that option 3 has a bunch of complications and is just not worth the effor.

Something that might be nice though is if a backend could provide inside the UI some "help text" / "details" about its quirks and its way of reporting things.

[martinling]

Yeah, option 2 seems like the best way forward to me too.

It has the nice property that although it does invent some bytes, it doesn't add any ambiguity or discard any information - I believe you could actually map backwards in each of those cases to exactly what header/bytes were sent by the ice40-usbtrace.

So I'll go ahead with integrating this using that approach.

[asdfuser]

Yeah, option 2 seems like the best way forward to me too.

It has the nice property that although it does invent some bytes, it doesn't add any ambiguity or discard any information - I believe you could actually map backwards in each of those cases to exactly what header/bytes were sent by the ice40-usbtrace.

So I'll go ahead with integrating this using that approach.

Just to make sure we're on the same page - it sounds like you'll take it from here? If there's anything else I can do feel free to say so.

[martinling]

Yes, I'll take it from here. Thank you!

[martinling]

Closing this as we now have an integrated version in #193.