wjasper / Linux_Drivers

Open source Linux device drivers
GNU General Public License v3.0
110 stars 64 forks source link

More problems with the USB-CTR scan #35

Closed MarkRivers closed 4 years ago

MarkRivers commented 4 years ago

This relates to the previous issue #30.

I have found 2 problems with usbScanStart_USB_CTR().

The first is the logic in this line:

} else if (scanData->mode & USB_CTR_CONTINUOUS_READOUT) {
    packet_size = (((wMaxPacketSize/(scanData->lastElement+1))*(scanData->lastElement+1)) / 2)

This is not correct, and fails under some circumstances. Consider the case of reading 3 32-bit counters, so scanData->lastElement+1 is 6. Each reading requires 12 bytes, so the maximum number of readings that can fit in a 512 byte buffer is 512/12 = 42 (using integer arithmetic). This means that the packet_size should be 42*12 = 504 bytes or 252 words.

However the above logic produces: 512/6 6 / 2 = 85 6 /2 = 255 words. This is incorrect since 255 words is 510 bytes which does not hold an integer number of readings. The correct logic is this:

  } else if (scanData->mode & USB_CTR_CONTINUOUS_READOUT) {
    int bytesPerReading = 2*(scanData->lastElement+1);
    packet_size = ((wMaxPacketSize / bytesPerReading) * bytesPerReading) / 2;

The second problem is more serious, and calls into question the whole way we have been approaching this. If USB_CTR_CONTINUOUS_READOUT is true we set the packet_size to an integer number of readings. I have discovered that this really does not work, because of how the firmware handles partial buffers.

The problem arises with final read when the specified value of scanData->count has been reached. It will generally be the case that the final buffer to be read will be smaller than the packet_size, because the final buffer will not contain, for example, 42 readings using the settings above. I have discovered that if the requested packet size is 512 bytes then the USBCTR sends a final partial buffer when the scan is complete. It might only contain 80 bytes, for example. However, if the packet size is anything other than 512 the USBCTR never sends that final partial buffer.

This is a show-stopper, because it will never return the last few readings in a scan. The problem never occurs if (scanData->lastElement+1) is a power of 2, because the logic in usbScanStart_USB_CTR() always sets the packet size to 512 bytes. This is true of the old bad logic and my fixed logic above. However, if (scanData->lastElement+1) is not a power of 2 then the problem occurs.

I have worked around this problem as follows:

This works fine. It is what I was trying to avoid initially, but I now think there is no alternative. I suspect this is what MCC is doing in the UL.

MarkRivers commented 4 years ago

There is also an issue in usbScanRead_USB_CTR.

  if (transferred != nbytes) {
    fprintf(stderr, "usbScanRead_USB_CTR: number of bytes transferred = %d, nbytes = %d\n", transferred, nbytes);
    return transferred;
  }

This is printing an error message if transferred != nbytes. However that is a normal occurrence on the last read in a scan if the number of bytes remaining is less than 512. I removed the fprintf.

wjasper commented 4 years ago

I fixed the calculation of packet_size. Thanks for the explanation. The second part of your comment only arises if packet_size is set incorrectly, which is why I was hesitant to allow the user to do so. If you specify a count, the function blocks until the entire count is returned. If you specify continuous mode, it returns in multiples of scanSize or just one Scan for slow scans. Continuous scan in MCC speak is setting the value of count to infinity. Since infinity is not a legal integer value, it is understood by the firmware to be the value 0. Those are your only options at the firmware level. When I do a bulk transfer, I need to tell the USB subsystem how many bytes to expect. If the number I requested is not equal to the number of bytes transferred, it usually indicates a problem. Not checking for this, or ignoring it I feel is dangerous. If things are set up correctly, you should not see this message. If you see this message, it means things are not set up correctly.

MarkRivers commented 4 years ago

The second part of your comment only arises if packet_size is set incorrectly, which is why I was hesitant to allow the user to do so. If you specify a count, the function blocks until the entire count is returned. If you specify continuous mode, it returns in multiples of scanSize or just one Scan for slow scans. Continuous scan in MCC speak is setting the value of count to infinity. Since infinity is not a legal integer value, it is understood by the firmware to be the value 0. Those are your only options at the firmware level. When I do a bulk transfer, I need to tell the USB subsystem how many bytes to expect. If the number I requested is not equal to the number of bytes transferred, it usually indicates a problem. Not checking for this, or ignoring it I feel is dangerous. If things are set up correctly, you should not see this message. If you see this message, it means things are not set up correctly.

I have to disagree. Here is a use case that does not work with the scheme you describe, but that does work perfectly with the code I am using in my higher level application.

The driver will set the packet size to 512/12 * 12 = 504 bytes = 252 words, Each packet will contain 42 readings.

This works fine until the very end of the scan.

The total number of bytes of be collected is 1000 points * 12 bytes/point = 12000 bytes. The number of packets is thus 12000/504 = 23.809. You can see this is not an integer number of packets, so the final packet sent needs to be less than 504 bytes. However, I have found empirically that the USBCTR will never send that final packet if the packet size is not 512 bytes or 256 words. It sends 23 packets of size 504, and then times out never sending that final shorter packet.

However, if I force the packet size to be 256 words then it works fine. Each packet does not contain an integer number of counter readings, so I need to use a ring buffer to pull them out in integer numbers. If the packet size is 256 words then the USBCTR does send a final short packet.

Consider another simple case:

In this case the packet size computed by the driver is 512/16*16 = 512 bytes = 256 words. Each packet will contain 32 readings. However, if I set the count to 1000 and continuous readout the total number of packets to be sent is 31.25. So again the final packet needs to be shorter than the other packets. It turn out that the USBCTR handles this fine, it simply sends a smaller packet for the final packet. But your driver will print an error message in this case when it should not.

If you would like I will write a version of test-usb-ctr.c that demonstrates the problem and that my solution works.

If you have another solution I'd be happy to hear it!

MarkRivers commented 4 years ago

Hi Warren, I have added a new option "I" to test-usb-ctr.c. I have attached it renamed to .txt on the end so Github would accept it. test-usb-ctr.c.txt

It uses the USB_CTR_CONTINUOUS_READOUT option with a non-zero value of scanData.count, unlike the "C" test. It illustrates exactly the problems I described above.

This is the output I see if I set numCounters=4 and count=100.

Testing scan input with continuous readout
Connect Timer 1 to Counter 1
scanData: lastElement=15, packet_size=256, options=0x0, mode=0x1
numBytesRead=512, numSamples=16
Scan: 0     0   0   0       0 0 0 0
Scan: 1     1000   100   0       0x3e8 0x3e8 0x3e8 0x3e8
Scan: 2     2000   200   0       0x7d0 0x7d0 0x7d0 0x7d0
Scan: 3     3000   300   0       0xbb8 0xbb8 0xbb8 0xbb8
Scan: 4     4000   400   0       0xfa0 0xfa0 0xfa0 0xfa0
Scan: 5     5000   500   0       0x1388 0x1388 0x1388 0x1388
Scan: 6     6000   600   0       0x1770 0x1770 0x1770 0x1770
Scan: 7     7000   700   0       0x1b58 0x1b58 0x1b58 0x1b58
Scan: 8     8000   800   0       0x1f40 0x1f40 0x1f40 0x1f40
Scan: 9     9000   900   0       0x2328 0x2328 0x2328 0x2328
Scan: 10     10000   1000   0       0x2710 0x2710 0x2710 0x2710
Scan: 11     11000   1100   0       0x2af8 0x2af8 0x2af8 0x2af8
Scan: 12     12000   1200   0       0x2ee0 0x2ee0 0x2ee0 0x2ee0
Scan: 13     13000   1300   0       0x32c8 0x32c8 0x32c8 0x32c8
Scan: 14     14000   1400   0       0x36b0 0x36b0 0x36b0 0x36b0
Scan: 15     15000   1500   0       0x3a98 0x3a98 0x3a98 0x3a98
numBytesRead=512, numSamples=16
Scan: 16     16000   1600   0       0x3e80 0x3e80 0x3e80 0x3e80
Scan: 17     17000   1700   0       0x4268 0x4268 0x4268 0x4268
Scan: 18     18000   1800   0       0x4650 0x4650 0x4650 0x4650
Scan: 19     19000   1900   0       0x4a38 0x4a38 0x4a38 0x4a38
Scan: 20     20000   2000   0       0x4e20 0x4e20 0x4e20 0x4e20
Scan: 21     21000   2100   0       0x5208 0x5208 0x5208 0x5208
Scan: 22     22000   2200   0       0x55f0 0x55f0 0x55f0 0x55f0
Scan: 23     23000   2300   0       0x59d8 0x59d8 0x59d8 0x59d8
Scan: 24     24000   2400   0       0x5dc0 0x5dc0 0x5dc0 0x5dc0
Scan: 25     25000   2500   0       0x61a8 0x61a8 0x61a8 0x61a8
Scan: 26     26000   2600   0       0x6590 0x6590 0x6590 0x6590
Scan: 27     27000   2700   0       0x6978 0x6978 0x6978 0x6978
Scan: 28     28000   2800   0       0x6d60 0x6d60 0x6d60 0x6d60
Scan: 29     29000   2900   0       0x7148 0x7148 0x7148 0x7148
Scan: 30     30000   3000   0       0x7530 0x7530 0x7530 0x7530
Scan: 31     31000   3100   0       0x7918 0x7918 0x7918 0x7918
numBytesRead=512, numSamples=16
Scan: 32     32000   3200   0       0x7d00 0x7d00 0x7d00 0x7d00
Scan: 33     33000   3300   0       0x80e8 0x80e8 0x80e8 0x80e8
Scan: 34     34000   3400   0       0x84d0 0x84d0 0x84d0 0x84d0
Scan: 35     35000   3500   0       0x88b8 0x88b8 0x88b8 0x88b8
Scan: 36     36000   3600   0       0x8ca0 0x8ca0 0x8ca0 0x8ca0
Scan: 37     37000   3700   0       0x9088 0x9088 0x9088 0x9088
Scan: 38     38000   3800   0       0x9470 0x9470 0x9470 0x9470
Scan: 39     39000   3900   0       0x9858 0x9858 0x9858 0x9858
Scan: 40     40000   4000   0       0x9c40 0x9c40 0x9c40 0x9c40
Scan: 41     41000   4100   0       0xa028 0xa028 0xa028 0xa028
Scan: 42     42000   4200   0       0xa410 0xa410 0xa410 0xa410
Scan: 43     43000   4300   0       0xa7f8 0xa7f8 0xa7f8 0xa7f8
Scan: 44     44000   4400   0       0xabe0 0xabe0 0xabe0 0xabe0
Scan: 45     45000   4500   0       0xafc8 0xafc8 0xafc8 0xafc8
Scan: 46     46000   4600   0       0xb3b0 0xb3b0 0xb3b0 0xb3b0
Scan: 47     47000   4700   0       0xb798 0xb798 0xb798 0xb798
numBytesRead=512, numSamples=16
Scan: 48     48000   4800   0       0xbb80 0xbb80 0xbb80 0xbb80
Scan: 49     49000   4900   0       0xbf68 0xbf68 0xbf68 0xbf68
Scan: 50     50000   5000   0       0xc350 0xc350 0xc350 0xc350
Scan: 51     51000   5100   0       0xc738 0xc738 0xc738 0xc738
Scan: 52     52000   5200   0       0xcb20 0xcb20 0xcb20 0xcb20
Scan: 53     53000   5300   0       0xcf08 0xcf08 0xcf08 0xcf08
Scan: 54     54000   5400   0       0xd2f0 0xd2f0 0xd2f0 0xd2f0
Scan: 55     55000   5500   0       0xd6d8 0xd6d8 0xd6d8 0xd6d8
Scan: 56     56000   5600   0       0xdac0 0xdac0 0xdac0 0xdac0
Scan: 57     57000   5700   0       0xdea8 0xdea8 0xdea8 0xdea8
Scan: 58     58000   5800   0       0xe290 0xe290 0xe290 0xe290
Scan: 59     59000   5900   0       0xe678 0xe678 0xe678 0xe678
Scan: 60     60000   6000   0       0xea60 0xea60 0xea60 0xea60
Scan: 61     61000   6100   0       0xee48 0xee48 0xee48 0xee48
Scan: 62     62000   6200   0       0xf230 0xf230 0xf230 0xf230
Scan: 63     63000   6300   0       0xf618 0xf618 0xf618 0xf618
numBytesRead=512, numSamples=16
Scan: 64     64000   6400   0       0xfa00 0xfa00 0xfa00 0xfa00
Scan: 65     65000   6500   0       0xfde8 0xfde8 0xfde8 0xfde8
Scan: 66     66000   6600   0       0x1d0 0x1d0 0x1d0 0x1d0
Scan: 67     67000   6700   0       0x5b8 0x5b8 0x5b8 0x5b8
Scan: 68     68000   6800   0       0x9a0 0x9a0 0x9a0 0x9a0
Scan: 69     69000   6900   0       0xd88 0xd88 0xd88 0xd88
Scan: 70     70000   7000   0       0x1170 0x1170 0x1170 0x1170
Scan: 71     71000   7100   0       0x1558 0x1558 0x1558 0x1558
Scan: 72     72000   7200   0       0x1940 0x1940 0x1940 0x1940
Scan: 73     73000   7300   0       0x1d28 0x1d28 0x1d28 0x1d28
Scan: 74     74000   7400   0       0x2110 0x2110 0x2110 0x2110
Scan: 75     75000   7500   0       0x24f8 0x24f8 0x24f8 0x24f8
Scan: 76     76000   7600   0       0x28e0 0x28e0 0x28e0 0x28e0
Scan: 77     77000   7700   0       0x2cc8 0x2cc8 0x2cc8 0x2cc8
Scan: 78     78000   7800   0       0x30b0 0x30b0 0x30b0 0x30b0
Scan: 79     79000   7900   0       0x3498 0x3498 0x3498 0x3498
numBytesRead=512, numSamples=16
Scan: 80     80000   8000   0       0x3880 0x3880 0x3880 0x3880
Scan: 81     81000   8100   0       0x3c68 0x3c68 0x3c68 0x3c68
Scan: 82     82000   8200   0       0x4050 0x4050 0x4050 0x4050
Scan: 83     83000   8300   0       0x4438 0x4438 0x4438 0x4438
Scan: 84     84000   8400   0       0x4820 0x4820 0x4820 0x4820
Scan: 85     85000   8500   0       0x4c08 0x4c08 0x4c08 0x4c08
Scan: 86     86000   8600   0       0x4ff0 0x4ff0 0x4ff0 0x4ff0
Scan: 87     87000   8700   0       0x53d8 0x53d8 0x53d8 0x53d8
Scan: 88     88000   8800   0       0x57c0 0x57c0 0x57c0 0x57c0
Scan: 89     89000   8900   0       0x5ba8 0x5ba8 0x5ba8 0x5ba8
Scan: 90     90000   9000   0       0x5f90 0x5f90 0x5f90 0x5f90
Scan: 91     91000   9100   0       0x6378 0x6378 0x6378 0x6378
Scan: 92     92000   9200   0       0x6760 0x6760 0x6760 0x6760
Scan: 93     93000   9300   0       0x6b48 0x6b48 0x6b48 0x6b48
Scan: 94     94000   9400   0       0x6f30 0x6f30 0x6f30 0x6f30
Scan: 95     95000   9500   0       0x7318 0x7318 0x7318 0x7318
usbScanRead_USB_CTR: number of bytes transferred = 128, nbytes = 512
numBytesRead=128, numSamples=4
Scan: 96     96000   9600   0       0x7700 0x7700 0x7700 0x7700
Scan: 97     97000   9700   0       0x7ae8 0x7ae8 0x7ae8 0x7ae8
Scan: 98     98000   9800   0       0x7ed0 0x7ed0 0x7ed0 0x7ed0
Scan: 99     99000   9900   0       0x82b8 0x82b8 0x82b8 0x82b8

It works fine. However, notice that usbScanRead_USB_CTR printed the error message because the last packet is 128 bytes rather than 512 bytes. That is NOT an error condition, it is expected because the last packet is necessarily smaller than 512 bytes because count=100 is not a multiple of 16 samples per buffer.

This is the output with numCounters=3 and count=100.

Connect Timer 1 to Counter 1
scanData: lastElement=11, packet_size=252, options=0x0, mode=0x1
numBytesRead=504, numSamples=21
Scan: 0     0   0       0 0 0 0
Scan: 1     1000   100       0x3e8 0x3e8 0x3e8 0x3e8
Scan: 2     2000   200       0x7d0 0x7d0 0x7d0 0x7d0
Scan: 3     3000   300       0xbb8 0xbb8 0xbb8 0xbb8
Scan: 4     4000   400       0xfa0 0xfa0 0xfa0 0xfa0
Scan: 5     5000   500       0x1388 0x1388 0x1388 0x1388
Scan: 6     6000   600       0x1770 0x1770 0x1770 0x1770
Scan: 7     7000   700       0x1b58 0x1b58 0x1b58 0x1b58
Scan: 8     8000   800       0x1f40 0x1f40 0x1f40 0x1f40
Scan: 9     9000   900       0x2328 0x2328 0x2328 0x2328
Scan: 10     10000   1000       0x2710 0x2710 0x2710 0x2710
Scan: 11     11000   1100       0x2af8 0x2af8 0x2af8 0x2af8
Scan: 12     12000   1200       0x2ee0 0x2ee0 0x2ee0 0x2ee0
Scan: 13     13000   1300       0x32c8 0x32c8 0x32c8 0x32c8
Scan: 14     14000   1400       0x36b0 0x36b0 0x36b0 0x36b0
Scan: 15     15000   1500       0x3a98 0x3a98 0x3a98 0x3a98
Scan: 16     16000   1600       0x3e80 0x3e80 0x3e80 0x3e80
Scan: 17     17000   1700       0x4268 0x4268 0x4268 0x4268
Scan: 18     18000   1800       0x4650 0x4650 0x4650 0x4650
Scan: 19     19000   1900       0x4a38 0x4a38 0x4a38 0x4a38
Scan: 20     20000   2000       0x4e20 0x4e20 0x4e20 0x4e20
numBytesRead=504, numSamples=21
Scan: 21     21000   2100       0x5208 0x5208 0x5208 0x5208
Scan: 22     22000   2200       0x55f0 0x55f0 0x55f0 0x55f0
Scan: 23     23000   2300       0x59d8 0x59d8 0x59d8 0x59d8
Scan: 24     24000   2400       0x5dc0 0x5dc0 0x5dc0 0x5dc0
Scan: 25     25000   2500       0x61a8 0x61a8 0x61a8 0x61a8
Scan: 26     26000   2600       0x6590 0x6590 0x6590 0x6590
Scan: 27     27000   2700       0x6978 0x6978 0x6978 0x6978
Scan: 28     28000   2800       0x6d60 0x6d60 0x6d60 0x6d60
Scan: 29     29000   2900       0x7148 0x7148 0x7148 0x7148
Scan: 30     30000   3000       0x7530 0x7530 0x7530 0x7530
Scan: 31     31000   3100       0x7918 0x7918 0x7918 0x7918
Scan: 32     32000   3200       0x7d00 0x7d00 0x7d00 0x7d00
Scan: 33     33000   3300       0x80e8 0x80e8 0x80e8 0x80e8
Scan: 34     34000   3400       0x84d0 0x84d0 0x84d0 0x84d0
Scan: 35     35000   3500       0x88b8 0x88b8 0x88b8 0x88b8
Scan: 36     36000   3600       0x8ca0 0x8ca0 0x8ca0 0x8ca0
Scan: 37     37000   3700       0x9088 0x9088 0x9088 0x9088
Scan: 38     38000   3800       0x9470 0x9470 0x9470 0x9470
Scan: 39     39000   3900       0x9858 0x9858 0x9858 0x9858
Scan: 40     40000   4000       0x9c40 0x9c40 0x9c40 0x9c40
Scan: 41     41000   4100       0xa028 0xa028 0xa028 0xa028
numBytesRead=504, numSamples=21
Scan: 42     42000   4200       0xa410 0xa410 0xa410 0xa410
Scan: 43     43000   4300       0xa7f8 0xa7f8 0xa7f8 0xa7f8
Scan: 44     44000   4400       0xabe0 0xabe0 0xabe0 0xabe0
Scan: 45     45000   4500       0xafc8 0xafc8 0xafc8 0xafc8
Scan: 46     46000   4600       0xb3b0 0xb3b0 0xb3b0 0xb3b0
Scan: 47     47000   4700       0xb798 0xb798 0xb798 0xb798
Scan: 48     48000   4800       0xbb80 0xbb80 0xbb80 0xbb80
Scan: 49     49000   4900       0xbf68 0xbf68 0xbf68 0xbf68
Scan: 50     50000   5000       0xc350 0xc350 0xc350 0xc350
Scan: 51     51000   5100       0xc738 0xc738 0xc738 0xc738
Scan: 52     52000   5200       0xcb20 0xcb20 0xcb20 0xcb20
Scan: 53     53000   5300       0xcf08 0xcf08 0xcf08 0xcf08
Scan: 54     54000   5400       0xd2f0 0xd2f0 0xd2f0 0xd2f0
Scan: 55     55000   5500       0xd6d8 0xd6d8 0xd6d8 0xd6d8
Scan: 56     56000   5600       0xdac0 0xdac0 0xdac0 0xdac0
Scan: 57     57000   5700       0xdea8 0xdea8 0xdea8 0xdea8
Scan: 58     58000   5800       0xe290 0xe290 0xe290 0xe290
Scan: 59     59000   5900       0xe678 0xe678 0xe678 0xe678
Scan: 60     60000   6000       0xea60 0xea60 0xea60 0xea60
Scan: 61     61000   6100       0xee48 0xee48 0xee48 0xee48
Scan: 62     62000   6200       0xf230 0xf230 0xf230 0xf230
numBytesRead=504, numSamples=21
Scan: 63     63000   6300       0xf618 0xf618 0xf618 0xf618
Scan: 64     64000   6400       0xfa00 0xfa00 0xfa00 0xfa00
Scan: 65     65000   6500       0xfde8 0xfde8 0xfde8 0xfde8
Scan: 66     66000   6600       0x1d0 0x1d0 0x1d0 0x1d0
Scan: 67     67000   6700       0x5b8 0x5b8 0x5b8 0x5b8
Scan: 68     68000   6800       0x9a0 0x9a0 0x9a0 0x9a0
Scan: 69     69000   6900       0xd88 0xd88 0xd88 0xd88
Scan: 70     70000   7000       0x1170 0x1170 0x1170 0x1170
Scan: 71     71000   7100       0x1558 0x1558 0x1558 0x1558
Scan: 72     72000   7200       0x1940 0x1940 0x1940 0x1940
Scan: 73     73000   7300       0x1d28 0x1d28 0x1d28 0x1d28
Scan: 74     74000   7400       0x2110 0x2110 0x2110 0x2110
Scan: 75     75000   7500       0x24f8 0x24f8 0x24f8 0x24f8
Scan: 76     76000   7600       0x28e0 0x28e0 0x28e0 0x28e0
Scan: 77     77000   7700       0x2cc8 0x2cc8 0x2cc8 0x2cc8
Scan: 78     78000   7800       0x30b0 0x30b0 0x30b0 0x30b0
Scan: 79     79000   7900       0x3498 0x3498 0x3498 0x3498
Scan: 80     80000   8000       0x3880 0x3880 0x3880 0x3880
Scan: 81     81000   8100       0x3c68 0x3c68 0x3c68 0x3c68
Scan: 82     82000   8200       0x4050 0x4050 0x4050 0x4050
Scan: 83     83000   8300       0x4438 0x4438 0x4438 0x4438
usbScanRead_USB_CTR: number of bytes transferred = 0, nbytes = 504
Error, usbScanRead_USB_CTR returned 0 bytes

It fails because the USBCTR is not sending the final shorter buffer. In this case samples per buffer is 21. Once it has sent 84 samples the final buffer should contain 16 samples. But the USBCTR never sends that buffer, so the read times out and we never receive all 100 samples.

As far as I can tell the only solution to this is either: