search

openvizsla / ov_ftdi

FT2232H-based USB sniffer
345 stars 102 forks source link

SDRAM data trapping #13

Open matwey opened 6 years ago

matwey commented 6 years ago

I am working on pure-C implementation for OpenVizsla host software: https://github.com/matwey/libopenvizsla I've faced the following issue with FPGA firmware last summer. I have been not able to make FPGA reliable restart sniffed data transmission.

The issue itself is the following. The protocol has two encapsulation levels. The upper level is packets come from SDRAM buffering module. The packets consists of 0x0D magic header, length byte, and the data. On practice this packets are of the same length. The nested data is a stream consisted of packets from captured data. They are consisted of 0xA0 magic header, length and USB data. This packets are not aligned with each other. One 0x0D-packet may consisted many 0xA0-packets, and 0xA0-packet may be split between two consecutive 0x0D-packets.

When I stop capturing and streaming and start it again then the first data byte of the first 0x0D-packet is not 0xA0 which it should be. This is an issue because there is no other reliable way to sync 0xA0-packet stream. We cannot just scan for first 0xA0 because 0xA0 byte may be consisted inside data itself (compare with SLIP protocol).

My stop sequence is the following:

My start sequence is the following (given I assured that the stream is stopped):

I've tried to add Reset for sdram_fifo in SDRAM_Sink and SDRAM_Host_Read to reset fifoes on SDRAM_SINK_GO/SDRAM_HOST_READ_GO switch but this didn't help.

The issue is still present in the latest firmware from new_migen branch.

tmbinc commented 6 years ago

First, remember how the SDRAM buffering works: There's the SDRAM_SINK which sinks 8-bit data from the cstream (aka "Whacker"), and writes it into SDRAM.

Then there's the SRAM_Host_Read, which reads data from SDRAM, packetizes it to host_burst_length (=16 * 2bytes), and sends those over the "CmdProc", aka. the thing that muxes together the different data sources for the return (FTDI) channel.

SDRAM_Sink and SDRAM_Host_Read know each other's read/write pointers so that we have flow control.

cstream (whacker's producer) receives data from ULPI (timestamp'ed d/is_start/is_end/is_err/isovr) and formats it into the A0

(which SDRAM_Sink writes into memory). That means that once these packets arrive in SDRAM, there's no packet boundary anymore; SDRAM_Host_Read doesn't care what the data is, it only knows about the write pointer and will stream everything to that point. Now, note that SDRAM_Sink will write partial packets, but at a 2 byte granularity. However they will be collected on the input side, so no data should be lost if you toggle GO, _assuming_ you still read out any data. sdram_host_sink on the other hand only sends full-sized packets to the host (as you observed). The current rptr resets if you toggle go, and at that point you almost certainly lost data. So I'd think (but I haven't verified) that the correct way to pause transfers would be the opposite order: 1. Disable streaming via CSTREAM_CFG. This halts receiving incoming ULPI data, but should write out the rest of the current A0... packet. 2. Disable writing data to SDRAM via SDRAM_SINK_GO. Once you read all data, you can do this: 3. Disable streaming data from SDRAM to HOST. However note that there's currently no "flush" action - the last packet will most likely not fully align to the 32 byte burst size so you'll miss the last packet. Additionally one byte could be stuck in the SDRAM_SINK that you can't un-stuck. (Perhaps it would be wise to make the packets always even-length size...) I'm not fully clear on your motivation to stop streaming, but if it's just to ignore USB data for a while, to avoid the issue with the last packet, I'd propose that you instead keep the SDRAM data streaming enabled, and only pause using CSTREAM (i.e. only clear CSTREAM_CFG[0], nothing else). You may still get a partial CSTREAM packet, but you would get the rest of the packet when you re-enable it (and you could then drop it). For starting, I would also advise to turn the data flow on first (SDRAM_HOST_Read+SDRAM_Sink, then CSTREAM) because otherwise you may get an overflow in CSTREAM if you're not fast enough in this sequence. In summary, I think the following additions would be "nice to have" : 1. CSTREAM should only produce even-length packets. 2. CSTREAM should indicate when it's done writing the last packet. 2a. CSTREAM could write a special pattern that indicates it was asked to stop. 3. SDRAM_Sink should indicate when a single byte is stuck when ~go (though this "shouldn't happen") 4. SDRAM_Host_Read should send a partial packet when go is deasserted, and not wait for the full burst. 5. SDRAM_Host_Read should indicate when it is done sending all data. 6. We should have a global reset (as opposed to "go"); the intention would be that deasserting "go" doesn't lose data (unless there's a fifo overrun), but asserting "reset" would. Felix On 8/5/2018 12:16 PM, Matwey V. Kornilov wrote: > I am working on pure-C implementation for OpenVizsla host software: > https://github.com/matwey/libopenvizsla > I've faced the following issue with FPGA firmware last summer. > I have been not able to make FPGA reliable restart sniffed data > transmission. > > The issue itself is the following. > The protocol has two encapsulation levels. > The upper level is packets come from SDRAM buffering module. > The packets consists of |0x0D| magic header, |length| byte, and the |data|. > On practice this packets are of the same length. > The nested data is a stream consisted of packets from captured data. > They are consisted of |0xA0| magic header, |length| and USB |data|. > This packets are not aligned with each other. > One |0x0D|-packet may consisted many |0xA0|-packets, and |0xA0|-packet > may be split between two consecutive |0x0D|-packets. > > When I stop capturing and streaming and start it again then the first > data byte of the first |0x0D|-packet is not |0xA0| which it should be. > This is an issue because there is no other reliable way to sync > |0xA0|-packet stream. We cannot just scan for first |0xA0| because > |0xA0| byte may be consisted inside |data| itself (compare with SLIP > protocol). > > My stop sequence is the following: > > * > > write |0| to |SDRAM_HOST_READ_GO| (|0xC28|) > > * > > write |0| to |SDRAM_SINK_GO| (|0xE11|) > > * > > write |0| to |CSTREAM_CFG| (|0x800|) > > My start sequence is the following (given I assured that the stream is > stopped): > > * > > write 32-bit |0| to |SDRAM_SINK_RING_BASE| (|0xE09|) > > * > > write 32-bit |0x01000000| to |SDRAM_SINK_RING_END| (|0xE0D|) > > * > > write 32-bit |0| to |SDRAM_HOST_READ_RING_BASE| (|0xC1C|) > > * > > write 32-bit |0x01000000| to |SDRAM_HOST_READ_RING_END| (|0xC20|) > > * > > write |0| to |SDRAM_SINK_PTR_READ| (|0xE00|) > > * > > write |1| to |CSTREAM_CFG| (|0x800|) > > * > > write |1| to |SDRAM_SINK_GO| (|0xE11|) > > * > > write |1| to |SDRAM_HOST_READ_GO| (|0xC28|) > > I've tried to add |Reset| for |sdram_fifo| in |SDRAM_Sink| and > |SDRAM_Host_Read| to reset fifoes on > |SDRAM_SINK_GO|/|SDRAM_HOST_READ_GO| switch but this didn't help. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > , or mute the thread > . >
matwey commented 6 years ago

Hello,

The motivation is simple. Any application can crash unexpectedly, so we cannot guarantee graceful exit. So the most reliable on-start behavior for us is to force stop streaming, and reinit all the things as they expected to be (full reset), and the start streaming from the scratch.

I've followed your advice and reordered starting procedure, but there is still no success. Even CSTREAM and SINK are OFF (and SDRAM pointers are reset to 0), then the unexpected data are transmitted to host after I trigger SDRAM_HOST_READ.

Could you please also review the following: https://github.com/matwey/ov_ftdi/commit/0ffe58f453fe70e08786f64027f241f3a6c0c96c Does it makes sense?

matwey commented 6 years ago

The most reliable setup is the following:

        // dev.regs.SDRAM_SINK_GO.wr(1)
        // dev.regs.SDRAM_HOST_READ_GO.wr(1)
        // dev.regs.CSTREAM_CFG.wr(1)

Using https://github.com/matwey/ov_ftdi/commit/0ffe58f453fe70e08786f64027f241f3a6c0c96c

matwey commented 6 years ago

I still see that there is extra byte between d0 and a0:

        0x0000:  3260 d01f 27a0 1000 0000 7df6 69a0 0000
        0x0010:  0300 3d43 7169 8498 a000 0001 00a5 4971
        0x0020:  5aa0 0000 0300 6596 7869 8498 a000 0001
        0x0030:  00cd 9c78 5aa0 0000 0300 8de9 7f69 8498
        0x0040:  a000 0001

Here 1f is size byte, and then a0 should follows in theory.

matwey commented 6 years ago

Now I see that self.fifo_fsm from SDRAM_sink needs to be force reset to READ_LOW at every sink restart. Otherwise, it may cache the last odd byte from previous run.

somehdlguy commented 6 years ago

Patches to fix this kind of thing are welcome! (I haven't been able to find my board to do anything that requires HW-in-the-loop testing).