Fixes to prefixes and size logic in usbScanStart_USB_CTR

MarkRivers commented 5 years ago

I am making a new pull request. It fixes the prefixes to be unique to this driver. The existing prefixes cannot be used in applications that need to include cbw.h, which mine does. Your new SINGLEIO definition collides with theirs, as do many others. As I said in a previous message there are also internal collisions in your driver header files such as

usb-1024LS.h:#define DIO_PORTB (0x04)
usb-1208FS.h:#define DIO_PORTB (0x01)
usb-1208LS.h:#define DIO_PORTB     (0x04)
usb-1408FS.h:#define DIO_PORTB (0x01)
usb-dio24.h:#define DIO_PORTB (0x04)
usb-dio32HS.h:#define DIO_PORTB  (0x01)
usb-erb.h:#define DIO_PORTB     (0x1)

which mean that I cannot include usb-1208FS.h an usb-1208LS.h in the same application. You have made all of your function names unique by adding a suffix like _USB1208LS. You need to do the same thing with all of the #define definitions.

I believe there is a logic problem on a size in usbScanStart_USB_CTR where a factor of 2 needs to be removed and I have fixed that as well.

However, there are problems with this new API, it does not work for me, but I will discuss that in the issue.

MarkRivers commented 5 years ago

I have made some changes that I think really improve the new API. Here are the changes to usb-ctr.h:

corvette:~/measComp_Linux_Drivers/USB/mcc-libusb>git diff -w -b 9de545727b822e8f4e7be76dfdb62b1ff75e84f0 usb-ctr.h
diff --git a/USB/mcc-libusb/usb-ctr.h b/USB/mcc-libusb/usb-ctr.h
index ba71ac0..6beec4a 100644
--- a/USB/mcc-libusb/usb-ctr.h
+++ b/USB/mcc-libusb/usb-ctr.h
@@ -77,8 +77,9 @@ typedef struct timerParams_t {
   uint32_t delay;
 } TimerParams;

-#define CONTINUOUS_SCAN (0x1)
-#define SINGLEIO        (0x2)
+#define USB_CTR_CONTINUOUS_READOUT (0x1)
+#define USB_CTR_SINGLEIO           (0x2)
+#define USB_CTR_FORCE_PACKET_SIZE  (0x4)

 typedef struct scanData_t {
   uint8_t scanList[33];   // the channel configuration
@@ -95,10 +96,11 @@ typedef struct scanData_t {
                                bit 6:   1 = retrigger mode,  0 = normal trigger
                                bit 7:   Reserved */
   uint8_t mode;            /* mode bits:
-                               bit 0:   0 = counting mode,  1 = CONTINUOUS_SCAN
-                               bit 1:   packet_size = 0xff, 1 = SINGLEIO
+                               bit 0:   0 = counting mode,  1 = CONTINUOUS_READOUT
+                               bit 1:   1 = SINGLEIO
+                               bit 2:   1 = use packet size passed scanData->packet_size
                           */
-  uint8_t packet_size;     // number of samples to return - 1 from FIFO
+  uint16_t packet_size;    // number of samples to return from FIFO
 } ScanData;

This is a description of these changes:

Renamed CONTINUOUS_SCAN to CONTINUOUS_READOUT. This is more accurate.
Added a new scanData.mode bit, FORCE_PACKET_SIZE. If this is 1 then the scanData.packet_size that the user passes in is used.
scanData.packet_size is the number of words to transfer, not the number of words-1. The -1 is a detail of the firmware implementation and should be hidden from the user. It needs to be a uint16_t because it can now hold the value 256.

These are the changes to usb-ctr.c.

corvette:~/measComp_Linux_Drivers/USB/mcc-libusb>git diff 9de545727b822e8f4e7be76dfdb62b1ff75e84f0 usb-ctr.c
diff --git a/USB/mcc-libusb/usb-ctr.c b/USB/mcc-libusb/usb-ctr.c
index 15ff769..56e2c58 100644
--- a/USB/mcc-libusb/usb-ctr.c
+++ b/USB/mcc-libusb/usb-ctr.c
@@ -649,7 +649,7 @@ void usbScanStart_USB_CTR(libusb_device_handle *udev, ScanData *scanData)
   uint8_t requesttype = (HOST_TO_DEVICE | VENDOR_TYPE | DEVICE_RECIPIENT);
   uint8_t data[14];
   uint32_t pacer_period;    //  pacer timer period value (0 for external clock)
-  uint8_t packet_size;      //  number of samples - 1 to transfer at a time.
+  uint16_t packet_size;     //  number of samples to transfer at a time.

   int ret;

@@ -659,24 +659,25 @@ void usbScanStart_USB_CTR(libusb_device_handle *udev, ScanData *scanData)
     pacer_period = rint((96.E6/scanData->frequency) - 1);
   }

