From: Gerhard Sittig Date: Sun, 19 Mar 2023 14:59:31 +0000 (+0100) Subject: serial_bt, bluez: rework diag in BLE reception, accept zero length data X-Git-Url: https://sigrok.org/gitaction?a=commitdiff_plain;h=a8b0e6f538b9f1d5d30af3695a1b625c9b626935;p=libsigrok.git serial_bt, bluez: rework diag in BLE reception, accept zero length data Rework how the sr_bt_check_notify() routine processes messages that were received from a Bluetooth socket. Comment on edge cases that we have yet to see how they are handled. Extend the amount of details that are given in diagnostics messages, and tune their verbosity level. The debug level nicely illustrates how data flows, the spew level makes bytes available. Consider zero length data chunks in receive code paths non-fatal. Accept either NULL or non-NULL addresses when the length is zero. A NULL pointer in combination with a non-zero length is considered fatal though. --- diff --git a/src/bt/bt_bluez.c b/src/bt/bt_bluez.c index d0a3b7c8..71c0dbd4 100644 --- a/src/bt/bt_bluez.c +++ b/src/bt/bt_bluez.c @@ -74,6 +74,7 @@ #include #include #include +#include #include #include #include @@ -902,10 +903,14 @@ SR_PRIV int sr_bt_check_notify(struct sr_bt_desc *desc) { uint8_t buf[1024]; ssize_t rdlen; + const uint8_t *bufptr; + size_t buflen; uint8_t packet_type; uint16_t packet_handle; uint8_t *packet_data; size_t packet_dlen; + const char *type_text; + int ret; if (!desc) return -1; @@ -913,57 +918,96 @@ SR_PRIV int sr_bt_check_notify(struct sr_bt_desc *desc) if (sr_bt_check_socket_usable(desc) < 0) return -2; - /* Get another message from the Bluetooth socket. */ + /* + * Get another message from the Bluetooth socket. + * + * TODO Can we assume that every "message" comes in a separate + * read(2) call, or can data combine at the caller's? Need we + * loop over the received content until all was consumed? + */ rdlen = sr_bt_read(desc, buf, sizeof(buf)); - if (rdlen < 0) + if (rdlen < 0) { + sr_dbg("check notifiy, read error, %zd", rdlen); return -2; - if (!rdlen) + } + if (!rdlen) { + if (0) sr_spew("check notifiy, empty read"); return 0; + } + bufptr = &buf[0]; + buflen = (size_t)rdlen; + if (sr_log_loglevel_get() >= SR_LOG_SPEW) { + GString *txt; + txt = sr_hexdump_new(bufptr, buflen); + sr_spew("check notifiy, read succes, length %zd, data: %s", + rdlen, txt->str); + sr_hexdump_free(txt); + } - /* Get header fields and references to the payload data. */ - packet_type = 0x00; + /* + * Get header fields and references to the payload data. Notice + * that the first 16bit item after the packet type often is the + * handle, but need not always be. That is why the read position + * is kept, so that individual packet type handlers can either + * read _their_ layout strictly sequentially, or can conveniently + * access what a common preparation step has provided to them. + */ packet_handle = 0x0000; packet_data = NULL; packet_dlen = 0; - if (rdlen >= 1) - packet_type = buf[0]; - if (rdlen >= 3) { - packet_handle = bt_get_le16(&buf[1]); - packet_data = &buf[3]; - packet_dlen = rdlen - 3; + packet_type = read_u8_inc_len(&bufptr, &buflen); + if (buflen >= sizeof(uint16_t)) { + packet_handle = read_u16le(bufptr); + packet_data = (void *)&bufptr[sizeof(uint16_t)]; + packet_dlen = buflen - sizeof(uint16_t); + if (!packet_dlen) + packet_data = NULL; } + if (0) sr_spew("check notifiy, prep, hdl %" PRIu16 ", data %p len %zu", + packet_handle, packet_data, packet_dlen); /* Dispatch according to the message type. */ switch (packet_type) { case BLE_ATT_ERROR_RESP: - sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "error response"); + type_text = "error response"; + if (!buflen) { + sr_dbg("%s, no payload", type_text); + break; + } /* EMPTY */ + sr_dbg("%s, not handled here", type_text); break; case BLE_ATT_WRITE_RESP: - sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "write response"); - /* EMPTY */ + type_text = "write response"; + sr_dbg("%s, note taken", type_text); break; case BLE_ATT_HANDLE_INDICATION: - sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "handle indication"); + type_text = "handle indication"; + sr_dbg("%s, data len %zu", type_text, packet_dlen); sr_bt_write_type(desc, BLE_ATT_HANDLE_CONFIRMATION); + sr_spew("%s, confirmation sent", type_text); if (packet_handle != desc->read_handle) return -4; - if (!packet_data) - return -4; if (!desc->data_cb) return 0; - return desc->data_cb(desc->data_cb_data, packet_data, packet_dlen); + ret = desc->data_cb(desc->data_cb_data, + packet_data, packet_dlen); + sr_spew("%s, data cb ret %d", type_text, ret); + return ret; case BLE_ATT_HANDLE_NOTIFICATION: - sr_spew("read() len %zd, type 0x%02x (%s)", rdlen, buf[0], "handle notification"); + type_text = "handle notification"; + sr_dbg("%s, data len %zu", type_text, packet_dlen); if (packet_handle != desc->read_handle) return -4; - if (!packet_data) - return -4; if (!desc->data_cb) return 0; - return desc->data_cb(desc->data_cb_data, packet_data, packet_dlen); + ret = desc->data_cb(desc->data_cb_data, + packet_data, packet_dlen); + sr_spew("%s, data cb ret %d", type_text, ret); + return ret; default: - sr_spew("unsupported type 0x%02x", packet_type); + sr_dbg("unhandled type 0x%02x, len %zu", + packet_type, buflen); return -3; } diff --git a/src/serial_bt.c b/src/serial_bt.c index ba2edab0..33abec0f 100644 --- a/src/serial_bt.c +++ b/src/serial_bt.c @@ -408,6 +408,11 @@ static int ser_bt_data_cb(void *cb_data, uint8_t *data, size_t dlen) if (!serial) return -1; + if (!data && dlen) + return -1; + if (!data || !dlen) + return 0; + ser_bt_mask_databits(serial, data, dlen); sr_ser_queue_rx_data(serial, data, dlen);