On Mon, Nov 27, 2017 at 7:50 PM, Stefan Brüns
<stefan.bru...@rwth-aachen.de> wrote:
>
> On Thursday, November 23, 2017 3:46:55 PM CET Vincent Palatin wrote:
> > Add the hardware driver to acquire both USB Control Channels
> > using the Chromium "Twinkie" USB Power Delivery Analyzer dongle.
> >
> > Signed-off-by: Vincent Palatin <vpala...@chromium.org>
>
> Some comments inline ...
>
> Kind regards,
>
> Stefan
>
> > ---
> >  Makefile.am                              |   6 +
> >  configure.ac                             |   1 +
> >  contrib/60-libsigrok.rules               |   3 +
> >  src/hardware/chromium-twinkie/api.c      | 524
> > +++++++++++++++++++++++++++++++ src/hardware/chromium-twinkie/protocol.c |
> > 248 +++++++++++++++
> >  src/hardware/chromium-twinkie/protocol.h |  59 ++++
> >  6 files changed, 841 insertions(+)
> >  create mode 100644 src/hardware/chromium-twinkie/api.c
> >  create mode 100644 src/hardware/chromium-twinkie/protocol.c
> >  create mode 100644 src/hardware/chromium-twinkie/protocol.h
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 830d8cf9..dad304b6 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -258,6 +258,12 @@ src_libdrivers_la_SOURCES += \
> >       src/hardware/center-3xx/protocol.c \
> >       src/hardware/center-3xx/api.c
> >  endif
> > +if HW_CHROMIUM_TWINKIE
> > +src_libdrivers_la_SOURCES += \
> > +     src/hardware/chromium-twinkie/protocol.h \
> > +     src/hardware/chromium-twinkie/protocol.c \
> > +     src/hardware/chromium-twinkie/api.c
> > +endif
> >  if HW_CHRONOVU_LA
> >  src_libdrivers_la_SOURCES += \
> >       src/hardware/chronovu-la/protocol.h \
> > diff --git a/configure.ac b/configure.ac
> > index 56c097fa..16ebc0cc 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -229,6 +229,7 @@ SR_DRIVER([Brymen BM86x], [brymen-bm86x], [libusb])
> >  SR_DRIVER([Brymen DMM], [brymen-dmm], [libserialport])
> >  SR_DRIVER([CEM DT-885x], [cem-dt-885x], [libserialport])
> >  SR_DRIVER([Center 3xx], [center-3xx], [libserialport])
> > +SR_DRIVER([Chromium Twinkie], [chromium-twinkie], [libusb])
> >  SR_DRIVER([ChronoVu LA], [chronovu-la], [libusb libftdi])
> >  SR_DRIVER([Colead SLM], [colead-slm], [libserialport])
> >  SR_DRIVER([Conrad DIGI 35 CPU], [conrad-digi-35-cpu], [libserialport])
> > diff --git a/contrib/60-libsigrok.rules b/contrib/60-libsigrok.rules
> > index 498f40cd..38dfabc8 100644
> > --- a/contrib/60-libsigrok.rules
> > +++ b/contrib/60-libsigrok.rules
> > @@ -50,6 +50,9 @@ ATTRS{idVendor}=="0820", ATTRS{idProduct}=="0001",
> > ENV{ID_SIGROK}="1" # CEM DT-8852
> >  ATTRS{idVendor}=="10c4", ATTRS{idProduct}=="ea60", ENV{ID_SIGROK}="1"
> >
> > +# Chromium Twinkie USB PD analyzer dongle
> > +ATTRS{idVendor}=="18d1", ATTRS{idProduct}=="500a", ENV{ID_SIGROK}="1"
> > +
> >  # ChronoVu LA8 (new VID/PID)
> >  # ChronoVu LA16 (new VID/PID)
> >  ATTRS{idVendor}=="0403", ATTRS{idProduct}=="8867", ENV{ID_SIGROK}="1"
> > diff --git a/src/hardware/chromium-twinkie/api.c
> > b/src/hardware/chromium-twinkie/api.c new file mode 100644
> > index 00000000..15f4cf9f
> > --- /dev/null
> > +++ b/src/hardware/chromium-twinkie/api.c
> > @@ -0,0 +1,524 @@
> > +/*
> > + * This file is part of the libsigrok project.
> > + *
> > + * Copyright 2017 Google, Inc
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <config.h>
> > +#include <libusb.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <math.h>
> > +#include <libsigrok/libsigrok.h>
> > +#include "libsigrok-internal.h"
> > +#include "protocol.h"
> > +
> > +#define TWINKIE_VID          0x18d1
> > +#define TWINKIE_PID          0x500a
> > +
> > +#define USB_INTERFACE                1
> > +#define USB_CONFIGURATION    1
> > +
> > +#define MAX_RENUM_DELAY_MS   3000
> > +#define NUM_SIMUL_TRANSFERS  32
> > +
> > +#define SAMPLE_RATE          SR_KHZ(2400)
> > +
> > +static const int32_t hwopts[] = {
> > +     SR_CONF_CONN,
> > +};
> > +
> > +static const int32_t hwcaps[] = {
> > +     SR_CONF_LOGIC_ANALYZER,
> > +     SR_CONF_CONTINUOUS,
> > +     SR_CONF_CONN | SR_CONF_GET,
> > +     SR_CONF_SAMPLERATE | SR_CONF_GET,
> > +     SR_CONF_LIMIT_SAMPLES | SR_CONF_SET,
> > +};
> > +
> > +static const struct chan {
> > +     char *name;
> > +     int type;
> > +} chan_defs[] = {
> > +     {"CC1", SR_CHANNEL_LOGIC},
> > +     {"CC2", SR_CHANNEL_LOGIC},
> > +
> > +     {NULL, 0}
> > +};
> > +
> > +static GSList *scan(struct sr_dev_driver *di, GSList *options)
> > +{
> > +     struct drv_context *drvc;
> > +     struct dev_context *devc;
> > +     struct sr_dev_inst *sdi;
> > +     struct sr_usb_dev_inst *usb;
> > +     struct sr_channel *ch;
> > +     struct sr_config *src;
> > +     GSList *l, *devices, *conn_devices;
> > +     struct libusb_device_descriptor des;
> > +     libusb_device **devlist;
> > +     int ret, i, j;
> > +     const char *conn;
> > +     char connection_id[64];
> > +
> > +     drvc = di->context;
> > +
> > +     conn = NULL;
> > +     for (l = options; l; l = l->next) {
> > +             src = l->data;
> > +             switch (src->key) {
> > +             case SR_CONF_CONN:
> > +                     conn = g_variant_get_string(src->data, NULL);
> > +                     break;
> > +             }
> > +     }
> > +     if (conn)
> > +             conn_devices = sr_usb_find(drvc->sr_ctx->libusb_ctx, conn);
> > +     else
> > +             conn_devices = NULL;
> > +
> > +     /* Find all Twinkie devices */
> > +     devices = NULL;
> > +     libusb_get_device_list(drvc->sr_ctx->libusb_ctx, &devlist);
> > +     for (i = 0; devlist[i]; i++) {
> > +             if (conn) {
> > +                     usb = NULL;
> > +                     for (l = conn_devices; l; l = l->next) {
> > +                             usb = l->data;
> > +                             if (usb->bus == 
> > libusb_get_bus_number(devlist[i])
> > +                                 && usb->address == 
> > libusb_get_device_address(devlist[i]))
> > +                                     break;
> > +                     }
> > +                     if (!l)
> > +                             /* This device matched none of the ones that
> > +                              * matched the conn specification. */
> > +                             continue;
> > +             }
> > +
> > +             if ((ret = libusb_get_device_descriptor(devlist[i], &des)) != 
> > 0) {
> > +                     sr_warn("Failed to get device descriptor: %s.",
> > +                             libusb_error_name(ret));
> > +                     continue;
> > +             }
> > +
> > +             if (des.idVendor != TWINKIE_VID || des.idProduct != 
> > TWINKIE_PID)
> > +                     continue;
> > +
> > +             usb_get_port_path(devlist[i], connection_id, 
> > sizeof(connection_id));
> > +
> > +             sdi = g_malloc0(sizeof(struct sr_dev_inst));
> > +             sdi->status = SR_ST_INITIALIZING;
> > +             sdi->vendor = g_strdup("Chromium");
> > +             sdi->model = g_strdup("Twinkie");
> > +             sdi->driver = di;
> > +             sdi->connection_id = g_strdup(connection_id);
> > +
> > +             for (j = 0; chan_defs[j].name; j++)
> > +                     if (!(ch = sr_channel_new(sdi, j, chan_defs[j].type,
> > +                                               TRUE, chan_defs[j].name)))
> > +                             return NULL;
> > +
> > +             if (!(devc = g_try_malloc0(sizeof(struct dev_context))))
> > +                     return NULL;
>
> You leak sdi here.


Good point, in V3 I will use g_malloc0 here as all other drivers.
Same thing I don't to check sr_channel_new above (which g_malloc0)



>
>
> > +             sdi->priv = devc;
> > +             drvc->instances = g_slist_append(drvc->instances, sdi);
> > +             devices = g_slist_append(devices, sdi);
> > +
> > +             sr_dbg("Found a Twinkie dongle.");
> > +             sdi->status = SR_ST_INACTIVE;
> > +             sdi->inst_type = SR_INST_USB;
> > +             sdi->conn = sr_usb_dev_inst_new(
> > +                     libusb_get_bus_number(devlist[i]),
> > +                     libusb_get_device_address(devlist[i]), NULL);
> > +     }
> > +     libusb_free_device_list(devlist, 1);
> > +     g_slist_free_full(conn_devices, (GDestroyNotify)sr_usb_dev_inst_free);
> > +
> > +     return devices;