-  if ( scanData->frequency < 10 || (scanData->mode & SINGLEIO) ) {
-    packet_size = (scanData->lastElement+1)*2 - 1;
-  } else if (scanData->mode & CONTINUOUS_SCAN) {
-    packet_size = (((wMaxPacketSize/(scanData->lastElement+1))*(scanData->lastElement+1)) / 2) - 1;
+  if (scanData->mode & USB_CTR_FORCE_PACKET_SIZE) {
+    packet_size = scanData->packet_size;
+  } else if (scanData->mode & USB_CTR_SINGLEIO) {
+    packet_size = scanData->lastElement+1;
+  } else if (scanData->mode & USB_CTR_CONTINUOUS_READOUT) {
+    packet_size = (((wMaxPacketSize/(scanData->lastElement+1))*(scanData->lastElement+1)) / 2);
   } else {
-    packet_size = wMaxPacketSize/2 - 1;
+    packet_size = wMaxPacketSize/2;
   }
   scanData->packet_size = packet_size;

-  if (scanData->mode & CONTINUOUS_SCAN || scanData->count == 0) {
-    scanData->count = 0;
-    scanData->mode |= CONTINUOUS_SCAN;
+  if (scanData->count == 0) {
+    scanData->mode |= USB_CTR_CONTINUOUS_READOUT;
   }

   memcpy(&data[0], &scanData->count, 4);
   memcpy(&data[4], &scanData->retrig_count, 4);
   memcpy(&data[8], &pacer_period, 4);
-  data[12] = packet_size;
+  data[12] = packet_size-1;
   data[13] = scanData->options;

   ret = libusb_control_transfer(udev, requesttype, SCAN_START, 0x0, 0x0,(unsigned char *) data, sizeof(data), HS_DELAY);
@@ -725,10 +726,8 @@ int usbScanRead_USB_CTR(libusb_device_handle *udev, ScanData scanData, uint16_t
   uint16_t status;

-  if ( (scanData.mode & CONTINUOUS_SCAN) && (scanData.mode & SINGLEIO) ) {
-    nbytes = 2*(scanData.lastElement + 1);
-  } else if (scanData.mode & CONTINUOUS_SCAN) {
-    nbytes = 2*(scanData.packet_size+1);
+  if ((scanData.mode & USB_CTR_CONTINUOUS_READOUT) || (scanData.mode & USB_CTR_SINGLEIO)) {
+    nbytes = 2*(scanData.packet_size);
   } else {
     nbytes = scanData.count*(scanData.lastElement+1)*2;
   }
@@ -745,7 +744,7 @@ int usbScanRead_USB_CTR(libusb_device_handle *udev, ScanData scanData, uint16_t
     return ret;
   }

-  if (scanData.mode & CONTINUOUS_SCAN) { // continuous mode
+  if (scanData.mode & USB_CTR_CONTINUOUS_READOUT) { // continuous mode
     return transferred;
   }

This is a discussion of these changes:

packet_size is 1 larger than previously, and increased to uint16_t as discussed above.
CONTINUOUS_SCAN renamed to CONTINUOUS_READOUT as discussed above.
Implement the new FORCE_PACKET_SIZE option to use the scanData.packet_size passed in. I tested this by setting the packet_size to 128 when it would have normally been set to 256 by the logic here. This allows readouts that are more efficient than SINGLEIO but still more frequent than 256.
Removed the logic for frequency<10. This should not be part of the driver, it should be at the caller level. Then they can set the crossover point from SINGLEIO to normal at whatever frequency they want just by doing this for example:
```
if (scanData.frequency < 10) scanData.mode |= SINGLEIO;
```
Fixed a bug. If SCAN_CONTINUOUS was set then it was forcing scanData.count to 0. This is not correct, in my MCS mode I need count!=0 but CONTINUOUS_READOUT set.
The logic in usbScanRead_USB_CTR is simplified since either CONTINUOUS_READOUT or SINGLEIO means we should 2*request packet_size bytes.

I really like the API with these changes, and I have tested that it works fine with my application in many modes.

Thanks, Mark

wjasper commented 5 years ago

Hi Mark, I have approved your merges. Thanks for taking the time to go through the code and make suggestions to improve it. I think the API is moving in the right direction and the code feels cleaner with the scanData data structure that is shared among the methods that deal with scanIO.

MarkRivers commented 5 years ago

Thanks for merging it. I agree about the API.

wjasper / Linux_Drivers

Fixes to prefixes and size logic in usbScanStart_USB_CTR #32