extmod/btstack: Schedule ops for background completion. #6159

jimmo · 2020-06-17T06:30:33Z

The goal of this PR is to allow using ble.gatts_notify() at any time, even if the stack is not ready to send the notification right now.

This also addresses the same issue for ble.gatts_indicate and ble.gattc_write (without response). The result is that you can now:

Call notify/indicate at any time.
Call write-without-response while another one is in progress. (But no additional ones can queue, and will fail with EALREADY)

Unfortunately the three cases notify/indicate, write-with-response and write-without-response all require slightly different implementations.

This PR also addresses two other issues:

The buffer passed to write-with-response wasn't copied, meaning it could be modified by the caller, affecting the in-progress write.
The buffer wasn't tracked to prevent the GC free'ing it.

dpgeorge · 2020-06-18T11:53:16Z

ports/unix/mpthreadport.c

@@ -65,7 +65,8 @@ STATIC pthread_key_t tls_key;
 // The mutex is used for any code in this port that needs to be thread safe.
 // Specifically for thread management, access to the linked list is one example.
 // But also, e.g. scheduler state.
-STATIC pthread_mutex_t thread_mutex = PTHREAD_MUTEX_INITIALIZER;
+STATIC pthread_mutex_t thread_mutex;
+STATIC pthread_mutexattr_t thread_mutex_attr;


Does this attr struct need to be in global data? Can't it be temporary, on the stack in mp_thread_init()? Is there any reference that says either way?

Looking at the implementation, yes, seems fine to put it on the stack. Also, there's an example of doing exactly that in my copy of APUE.

dpgeorge · 2020-06-18T12:02:03Z

extmod/modbluetooth.c

-    }
+    if (entry) {
+        if (value_len > entry->data_alloc) {
+            entry->data = m_new(uint8_t, value_len);


This may raise an exception which would leave the function without releasing the bluetooth lock (yes this would be much clearer without NLR...). I think it can be fixed pretty easily by using m_new_maybe() and then explicitly raising an OOM error after releasing the lock. Or just returning MP_ENOMEM. But the latter requires all callers to check the return value if they can't guarantee the write won't need to allocate.

Also, related, do we need to check that all calls of mp_bluetooth_gatts_db_write() do not have the bluetooth lock?

Also, related, do we need to check that all calls of mp_bluetooth_gatts_db_write() do not have the bluetooth lock?

I checked this. It looks ok, the bluetooth lock is only held in sections of code that are "low level" or call into BTstack. Ie there are no calls into the uPy runtime (that could raise).

There are very few callers of these methods, and most already checked the return value. I've made all uses do so.

extmod/modbluetooth.c

extmod/btstack/modbluetooth_btstack.h

extmod/btstack/modbluetooth_btstack.c

dpgeorge · 2020-06-18T12:56:58Z

extmod/btstack/modbluetooth_btstack.c

+    }
+    assert(btstack_linked_list_remove(&MP_STATE_PORT(bluetooth_btstack_root_pointers)->pending_ops, (btstack_linked_item_t*)pending_op));
+    MICROPY_PY_BLUETOOTH_EXIT
+}


Is it possible to explicitly m_del() the pending_op->buf here, because we were responsible for allocating it and know it'll never be used again?

The buf is inline in the pending_op struct. But we can del the whole op.

I've split "remove (and optionally del)" from "find a pending op". Anywhere that attempts to unref a pending op, that isn't in interrupt context, now also frees it.

extmod/btstack/modbluetooth_btstack.c

dpgeorge · 2020-06-18T13:00:18Z

extmod/btstack/modbluetooth_btstack.c

+            DEBUG_EVENT_printf("btstack_finish_pending_operation: found value_handle=%d len=%lu\n", pending_op->value_handle, pending_op->len);
+            assert(btstack_linked_list_remove(&MP_STATE_PORT(bluetooth_btstack_root_pointers)->pending_ops, (btstack_linked_item_t*)pending_op));
+            MICROPY_PY_BLUETOOTH_EXIT
+            return pending_op;


as above, is it possible to explicitly free the buf before returning?

The caller of this function needs it (e.g. event_type == GATT_EVENT_CAN_WRITE_WITHOUT_RESPONSE in the packet handler). But all the callers can free it.

extmod/btstack/modbluetooth_btstack.c

dpgeorge · 2020-06-18T13:09:34Z

extmod/btstack/modbluetooth_btstack.c

+
+        att_server_request_to_send_notification(&pending_op->context_registration, conn_handle);
+        // We don't know how many bytes will be sent.
+        *value_len = 0;


hmm, returning 0 here means the caller always needs to test for this value to know if the notify was sent immediately or is pending... but there's really no other way to do it. But the docs don't actually specify any return value for gatts_notify() so maybe it should just return None in all cases?

Yes, this was never reliably used anywhere. I've made gatts_notify always return None as suggested.

dpgeorge · 2020-06-18T13:10:38Z

extmod/btstack/modbluetooth_btstack.c

+        return 0;
+    } else {
+        return btstack_error_to_errno(err);
+    }
 }

 int mp_bluetooth_gatts_indicate(uint16_t conn_handle, uint16_t value_handle) {


I just noticed: it doesn't look like this function is ever used??

I've added gatts_indicate(conn_handle, value_handle) to modbluetooth. (And added to the ble_characteristic.py multitest)

extmod/btstack/modbluetooth_btstack.c

dpgeorge · 2020-06-18T13:22:32Z

Call notify/indicate at any time.

Does this mean you can queue an arbitrary number of notify/indicate data packets?

This mutex is used to make the unix port behave more like bare metal (i.e. it allows "IRQ handlers" to run exclusively). In order to match the bare-metal behavior of MICROPY_BEGIN_ATOMIC_SECTION, this mutex needs to be recursive.

gatts_notify/indicate will now run in the background if the ACL buffer is currently full. gattc_write(mode=0) (no response) will now allow for one outstanding write. gattc_write(mode=1) (with response) will now copy the buffer so that it can't be modified by the caller. All four paths also now track the buffer while the op is in progress.

jimmo · 2020-07-14T07:27:16Z

Added fixes as a new commit so they can be reviewed independently. Will rebase and squash into the other commits after review.

dpgeorge · 2020-07-16T14:17:09Z

Thanks for updating, it all looks very good. Only minor thing I noticed is that the new gatts_indicate(...) function needs to be added to the docs.

Also clarify behavior of `gatts_notify` and add some TODOs about adding an event for indication acknowledgement.

dpgeorge · 2020-07-18T04:38:19Z

Thanks for adding docs. Now rebased and merged in 5d0be97 through b769884

jimmo force-pushed the btstack-notify-write-busy branch from 93fb814 to bbcb4bf Compare June 17, 2020 06:52

dpgeorge added the extmod Relates to extmod/ directory in source label Jun 18, 2020