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

windows/bluetooth: Add support for nimble BLE to windows port. #7781

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

Conversation

andrewleech
Copy link
Sponsor Contributor

@andrewleech andrewleech commented Sep 13, 2021

This was developed in conjunction with #7780 to provide support for nimble BLE on windows to the unix port.

This allows a usb/serial HCI controller to be used on windows to test and run bluetooth micropython code.

This has only been added to minwg / make based builds at this stage. The MSVC project has not been updated to match.
I suspect nimble builds may not be possible with MSVC due to a struct size issue that took quite a long time to debug.

Nimble uses struct size to test whether the correct HCI packet has been received, eg.

struct ble_hci_ev_command_complete {
    uint8_t  num_packets;
    uint16_t opcode;
    uint8_t  status;
    uint8_t  return_params[0];
} __attribute__((packed));

...

ble_hs_hci_rx_cmd_complete(const void *data, int len,
                           struct ble_hs_hci_ack *out_ack)
{
    const struct ble_hci_ev_command_complete *ev = data;
    const struct ble_hci_ev_command_complete_nop *nop = data;
    uint16_t opcode;

    if (len < sizeof(*ev)) {
        if (len < sizeof(*nop)) {
            return BLE_HS_ECONTROLLER;
        }
...

On linux/unix/arm builds sizeof(struct ble_hci_ev_command_complete) == 4 as the uint8_t return_params[0]; is essentially ignored.

On MSVC apparently this null array is apparently included as size 1, eg. sizeof(struct ble_hci_ev_command_complete) == 5 - and by default mingw matches MSVC behavior on this. This was causing the code above to return BLE_HS_ECONTROLLER error instead of correctly processing the hci packet.

I had to add the compiler flag -mno-ms-bitfields to make mingw match "regular" gcc behavior - I don't know if a similar compile flag exists to make MSVC match gcc packed struct size - let alone whether the rest of the nimble codebase is at all compilable under msvc.

@codecov-commenter
Copy link

Codecov Report

Merging #7781 (4ff0f9b) into master (0a51073) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7781   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files         154      154           
  Lines       20040    20040           
=======================================
  Hits        19689    19689           
  Misses        351      351           
Impacted Files Coverage Δ
py/objlist.c 99.23% <0.00%> (-0.39%) ⬇️
py/parse.c 99.00% <0.00%> (-0.20%) ⬇️
py/runtime.c 99.24% <0.00%> (-0.16%) ⬇️
py/obj.c 97.61% <0.00%> (+0.79%) ⬆️
py/bc.c 89.69% <0.00%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a51073...4ff0f9b. Read the comment docs.

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

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.

None yet

4 participants