I should have put std_scan_complete(di, devices) here
I missed that conversion.


> > +}
> > +
> > +static int twinkie_dev_open(struct sr_dev_inst *sdi)
> > +{
> > +     struct sr_dev_driver *di;
> > +     libusb_device **devlist;
> > +     struct sr_usb_dev_inst *usb;
> > +     struct libusb_device_descriptor des;
> > +     struct drv_context *drvc;
> > +     int ret, i, device_count;
> > +     char connection_id[64];
> > +
> > +     di = sdi->driver;
> > +     drvc = di->context;
> > +     usb = sdi->conn;
> > +
> > +     if (sdi->status == SR_ST_ACTIVE)
> > +             /* Device is already in use. */
> > +             return SR_ERR;
> > +
> > +     device_count = libusb_get_device_list(drvc->sr_ctx->libusb_ctx, 
> > &devlist);
> > +     if (device_count < 0) {
> > +             sr_err("Failed to get device list: %s.",
> > +                    libusb_error_name(device_count));
> > +             return SR_ERR;
> > +     }
> > +
> > +     for (i = 0; i < device_count; i++) {
> > +             if ((ret = libusb_get_device_descriptor(devlist[i], &des))) {
> > +                     sr_err("Failed to get device descriptor: %s.",
> > +                            libusb_error_name(ret));
> > +                     continue;
> > +             }
> > +
> > +             if (des.idVendor != TWINKIE_VID || des.idProduct != 
> > TWINKIE_PID)
> > +                     continue;
> > +
> > +             if ((sdi->status == SR_ST_INITIALIZING) ||
> > +                             (sdi->status == SR_ST_INACTIVE)) {
> > +                     /*
> > +                      * Check device by its physical USB bus/port address.
> > +                      */
> > +                     usb_get_port_path(devlist[i], connection_id, 
> > sizeof(connection_id));
> > +                     if (strcmp(sdi->connection_id, connection_id))
> > +                             /* This is not the one. */
> > +                             continue;
> > +             }
> > +
> > +             if (!(ret = libusb_open(devlist[i], &usb->devhdl))) {
> > +                     if (usb->address == 0xff)
> > +                             /*
> > +                              * First time we touch this device after FW
> > +                              * upload, so we don't know the address yet.
> > +                              */
> > +                             usb->address = 
> > libusb_get_device_address(devlist[i]);
> > +             } else {
> > +                     sr_err("Failed to open device: %s.",
> > +                            libusb_error_name(ret));
> > +                     break;
> > +             }
> > +
> > +             ret = libusb_claim_interface(usb->devhdl, USB_INTERFACE);
> > +             if (ret == LIBUSB_ERROR_BUSY) {
> > +                     sr_err("Unable to claim USB interface. Another "
> > +                            "program or driver has already claimed it.");
> > +                     break;
> > +             } else if (ret == LIBUSB_ERROR_NO_DEVICE) {
> > +                     sr_err("Device has been disconnected.");
> > +                     break;
> > +             } else if (ret != 0) {
> > +                     sr_err("Unable to claim interface: %s.",
> > +                            libusb_error_name(ret));
> > +                     break;
> > +             }
> > +
> > +             if ((ret = twinkie_init_device(sdi)) != SR_OK) {
> > +                     sr_err("Failed to init device.");
> > +                     break;
> > +             }
> > +
> > +             sdi->status = SR_ST_ACTIVE;
> > +             sr_info("Opened device %d.%d, interface %d.",
> > +                     usb->bus, usb->address, USB_INTERFACE);
> > +
> > +             break;
> > +     }
> > +     libusb_free_device_list(devlist, 1);
> > +
> > +     if (sdi->status != SR_ST_ACTIVE) {
> > +             if (usb->devhdl) {
> > +                     libusb_release_interface(usb->devhdl, USB_INTERFACE);
> > +                     libusb_close(usb->devhdl);
> > +                     usb->devhdl = NULL;
> > +             }
> > +             return SR_ERR;
> > +     }
> > +
> > +     return SR_OK;
> > +}
> > +
> > +static int dev_open(struct sr_dev_inst *sdi)
> > +{
> > +     int ret;
> > +
> > +     ret = twinkie_dev_open(sdi);
> > +     if (ret != SR_OK) {
> > +             sr_err("Unable to open device.");
> > +             return SR_ERR;
> > +     }
> > +
> > +     return SR_OK;
> > +}
> > +
> > +static int dev_close(struct sr_dev_inst *sdi)
> > +{
> > +     struct sr_usb_dev_inst *usb;
> > +
> > +     usb = sdi->conn;
> > +     if (usb->devhdl == NULL)
> > +             return SR_ERR;
> > +
> > +     sr_info("Closing device %d.%d interface %d.",
> > +             usb->bus, usb->address, USB_INTERFACE);
> > +     libusb_release_interface(usb->devhdl, USB_INTERFACE);
> > +     libusb_close(usb->devhdl);
> > +     usb->devhdl = NULL;
> > +     sdi->status = SR_ST_INACTIVE;
> > +
> > +     return SR_OK;
> > +}
> > +
> > +static int dev_clear(const struct sr_dev_driver *di)
> > +{
> > +     return std_dev_clear(di);
> > +}
> > +
> > +static int config_get(uint32_t key, GVariant **data,
> > +             const struct sr_dev_inst *sdi,
> > +             const struct sr_channel_group *cg)
> > +{
> > +     struct sr_usb_dev_inst *usb;
> > +     char str[128];
> > +     int ret;
> > +
> > +     (void)cg;
> > +
> > +     ret = SR_OK;
> > +     switch (key) {
> > +     case SR_CONF_CONN:
> > +             if (!sdi || !sdi->conn)
> > +                     return SR_ERR_ARG;
> > +             usb = sdi->conn;
> > +             if (usb->address == 255)
> > +                     /* Device still needs to re-enumerate after firmware
> > +                      * upload, so we don't know its (future) address. */
> > +                     return SR_ERR;
> > +             snprintf(str, 128, "%d.%d", usb->bus, usb->address);
> > +             *data = g_variant_new_string(str);
> > +             break;
> > +     case SR_CONF_SAMPLERATE:
> > +             *data = g_variant_new_uint64(SAMPLE_RATE);
> > +             break;
> > +     default:
> > +             return SR_ERR_NA;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int config_set(uint32_t key, GVariant *data,
> > +             const struct sr_dev_inst *sdi,
> > +             const struct sr_channel_group *cg)
> > +{
> > +     struct dev_context *devc;
> > +     int ret;
> > +
> > +     (void)cg;
> > +
> > +     if (sdi->status != SR_ST_ACTIVE)
> > +             return SR_ERR_DEV_CLOSED;
> > +
> > +     devc = sdi->priv;
> > +
> > +     ret = SR_OK;
> > +     switch (key) {
> > +     case SR_CONF_LIMIT_SAMPLES:
> > +             devc->limit_samples = g_variant_get_uint64(data);
> > +             break;
> > +     default:
> > +             ret = SR_ERR_NA;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int config_list(uint32_t key, GVariant **data,
> > +             const struct sr_dev_inst *sdi,
> > +             const struct sr_channel_group *cg)
> > +{
> > +     int ret;
> > +
> > +     (void)sdi;
> > +     (void)cg;
> > +
> > +     ret = SR_OK;
> > +     switch (key) {
> > +     case SR_CONF_SCAN_OPTIONS:
> > +             *data = g_variant_new_fixed_array(G_VARIANT_TYPE_INT32,
> > +                             hwopts, ARRAY_SIZE(hwopts), sizeof(int32_t));
> > +             break;
> > +     case SR_CONF_DEVICE_OPTIONS:
> > +             *data = g_variant_new_fixed_array(G_VARIANT_TYPE_INT32,
> > +                             hwcaps, ARRAY_SIZE(hwcaps), sizeof(int32_t));
> > +             break;
> > +     default:
> > +             return SR_ERR_NA;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static void abort_acquisition(struct dev_context *devc)
> > +{
> > +     int i;
> > +
> > +     devc->sent_samples = -1;
> > +
> > +     for (i = devc->num_transfers - 1; i >= 0; i--) {
> > +             if (devc->transfers[i])
> > +                     libusb_cancel_transfer(devc->transfers[i]);
> > +     }
> > +}
> > +
> > +static int receive_data(int fd, int revents, void *cb_data)
> > +{
> > +     struct timeval tv;
> > +     struct dev_context *devc;
> > +     struct drv_context *drvc;
> > +     const struct sr_dev_inst *sdi;
> > +     struct sr_dev_driver *di;
> > +
> > +     (void)fd;
> > +     (void)revents;
> > +
> > +     sdi = cb_data;
> > +     di = sdi->driver;
> > +     drvc = di->context;
> > +     devc = sdi->priv;
> > +
> > +     tv.tv_sec = tv.tv_usec = 0;
> > +     libusb_handle_events_timeout(drvc->sr_ctx->libusb_ctx, &tv);
> > +
> > +     if (devc->sent_samples == -2) {
> > +             abort_acquisition(devc);
> > +     }
> > +
> > +     return TRUE;
> > +}
> > +
> > +static int dev_acquisition_start(const struct sr_dev_inst *sdi)
> > +{
> > +     struct sr_dev_driver *di = sdi->driver;
> > +     struct dev_context *devc;
> > +     struct drv_context *drvc;
> > +     struct sr_usb_dev_inst *usb;
> > +     struct libusb_transfer *transfer;
> > +     unsigned int i, timeout, num_transfers;
> > +     int ret;
> > +     unsigned char *buf;
> > +     size_t size, convsize;
> > +
> > +     if (sdi->status != SR_ST_ACTIVE)
> > +             return SR_ERR_DEV_CLOSED;
> > +
> > +     drvc = di->context;
> > +     devc = sdi->priv;
> > +     usb = sdi->conn;
> > +
> > +     devc->sent_samples = 0;
> > +     /* reset per-CC context */
> > +     memset(devc->cc, 0, sizeof(devc->cc));
> > +
> > +     timeout = 1000;
> > +     num_transfers = 10;
> > +     size = 10*1024;
> > +     convsize = size * 8 * 256 /* largest size : only rollbacks/no edges 
> > */;
> > +     devc->submitted_transfers = 0;
> > +
> > +     devc->convbuffer_size = convsize;
> > +     if (!(devc->convbuffer = g_try_malloc(convsize))) {
>
> Are you sure you need a 20 MByte conversion buffer?


The (low-cost) device on the other side has not much buffering,
In order to avoid losing samples while doing continuous acquisition,
I really want the 10KB of USB buffers above and unfortunately they can
expand up to those 20MB once uncompressed to the Sigrok logic format
(see more about the frame format below).
That said, we can put the cursor somewhere else, but for me the 20MB
of RAM is cheaper than a broken/underflow capture to re-do or spending
time debugging/optimizing random janks on my PC. We can add a
parameter for this if the trade-off seems too extreme for some users
(but I would let the default to this value)


>
> > +             sr_err("Conversion buffer malloc failed.");
> > +             return SR_ERR_MALLOC;
> > +     }
> > +     memset(devc->convbuffer, 0, devc->convbuffer_size);
> > +
> > +     devc->transfers = g_try_malloc0(sizeof(*devc->transfers) * 
> > num_transfers);
> > +     if (!devc->transfers) {
> > +             sr_err("USB transfers malloc failed.");
> > +             g_free(devc->convbuffer);
> > +             return SR_ERR_MALLOC;
> > +     }
> > +
> > +     devc->num_transfers = num_transfers;
> > +     for (i = 0; i < num_transfers; i++) {
> > +             if (!(buf = g_try_malloc(size))) {
> > +                     sr_err("USB transfer buffer malloc failed.");
> > +                     if (devc->submitted_transfers)
> > +                             abort_acquisition(devc);
> > +                     else {
> > +                             g_free(devc->transfers);
> > +                             g_free(devc->convbuffer);
> > +                     }
> > +                     return SR_ERR_MALLOC;
> > +             }
> > +             transfer = libusb_alloc_transfer(0);
> > +             libusb_fill_bulk_transfer(transfer, usb->devhdl,
> > +                             3 | LIBUSB_ENDPOINT_IN, buf, size,
> > +                             twinkie_receive_transfer, (void *)sdi, 
> > timeout);
> > +             if ((ret = libusb_submit_transfer(transfer)) != 0) {
> > +                     sr_err("Failed to submit transfer: %s.",
> > +                            libusb_error_name(ret));
> > +                     libusb_free_transfer(transfer);
> > +                     g_free(buf);
> > +                     abort_acquisition(devc);
> > +                     return SR_ERR;
> > +             }
> > +             devc->transfers[i] = transfer;
> > +             devc->submitted_transfers++;
> > +     }
> > +
> > +     devc->ctx = drvc->sr_ctx;
> > +
> > +     usb_source_add(sdi->session, devc->ctx, timeout, receive_data,
> > +                   (void *)sdi);
> > +
> > +     /* Send header packet to the session bus. */
> > +     std_session_send_df_header(sdi);
> > +
> > +     if ((ret = twinkie_start_acquisition(sdi)) != SR_OK) {
> > +             abort_acquisition(devc);
> > +             return ret;
> > +     }
> > +
> > +     return SR_OK;
> > +}
> > +
> > +static int dev_acquisition_stop(struct sr_dev_inst *sdi)
> > +{
> > +     if (sdi->status != SR_ST_ACTIVE)
> > +             return SR_ERR_DEV_CLOSED;
> > +
> > +     abort_acquisition(sdi->priv);
> > +
> > +     return SR_OK;
> > +}
> > +
> > +static struct sr_dev_driver chromium_twinkie_driver_info = {
> > +     .name = "chromium-twinkie",
> > +     .longname = "Chromium Twinkie",
> > +     .api_version = 1,
> > +     .init = std_init,
> > +     .cleanup = std_cleanup,
> > +     .scan = scan,
> > +     .dev_list = std_dev_list,
> > +     .dev_clear = dev_clear,
> > +     .config_get = config_get,
> > +     .config_set = config_set,
> > +     .config_list = config_list,
> > +     .dev_open = dev_open,
> > +     .dev_close = dev_close,
> > +     .dev_acquisition_start = dev_acquisition_start,
> > +     .dev_acquisition_stop = dev_acquisition_stop,
> > +};
> > +SR_REGISTER_DEV_DRIVER(chromium_twinkie_driver_info);
> > diff --git a/src/hardware/chromium-twinkie/protocol.c
> > b/src/hardware/chromium-twinkie/protocol.c new file mode 100644
> > index 00000000..c7327bd5
> > --- /dev/null
> > +++ b/src/hardware/chromium-twinkie/protocol.c
> > @@ -0,0 +1,248 @@
> > +/*
> > + * This file is part of the libsigrok project.
> > + *
> > + * Copyright 2017 Google, Inc
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <config.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +#include <libusb.h>
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <math.h>
> > +#include <libsigrok/libsigrok.h>
> > +#include "libsigrok-internal.h"
> > +#include "protocol.h"
> > +
> > +SR_PRIV int twinkie_start_acquisition(const struct sr_dev_inst *sdi)
> > +{
> > +     (void)sdi;
> > +
> > +     return SR_OK;
> > +}
> > +
> > +SR_PRIV int twinkie_init_device(const struct sr_dev_inst *sdi)
> > +{
> > +     (void)sdi;
> > +
> > +     return SR_OK;
> > +}
> > +
> > +static void finish_acquisition(struct sr_dev_inst *sdi)
> > +{
> > +     struct sr_datafeed_packet packet;
> > +     struct dev_context *devc = sdi->priv;
> > +
> > +     /* Terminate session. */
> > +     packet.type = SR_DF_END;
> > +     sr_session_send(sdi, &packet);
> > +
> > +     /* Remove fds from polling. */
> > +     usb_source_remove(sdi->session, devc->ctx);
> > +
> > +     devc->num_transfers = 0;
> > +     g_free(devc->transfers);
> > +     g_free(devc->convbuffer);
> > +}
> > +
> > +static void free_transfer(struct libusb_transfer *transfer)
> > +{
> > +     struct sr_dev_inst *sdi;
> > +     struct dev_context *devc;
> > +     unsigned int i;
> > +
> > +     sdi = transfer->user_data;
> > +     devc = sdi->priv;
> > +
> > +     g_free(transfer->buffer);
> > +     transfer->buffer = NULL;
> > +     libusb_free_transfer(transfer);
> > +
> > +     for (i = 0; i < devc->num_transfers; i++) {
> > +             if (devc->transfers[i] == transfer) {
> > +                     devc->transfers[i] = NULL;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     devc->submitted_transfers--;
> > +     if (devc->submitted_transfers == 0)
> > +             finish_acquisition(sdi);
> > +}
> > +
> > +static void export_samples(const struct sr_dev_inst *sdi, size_t cnt)
> > +{
> > +     struct sr_datafeed_packet packet;
> > +     struct sr_datafeed_logic logic;
> > +     struct dev_context *devc = sdi->priv;
> > +
> > +     /* export the received data */
> > +     packet.type = SR_DF_LOGIC;
> > +     packet.payload = &logic;
> > +     if (devc->limit_samples &&
> > +         cnt > devc->limit_samples - devc->sent_samples)
> > +             cnt = devc->limit_samples - devc->sent_samples;
> > +     logic.length = cnt;
> > +     logic.unitsize = 1;
> > +     logic.data = devc->convbuffer;
> > +     sr_session_send(sdi, &packet);
> > +     devc->sent_samples += cnt;
> > +}
> > +
>
> Can you add information about the frame format? Apparently there is a header,
> and the data seems to be compressed?

Each 64-byte USB full-speed packet is something like this:
  U16 timestamp /* capture timestamp in microseconds (thus rolling
over every 65ms) */
  U16 seq  /* [15] underrun detected [12] CCx Channel (CC1=0, CC2=1)
                     [11:0]  frame sequence number */
  U8 edges[60] /* edge timings see details below */

For the recording the edges seen on the CC1/CC2 logic channels, it is
using a 2.4Mhz free running hardware counter. Everytime a transition
is seen (raising edge or falling edge) on the channel, the value of
the 8-bit counter is recorded in the edges array. So basically one
sample is giving the delay (N * 1/2.4Mhz) between 2 edges, excepted
the following detail: when the hw counter is rolling over, it's
recording another time the last sample in the array (so we can detect
longer delay/multiple rollovers).
This scheme lose easily the absolute reference (ie is our starting
point level 0 or 1), but given the USB PD Control channels are
BMC-encoded, it's not terribly important.

This scheme compresses a lot the logic data when there are few
transitions/packets on the wire (which is the normal case), ie with no
transition, we end up with one sample every rollover (256*1/2.4Mhz =
0,1 ms) so on packet every 6.4ms. But Sigrok  has only a fixed sample
rate logic format, so it expand a lot here, hence the 20MB buffer
after decompression.

I will add a blurb about it.


>
> > +static void expand_sample_data(const struct sr_dev_inst *sdi,
> > +                            const uint8_t *src, size_t srccnt)
> > +{
> > +     struct dev_context *devc = sdi->priv;
> > +     int i, f;
> > +     size_t b;
> > +     size_t rdy_samples, left_samples;
> > +     int frames = srccnt / 64;
> > +
> > +     for (f = 0; f < frames; f++) {
> > +             int ch = (src[1] >> 4) & 3; /* samples channel number */
> > +             int bit = 1 << ch; /* channel bit mask */
> > +             struct cc_context *cc = devc->cc + ch;
> > +             uint8_t *dest = devc->convbuffer + cc->idx;
> > +
> > +             if (ch >= 2) /* only acquires CCx channels */
> > +                     continue;
> > +
> > +             /* TODO: check timestamp, overflow, sequence number */
> > +
>
> I think the function becomes easier to read if you put the loop body (i.e. the
> frame parsing) into a separate function an call that from the loop.


Ok, I will try this for V3

>
> > +             /* skip header, go to edges data */
> > +             src+=4;
> > +             for (i = 0; i < 60; i++,src++)
> > +                     if (*src == cc->prev_src) {
> > +                             cc->rollbacks++;
> > +                     } else {
> > +                             uint8_t diff = *src - cc->prev_src;
> > +                             int fixup = cc->rollbacks && (((int)*src < 
> > (int)cc->prev_src) ||
> (*src
> > == 0xff)); +                          size_t total = (fixup ? cc->rollbacks 
> > - 1 : cc-
> >rollbacks)
> > * 256 + diff; +
> > +                             if (total + cc->idx > devc->convbuffer_size) {
> > +                                     sr_warn("overflow %d+%zd/%zd\n",
> > +                                             cc->idx, total,
> > +                                             devc->convbuffer_size);
> > +                                     /* reset current decoding */
> > +                                     cc->rollbacks = 0;
> > +                                     break;
> > +                             }
> > +
> > +                             /* insert bits in the buffer */
> > +                             if (cc->level)
> > +                                     for (b = 0 ; b < total ; b++, dest++)
> > +                                             *dest |= bit;
> > +                             else
> > +                                     dest += total;
> > +                             cc->idx += total;
> > +
> > +                             /* flip level on the next edge */
> > +                             cc->level = ~cc->level;
> > +
> > +                             cc->rollbacks = 0;
> > +                             cc->prev_src = *src;
> > +                     }
> > +             /* expand repeated rollbacks */
> > +             if (cc->rollbacks > 1) {
> > +                     size_t total = 256 * (cc->rollbacks - 1);
> > +                     if (total + cc->idx > devc->convbuffer_size) {
> > +                             sr_warn("overflow %d+%zd/%zd\n",
> > +                                     cc->idx, total, 
> > devc->convbuffer_size);
> > +                             /* reset current decoding */
> > +                             total = 0;
> > +                     }
> > +                     /* insert bits in the buffer */
> > +                     if (cc->level)
> > +                             for (b = 0 ; b < total ; b++, dest++)
> > +                                     *dest |= bit ;
> > +                     cc->idx += total;
> > +                     cc->rollbacks = 1;
> > +             }
> > +     }
> > +
> > +     /* samples ready to be pushed (with both channels) */
> > +     rdy_samples = MIN(devc->cc[0].idx, devc->cc[1].idx);
> > +     left_samples = MAX(devc->cc[0].idx, devc->cc[1].idx) - rdy_samples;
> > +     /* skip empty transfer */
> > +     if (rdy_samples == 0)
> > +             return;
> > +
> > +     export_samples(sdi, rdy_samples);
> > +
> > +     /* clean up what we have sent */
> > +     memmove(devc->convbuffer, devc->convbuffer + rdy_samples, 
> > left_samples);
> > +     memset(devc->convbuffer + left_samples, 0, rdy_samples);
> > +     devc->cc[0].idx -= rdy_samples;
> > +     devc->cc[1].idx -= rdy_samples;
> > +}
> > +
> > +SR_PRIV void LIBUSB_CALL twinkie_receive_transfer(struct libusb_transfer
> > *transfer) +{
> > +     gboolean packet_has_error = FALSE;
> > +     struct sr_dev_inst *sdi;
> > +     struct dev_context *devc;
> > +
> > +     sdi = transfer->user_data;
> > +     devc = sdi->priv;
> > +
> > +     /*
> > +      * If acquisition has already ended, just free any queued up
> > +      * transfer that come in.
> > +      */
> > +     if (devc->sent_samples < 0) {
> > +             free_transfer(transfer);
> > +             return;
> > +     }
> > +
> > +     if (transfer->status || transfer->actual_length)
> > +             sr_info("receive_transfer(): status %d received %d bytes.",
> > +                     transfer->status, transfer->actual_length);
> > +
> > +     switch (transfer->status) {
> > +     case LIBUSB_TRANSFER_NO_DEVICE:
> > +             devc->sent_samples = -2;
> > +             free_transfer(transfer);
> > +             return;
> > +     case LIBUSB_TRANSFER_COMPLETED:
> > +     case LIBUSB_TRANSFER_TIMED_OUT: /* We may have received some data 
> > though.
> > */ +          break;
> > +     default:
> > +             packet_has_error = TRUE;
> > +             break;
> > +     }
> > +
> > +     if (transfer->actual_length % 64) {
> > +             sr_err("Bad USB packet size.");
> > +             packet_has_error = TRUE;
> > +     }
> > +
> > +     if (transfer->actual_length == 0 || packet_has_error)
> > +             goto resubmit;
> > +
> > +     /* decode received edges */
> > +     expand_sample_data(sdi, transfer->buffer, transfer->actual_length);
> > +
> > +     if (devc->limit_samples &&
> > +                     (uint64_t)devc->sent_samples >= devc->limit_samples) {
> > +             devc->sent_samples = -2;
> > +             free_transfer(transfer);
> > +             return;
> > +     }
> > +resubmit:
> > +     if (libusb_submit_transfer(transfer) != LIBUSB_SUCCESS)
> > +             free_transfer(transfer);
> > +}
> > diff --git a/src/hardware/chromium-twinkie/protocol.h
> > b/src/hardware/chromium-twinkie/protocol.h new file mode 100644
> > index 00000000..3a282cc1
> > --- /dev/null
> > +++ b/src/hardware/chromium-twinkie/protocol.h
> > @@ -0,0 +1,59 @@
> > +/*
> > + * This file is part of the libsigrok project.
> > + *
> > + * Copyright 2017 Google, Inc
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef LIBSIGROK_HARDWARE_CHROMIUM_TWINKIE_PROTOCOL_H
> > +#define LIBSIGROK_HARDWARE_CHROMIUM_TWINKIE_PROTOCOL_H
> > +
> > +#include <stdint.h>
> > +#include <glib.h>
> > +#include <libsigrok/libsigrok.h>
> > +#include "libsigrok-internal.h"
> > +
> > +#define LOG_PREFIX "twinkie"
> > +
> > +/** Private, per-CC logical channel context. */
> > +struct cc_context {
> > +     int idx;
> > +     int rollbacks;
> > +     uint8_t prev_src;
> > +     uint8_t level;
> > +};
> > +
> > +/** Private, per-device-instance driver context. */
> > +struct dev_context {
> > +     /** Maximum number of samples to capture, if nonzero. */
> > +     uint64_t limit_samples;
> > +
> > +     int64_t sent_samples;
> > +     int submitted_transfers;
> > +     uint8_t *convbuffer;
> > +     size_t convbuffer_size;
> > +
> > +     unsigned int num_transfers;
> > +     struct libusb_transfer **transfers;
> > +     struct sr_context *ctx;
> > +
> > +     struct cc_context cc[2];
> > +};
> > +
> > +SR_PRIV int twinkie_start_acquisition(const struct sr_dev_inst *sdi);
> > +SR_PRIV int twinkie_init_device(const struct sr_dev_inst *sdi);
> > +SR_PRIV void LIBUSB_CALL twinkie_receive_transfer(struct libusb_transfer
> > *transfer); +
> > +#endif
>
>
> --
> Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
> home: +49 241 53809034     mobile: +49 151 50412019
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> sigrok-devel mailing list
> sigrok-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/sigrok-devel
>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel

Reply via email to