Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unix: Fix support for Bluetooth via libusb, add HCI dump feature #14006

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yasnim24
Copy link

@yasnim24 yasnim24 commented Mar 2, 2024

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".

@yasnim24
Copy link
Author

yasnim24 commented Mar 2, 2024

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":
commit bc424dd: "py/modthread: Move thread state initialisation to shared function."

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (0e490b7) to head (44acdbf).

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.
📢 Have feedback on the report? Share it here.

@yasnim24
Copy link
Author

yasnim24 commented Mar 17, 2024

The first three commits #29f6e66, #7acadbd, #5393f63 are identical with the original commits except that the author has been fixed.
The other two commits #b4c69d8 and #0e75c12 are new.

@projectgus projectgus self-requested a review September 11, 2024 04:08
@projectgus projectgus changed the title Make ports/unix with libusb support work. ports/unix: Add support for Bluetooth via libusb Sep 11, 2024
@projectgus projectgus changed the title ports/unix: Add support for Bluetooth via libusb unix: Add support for Bluetooth via libusb Sep 11, 2024
Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Contributor

@projectgus projectgus Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest moving this up before the endif for bluetooth

endif

endif
endif
Copy link
Contributor

@projectgus projectgus Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
endif
endif # MICROPY_PY_BLUETOOTH

(I know the original version doesn't have this, but it makes it all a lot easier to read.)

@projectgus projectgus changed the title unix: Add support for Bluetooth via libusb unix: Fix support for Bluetooth via libusb, add HCI dump feature Oct 8, 2024
@projectgus projectgus requested a review from jimmo October 8, 2024 06:42
@yasnim24
Copy link
Author

Thank you for your comment, @projectgus.

Your suggested changes are OK for me.
But I'm not sure what to do. Should I add new commits for the changes? Or do I have to change the original commits and do a force push?

Can you also further explain what you mean with:

... 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. ...

@projectgus
Copy link
Contributor

But I'm not sure what to do. Should I add new commits for the changes? Or do I have to change the original commits and do a force push?

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

Can you also further explain what you mean with:

... 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. ...

We have some "variants" for the unix port with different configs:
https://github.com/micropython/micropython/tree/master/ports/unix/variants

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 "btstack_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>
Copy link

github-actions bot commented Nov 1, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@yasnim24
Copy link
Author

yasnim24 commented Nov 1, 2024

Merged changes as suggested by @projectgus to commits afe2867, 60a5e47.

@projectgus projectgus self-requested a review February 4, 2025 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants