You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by "m-gorecki (via GitHub)" <gi...@apache.org> on 2023/05/25 13:45:38 UTC
[GitHub] [mynewt-nimble] m-gorecki opened a new pull request, #1507: Add BIGInfo reports to controller and host
m-gorecki opened a new pull request, #1507:
URL: https://github.com/apache/mynewt-nimble/pull/1507
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] andrzej-kaczmarek commented on a diff in pull request #1507: Add BIGInfo reports to controller and host
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #1507:
URL: https://github.com/apache/mynewt-nimble/pull/1507#discussion_r1206580766
##########
nimble/host/include/host/ble_gap.h:
##########
@@ -982,6 +983,54 @@ struct ble_gap_event {
} periodic_transfer;
#endif
+#if MYNEWT_VAL(BLE_BIGINFO_REPORTS)
+ /**
+ * Represents a periodic advertising sync transfer received. Valid for
+ * the following event types:
+ * o BLE_GAP_EVENT_BIGINFO_REPORT
+ */
+ struct {
+ /** Synchronization handle */
+ uint16_t sync_handle;
+
+ /** Number of present BISes */
+ uint8_t bis_cnt;
+
+ /** Number of SubEvents */
+ uint8_t nse;
+
+ /** ISO Interval */
+ uint16_t iso_interval;
+
+ /** Burst Number */
+ uint8_t bn;
+
+ /** Pre-Transmission Offset */
+ uint8_t pto;
+
+ /** Immediate Repetition Count */
+ uint8_t irc;
+
+ /** Maximum PDU size */
+ uint16_t max_pdu;
+
+ /** Service Data Unit Interval */
+ uint8_t sdu_interval[3];
Review Comment:
`uint32_t` will be easier to handle by app
also move below `max_sdu` so we don't waste space on alignment unnecessarily
##########
nimble/host/include/host/ble_gap.h:
##########
@@ -982,6 +983,54 @@ struct ble_gap_event {
} periodic_transfer;
#endif
+#if MYNEWT_VAL(BLE_BIGINFO_REPORTS)
+ /**
+ * Represents a periodic advertising sync transfer received. Valid for
+ * the following event types:
+ * o BLE_GAP_EVENT_BIGINFO_REPORT
+ */
+ struct {
+ /** Synchronization handle */
+ uint16_t sync_handle;
+
+ /** Number of present BISes */
+ uint8_t bis_cnt;
+
+ /** Number of SubEvents */
+ uint8_t nse;
+
+ /** ISO Interval */
+ uint16_t iso_interval;
+
+ /** Burst Number */
+ uint8_t bn;
+
+ /** Pre-Transmission Offset */
+ uint8_t pto;
+
+ /** Immediate Repetition Count */
+ uint8_t irc;
+
+ /** Maximum PDU size */
+ uint16_t max_pdu;
+
+ /** Service Data Unit Interval */
+ uint8_t sdu_interval[3];
+
+ /** Maximum SDU size */
+ uint16_t max_sdu;
+
+ /** Advertising PHY */
+ uint8_t phy;
+
+ /** Framing of BIS Data PDUs */
+ uint8_t framing;
+
+ /** Encryption */
+ uint8_t encryption;
Review Comment:
use bitfield for both
##########
nimble/host/include/host/ble_gap.h:
##########
@@ -982,6 +983,54 @@ struct ble_gap_event {
} periodic_transfer;
#endif
+#if MYNEWT_VAL(BLE_BIGINFO_REPORTS)
+ /**
+ * Represents a periodic advertising sync transfer received. Valid for
+ * the following event types:
+ * o BLE_GAP_EVENT_BIGINFO_REPORT
+ */
+ struct {
+ /** Synchronization handle */
+ uint16_t sync_handle;
+
+ /** Number of present BISes */
+ uint8_t bis_cnt;
+
+ /** Number of SubEvents */
+ uint8_t nse;
+
+ /** ISO Interval */
+ uint16_t iso_interval;
+
+ /** Burst Number */
+ uint8_t bn;
+
+ /** Pre-Transmission Offset */
+ uint8_t pto;
+
+ /** Immediate Repetition Count */
+ uint8_t irc;
+
+ /** Maximum PDU size */
+ uint16_t max_pdu;
+
+ /** Service Data Unit Interval */
+ uint8_t sdu_interval[3];
+
+ /** Maximum SDU size */
+ uint16_t max_sdu;
+
+ /** Advertising PHY */
+ uint8_t phy;
Review Comment:
this is big phy, not advertising phy
##########
nimble/host/src/ble_hs_hci_evt.c:
##########
@@ -128,6 +131,9 @@ static ble_hs_hci_evt_le_fn * const ble_hs_hci_evt_le_dispatch[] = {
[BLE_HCI_LE_SUBEV_ADV_SET_TERMINATED] = ble_hs_hci_evt_le_adv_set_terminated,
[BLE_HCI_LE_SUBEV_SCAN_REQ_RCVD] = ble_hs_hci_evt_le_scan_req_rcvd,
[BLE_HCI_LE_SUBEV_PERIODIC_ADV_SYNC_TRANSFER] = ble_hs_hci_evt_le_periodic_adv_sync_transfer,
+#if MYNEWT_VAL(BLE_BIGINFO_REPORTS)
+ [BLE_HCI_LE_SUBEV_BIGINFO_ADV_REPORT] = ble_hs_hci_evt_le_biginfo_adv_rpt,
Review Comment:
`...adv_report`
##########
nimble/host/src/ble_hs_startup.c:
##########
@@ -261,6 +261,16 @@ ble_hs_startup_le_set_evmask_tx(void)
}
#endif
+#if MYNEWT_VAL(BLE_BIGINFO_REPORTS)
+ if (version >= BLE_HCI_VER_BCS_5_2) {
+ /**
+ * Enable the following LE events:
+ * 0x0000000200000000 LE Periodic Advertising Sync Transfer Received event
Review Comment:
this is not the proper event (at least according to description)
##########
nimble/host/src/ble_gap.c:
##########
@@ -1849,6 +1849,50 @@ ble_gap_rx_periodic_adv_sync_transfer(const struct ble_hci_ev_le_subev_periodic_
ble_hs_unlock();
+ cb(&event, cb_arg);
+}
+#endif
+
+#if MYNEWT_VAL(BLE_BIGINFO_REPORTS)
+void
+ble_gap_rx_biginfo_adv_rpt(const struct ble_hci_ev_le_subev_biginfo_adv_report *ev)
+{
+ struct ble_hs_periodic_sync *psync;
+ struct ble_gap_event event;
+ ble_gap_event_fn *cb;
+ void *cb_arg;
+
+ ble_hs_lock();
+ psync = ble_hs_periodic_sync_find_by_handle(le16toh(ev->sync_handle));
+ if (psync) {
+ cb = psync->cb;
+ cb_arg = psync->cb_arg;
+ }
+ ble_hs_unlock();
+
+ if (!psync || !cb) {
Review Comment:
it does not make sense to return if `!cb` since the same condition is checked below
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] andrzej-kaczmarek merged pull request #1507: Add BIGInfo reports to controller and host
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek merged PR #1507:
URL: https://github.com/apache/mynewt-nimble/pull/1507
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] andrzej-kaczmarek commented on a diff in pull request #1507: Add BIGInfo reports to controller and host
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #1507:
URL: https://github.com/apache/mynewt-nimble/pull/1507#discussion_r1209910285
##########
nimble/host/src/ble_gap.c:
##########
@@ -1849,6 +1849,47 @@ ble_gap_rx_periodic_adv_sync_transfer(const struct ble_hci_ev_le_subev_periodic_
ble_hs_unlock();
+ cb(&event, cb_arg);
+}
+#endif
+
+#if MYNEWT_VAL(BLE_BIGINFO_REPORTS)
+void
+ble_gap_rx_biginfo_adv_rpt(const struct ble_hci_ev_le_subev_biginfo_adv_report *ev)
+{
+ struct ble_hs_periodic_sync *psync;
+ struct ble_gap_event event;
+ ble_gap_event_fn *cb;
+ void *cb_arg;
+
+ cb = NULL;
+ cb_arg = NULL;
+
+ ble_hs_lock();
+ psync = ble_hs_periodic_sync_find_by_handle(le16toh(ev->sync_handle));
+ if (psync) {
+ cb = psync->cb;
+ cb_arg = psync->cb_arg;
+ }
+ ble_hs_unlock();
+
+ memset(&event, 0, sizeof event);
+
+ event.type = BLE_GAP_EVENT_BIGINFO_REPORT;
+ event.biginfo_report.sync_handle = ev->sync_handle;
+ event.biginfo_report.bis_cnt = ev->bis_cnt;
+ event.biginfo_report.nse = ev->nse;
+ event.biginfo_report.iso_interval = ev->iso_interval;
+ event.biginfo_report.bn = ev->bn;
+ event.biginfo_report.pto = ev->pto;
+ event.biginfo_report.irc = ev->irc;
+ event.biginfo_report.max_pdu = ev->max_pdu;
+ event.biginfo_report.sdu_interval = get_le32(&ev->sdu_interval[0]) & 0x00FFFFFF;
Review Comment:
get_le24
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -661,6 +669,72 @@ ble_ll_sync_send_truncated_per_adv_rpt(struct ble_ll_sync_sm *sm, uint8_t *evbuf
ble_ll_hci_event_send(hci_ev);
}
+#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS)
+static void
+ble_ll_sync_parse_biginfo_adv_data_to_ev(struct ble_hci_ev_le_subev_biginfo_adv_report *ev, const uint8_t *acad, uint8_t biginfo_acad_offset)
Review Comment:
ble_ll_sync_parse_biginfo_to_ev
also parameters should be biginfo pointer and length, it does not make sense to pass acad and offset and calculate biginfo pointer over and over again
##########
nimble/controller/syscfg.yml:
##########
@@ -323,6 +323,12 @@ syscfg.defs:
Advertising Sync Transfer Feature.
value: MYNEWT_VAL(BLE_PERIODIC_ADV_SYNC_TRANSFER)
+ BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS:
Review Comment:
`BLE_LL_PERIODIC_ADV_SYNC_BIGINFO_REPORTS`
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -117,6 +122,10 @@ struct ble_ll_sync_sm {
uint16_t event_cntr_last_received;
uint8_t adv_addr_rpa[6];
#endif
+
+#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS)
+ struct ble_ll_acad_biginfo biginfo;
+#endif
Review Comment:
no need to keep offset in sm, `ble_ll_sync_check_acad` is called from `ble_ll_sync_rx_pkt_in` so it can just return pointer to biginfo and biginfo length
##########
nimble/controller/include/controller/ble_ll.h:
##########
@@ -475,12 +475,38 @@ struct ble_dev_addr
/* ACAD data types */
#define BLE_LL_ACAD_CHANNEL_MAP_UPDATE_IND 0x28
+#define BLE_LL_ACAD_BIGINFO 0x2C
struct ble_ll_acad_channel_map_update_ind {
uint8_t map[5];
uint16_t instant;
} __attribute__((packed));
+struct ble_ll_acad_biginfo {
+ uint16_t big_offset;
+ uint8_t big_offset_units;
+ uint16_t iso_interval;
+ uint8_t num_bis;
+ uint8_t nse;
+ uint8_t bn;
+ uint32_t sub_interval;
+ uint8_t pto;
+ uint32_t bis_spacing;
+ uint8_t irc;
+ uint8_t max_pdu;
+ uint8_t rfu;
+ uint32_t seed_access_addr;
+ uint8_t sdu_interval[3];
+ uint16_t max_sdu;
+ uint16_t base_crc_init;
+ uint8_t ch_m[5];
+ uint8_t phy;
+ uint8_t bis_payload_count[5];
+ uint8_t framing;
+ uint8_t giv[8];
+ uint8_t gskd[16];
+};
Review Comment:
this is not used anywhere
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -52,19 +52,23 @@
#define BLE_LL_SYNC_CNT MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_PERIODIC_ADV_SYNC_CNT)
#define BLE_LL_SYNC_LIST_CNT MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_PERIODIC_ADV_SYNC_LIST_CNT)
-#define BLE_LL_SYNC_SM_FLAG_RESERVED 0x0001
-#define BLE_LL_SYNC_SM_FLAG_ESTABLISHING 0x0002
-#define BLE_LL_SYNC_SM_FLAG_ESTABLISHED 0x0004
-#define BLE_LL_SYNC_SM_FLAG_SET_ANCHOR 0x0008
-#define BLE_LL_SYNC_SM_FLAG_OFFSET_300 0x0010
-#define BLE_LL_SYNC_SM_FLAG_SYNC_INFO 0x0020
-#define BLE_LL_SYNC_SM_FLAG_DISABLED 0x0040
-#define BLE_LL_SYNC_SM_FLAG_ADDR_RESOLVED 0x0080
-#define BLE_LL_SYNC_SM_FLAG_HCI_TRUNCATED 0x0100
-#define BLE_LL_SYNC_SM_FLAG_NEW_CHANMAP 0x0200
-#define BLE_LL_SYNC_SM_FLAG_CHAIN 0x0400
-
-#define BLE_LL_SYNC_ITVL_USECS 1250
+#define BLE_LL_SYNC_SM_FLAG_RESERVED 0x0001
+#define BLE_LL_SYNC_SM_FLAG_ESTABLISHING 0x0002
+#define BLE_LL_SYNC_SM_FLAG_ESTABLISHED 0x0004
+#define BLE_LL_SYNC_SM_FLAG_SET_ANCHOR 0x0008
+#define BLE_LL_SYNC_SM_FLAG_OFFSET_300 0x0010
+#define BLE_LL_SYNC_SM_FLAG_SYNC_INFO 0x0020
+#define BLE_LL_SYNC_SM_FLAG_DISABLED 0x0040
+#define BLE_LL_SYNC_SM_FLAG_ADDR_RESOLVED 0x0080
+#define BLE_LL_SYNC_SM_FLAG_HCI_TRUNCATED 0x0100
+#define BLE_LL_SYNC_SM_FLAG_NEW_CHANMAP 0x0200
+#define BLE_LL_SYNC_SM_FLAG_CHAIN 0x0400
+
+#define BLE_LL_SYNC_ITVL_USECS 1250
Review Comment:
do not reformat code unnecesarilly
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -661,6 +669,72 @@ ble_ll_sync_send_truncated_per_adv_rpt(struct ble_ll_sync_sm *sm, uint8_t *evbuf
ble_ll_hci_event_send(hci_ev);
}
+#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS)
+static void
+ble_ll_sync_parse_biginfo_adv_data_to_ev(struct ble_hci_ev_le_subev_biginfo_adv_report *ev, const uint8_t *acad, uint8_t biginfo_acad_offset)
+{
+ uint8_t biginfo_len;
+ uint32_t fields_buf;
+
+ biginfo_len = acad[biginfo_acad_offset] - 1;
+
+ fields_buf = get_le32(&acad[biginfo_acad_offset + 2]);
+ ev->iso_interval = (fields_buf >> 15) & 0x0FFF;
+ ev->bis_cnt = (fields_buf >> 27) & 0x1F;
+
+ fields_buf = get_le32(&acad[biginfo_acad_offset + 6]);
+ ev->nse = fields_buf & 0x1F;
+ ev->bn = (fields_buf >> 5) & 0x07;
+ ev->pto = (fields_buf >> 28) & 0x0F;
+
+ fields_buf = get_le32(&acad[biginfo_acad_offset + 10]);
+ ev->irc = (fields_buf >> 20) & 0x0F;
+ ev->max_pdu = (fields_buf >> 24) & 0xFF;
+
+ fields_buf = get_le32(&acad[biginfo_acad_offset + 19]);
+ ev->sdu_interval[0] = fields_buf & 0xFF;
+ ev->sdu_interval[1] = (fields_buf >> 8) & 0xFF;
+ ev->sdu_interval[2] = (fields_buf >> 16) & 0x0F;
+ ev->max_sdu = (fields_buf >> 20) & 0x0FFF;
+
+ ev->phy = (acad[biginfo_acad_offset + 29] >> 5) & 0x07;
+
+ ev->framing = (acad[biginfo_acad_offset + 34] >> 7) & 0x01;
+
+ if (biginfo_len == BLE_LL_SYNC_BIGINFO_LEN_ENC) {
+ ev->encryption = 1;
+ } else {
+ ev->encryption = 0;
+ }
+}
+
+static void
+ble_ll_sync_send_biginfo_adv_rpt(struct ble_ll_sync_sm *sm, const uint8_t *acad)
Review Comment:
`biginfo` and `biginfo_len` instead of `acad`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] andrzej-kaczmarek commented on a diff in pull request #1507: Add BIGInfo reports to controller and host
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #1507:
URL: https://github.com/apache/mynewt-nimble/pull/1507#discussion_r1206382324
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -1009,6 +1134,21 @@ ble_ll_sync_check_acad(struct ble_ll_sync_sm *sm,
sm->chan_map_new_instant = le16toh(chmu->instant);
sm->flags |= BLE_LL_SYNC_SM_FLAG_NEW_CHANMAP;
break;
+
+#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS)
+ case BLE_LL_ACAD_BIGINFO:
+ if (ad_len - 1 == BLE_LL_SYNC_BIGINFO_LEN) {
+ ble_ll_sync_parse_biginfo(&acad[2], 0);
+ sm->flags |= BLE_LE_SYNC_SM_FLAG_BIGINFO;
+ } else if (ad_len - 1 == BLE_LL_SYNC_BIGINFO_LEN_ENC) {
+ ble_ll_sync_parse_biginfo(&acad[2], 1);
+ sm->flags |= BLE_LE_SYNC_SM_FLAG_BIGINFO;
+ } else {
+ return false;
+ }
Review Comment:
you can just pass pointer and length to function, no need to check for encryption here
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -117,6 +122,10 @@ struct ble_ll_sync_sm {
uint16_t event_cntr_last_received;
uint8_t adv_addr_rpa[6];
#endif
+
+#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS)
+ struct ble_ll_acad_biginfo biginfo;
+#endif
Review Comment:
there's no need to keep this huge structure in sm all the time. all you need is likely to check if biginfo is present in acad, store the offset and then parse it directly to hci event.
for now I think it will be enough to return that offset from `ble_ll_sync_check_acad` and use it in `ble_ll_sync_rx_pkt_in` to check whether to send biginfo report
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -661,6 +670,44 @@ ble_ll_sync_send_truncated_per_adv_rpt(struct ble_ll_sync_sm *sm, uint8_t *evbuf
ble_ll_hci_event_send(hci_ev);
}
+#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS)
+static void
+ble_ll_sync_send_biginfo_adv_rpt(struct ble_ll_sync_sm *sm)
+{
+ struct ble_hci_ev_le_subev_biginfo_adv_report *ev;
+ struct ble_hci_ev *hci_ev;
+
+ if (!ble_ll_hci_is_le_event_enabled(BLE_HCI_LE_SUBEV_BIGINFO_ADV_REPORT)) {
+ return;
+ }
+
+ hci_ev = ble_transport_alloc_evt(1);
+ if (!hci_ev) {
+ assert(0);
Review Comment:
no need to assert here, we'll just skip that event
##########
nimble/controller/src/ble_ll_sync.c:
##########
@@ -1117,6 +1257,14 @@ ble_ll_sync_rx_pkt_in(struct os_mbuf *rxpdu, struct ble_mbuf_hdr *hdr)
/* send reports from this PDU */
ble_ll_sync_send_per_adv_rpt(sm, rxpdu, hdr->rxinfo.rssi, tx_power,
datalen, aux, aux_scheduled);
+
+#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_BIGINFO_REPORTS)
+ /* send biginfo only if it is present and at least one per_adv_rpt event was sent */
+ if ((sm->flags & BLE_LE_SYNC_SM_FLAG_BIGINFO) &&
+ !(sm->flags & BLE_LL_SYNC_SM_FLAG_HCI_NO_DATA_SENT)) {
+ ble_ll_sync_send_biginfo_adv_rpt(sm);
+ }
Review Comment:
you just send biginfo report if it's present in current PDU, no need to use additional flags
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org