Navigation Menu
Conversation
|
Several minutes ago I saw a commit that could be used to replace the init_mp_state_thread() function in "ports/unix/mpbtstackport_usb.c": |
|
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14006 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 164 164
Lines 21345 21345
=======================================
Hits 21040 21040
Misses 305 305 ☔ View full report in Codecov by Sentry. |
|
The first three commits #29f6e66, #7acadbd, #5393f63 are identical with the original commits except that the author has been fixed. |
There was a problem hiding this comment.
Thanks for submitting this PR, @yasnim24.
I was a little confused by the title - the unix port already has support for Bluetooth via libusb, but it's broken on master and this PR fixes it, yes? The only new functionality is the HCI dump option.
I think the changes look reasonable to me. We should probably add a bluetooth variant (or two) to the unix port and build them in CI, though, to catch any future bitrot of this kind. You don't have to do that part unless you're keen, though.
ports/unix/Makefile
Outdated
| else | ||
| # Fallback to HCI controller via a H4 UART (e.g. Zephyr on nRF) over a /dev/tty serial port. | ||
| MICROPY_BLUETOOTH_BTSTACK_H4 ?= 1 | ||
| ifndef MICROPY_BLUETOOTH_BTSTACK_HCI_DUMP |
There was a problem hiding this comment.
Please add a comment here about what this value is for, similar to the other config items.
Suggest even moving it up near the top with the other user-settable values.
| # | ||
| # The original file "extmod/btstack/btstack_config_common.h" | ||
| # will be included by it. | ||
| MICROPY_BLUETOOTH_BTSTACK_CONFIG_FILE ?= '"ports/unix/btstack_config.h"' |
There was a problem hiding this comment.
Suggest moving this up before the endif for bluetooth
| endif | ||
|
|
||
| endif | ||
| endif |
There was a problem hiding this comment.
| endif | |
| endif # MICROPY_PY_BLUETOOTH |
(I know the original version doesn't have this, but it makes it all a lot easier to read.)
|
Thank you for your comment, @projectgus. Your suggested changes are OK for me. Can you also further explain what you mean with:
|
Best to rebase, change the commits, and force push to keep clean commit history. Jimmo has pointers published at https://github.com/jimmo/git-and-micropython#README
We have some "variants" for the unix port with different configs: However I don't think any of the variants enable the Bluetooth feature. Therefore, when the code broke ("bitrotted") at some point in the past nobody noticed until now. If we had a variant to build Bluetooth via GitHub Actions (Continuous Integration aka CI) then we would have known that this feature no longer compiled. Don't worry about that for this PR, though. |
There is a code that evaluates "HAVE_LIBUSB" and then sets "MICROPY_BLUETOOTH_BTSTACK_USB" and "MICROPY_BLUETOOTH_BTSTACK_H4" accordingly. That code has been moved before "include $(TOP)/extmod/extmod.mk" to be effective. Details: "include $(TOP)/extmod/extmod.mk" will "include $(TOP)/extmod/btstack/btstack.mk" and that in turn sets "MICROPY_BLUETOOTH_BTSTACK_USB ?= 0" "MICROPY_BLUETOOTH_BTSTACK_H4 ?= 1" Signed-off-by: Yasmin Bosch <development@yasmin-bosch.de>
Added a "ports/unix/btstack_config.h" for a workaround:
If "MICROPY_BLUETOOTH_BTSTACK_USB" is set and "ENABLE_SCO_OVER_HCI"
is not defined there is a compiler error:
../../lib/btstack/platform/libusb/hci_transport_h2_libusb.c:1354:13:
error: ‘signal_sco_can_send_now’ defined but not used
Because the problem is in an external code a workaround is done by
defining MICROPY_BLUETOOTH_BTSTACK_USB in the added "btstack_config.h".
The "ports/unix/Makefile" has been changed so that the "bts
8000
tack_config.h"
will be included.
Signed-off-by: Yasmin Bosch <development@yasmin-bosch.de>
ports/unix/mpbtstackport_usb.c: "lib/btstack/platform/posix/btstack_run_loop_posix.c" is now used instead of "lib/btstack/platform/embedded/btstack_run_loop_embedded.c". Additional code has been added to make it work. Now also a log file "/tmp/hci_dump.pklg" could be activated by setting "MICROPY_BLUETOOTH_BTSTACK_HCI_DUMP=1". The "Makefile" and "mpbtstackport_common.c" has been changed accordingly. Signed-off-by: Yasmin Bosch <development@yasmin-bosch.de>
See 'global: Remove the STATIC macro.': micropython#13763 Signed-off-by: Yasmin Bosch <development@yasmin-bosch.de>
The function 'mp_thread_init_state()' has been added to 'py/runtime.h' with commit #bc424dd in micropython/micropython: 'py/modthread: Move thread state initialisation to shared function.' So use that function instead of defining own function in 'ports/unix/mpbtstackport_usb.c'. Signed-off-by: Yasmin Bosch <development@yasmin-bosch.de>
|
Code size report: |
|
Merged changes as suggested by @projectgus to commits afe2867, 60a5e47. |
ports/unix did not build with
"make MICROPY_PY_BLUETOOTH=1 MICROPY_BLUETOOTH_BTSTACK=1".
This PR fixes it. If libusb-1.0 exists on the machine "MICROPY_BLUETOOTH_BTSTACK_USB=1" is
automatically set and Bluetooth over USB should work.
Now also a log file "/tmp/hci_dump.pklg" could be activated by setting
"MICROPY_BLUETOOTH_BTSTACK_HCI_DUMP=1".