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

stm32/wb55: Add usb/bluetooth transparent mode to stm32wb55 via native module. #9046

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

Conversation

andrewleech
Copy link
Sponsor Contributor

@andrewleech andrewleech commented Aug 13, 2022

Alternate implementation of #7703 which moves most of the task-specific code to a native module example.

This version has full support for use with STM32CubeMonitor-Rf app. It should still also work as an external usb-cdc-hci radio for use with BLE in the unix port of micropython.

With this micropython compiled and flashed to the WB55 board (currently tested on nucleo) the native module can be compiled and deployed with:

cd examples/natmod/stm32wb55_transparent_vcp
make
mpremote cp transparent_vcp.mpy :

I'm using it from main.py as such:

from pyb import Pin, LED

sw = Pin("SW3", Pin.IN, Pin.PULL_UP)

if sw.value():
    LED(2).on()
    import transparent_vcp
    transparent_vcp.start()

@andrewleech andrewleech force-pushed the wb55-transparent-mode-nm branch 2 times, most recently from 42fe637 to 0abc022 Compare August 13, 2022 02:33
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #9046 (f8fb56a) into master (b336b6b) will not change coverage.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #9046   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files         155      155           
  Lines       20549    20549           
=======================================
  Hits        20241    20241           
  Misses        308      308           
Impacted Files Coverage Δ
py/builtinimport.c 98.94% <ø> (ø)
py/gc.c 99.18% <100.00%> (ø)
py/modmicropython.c 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andrewleech andrewleech force-pushed the wb55-transparent-mode-nm branch 3 times, most recently from 3b8232a to 5443323 Compare August 17, 2022 01:22
@andrewleech andrewleech force-pushed the wb55-transparent-mode-nm branch 2 times, most recently from c5d448f to 1455b37 Compare August 17, 2022 05:17
@@ -102,3 +102,11 @@ the second CPU, the RF core.
Execute a HCI command on the SYS channel. The execution is synchronous.

Returns a bytes object with the result of the SYS command.

.. function:: rfcore_ble_hci(ogf, ocf, data) or rfcore_ble_hci(buffer)
Copy link
Member

Choose a reason for hiding this comment

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

Since the first form is not used (yet!) I'd say just implement the second form. It can be enhanced later if anyone ever needs it (BLE HCI commands are arguably different to SYS HCI commands, so I don't think it's a problem having a different function signature here, compared to rfcore_sys_hci).

Also, it could be: rfcore_ble_hci(cmd, outbuf) -> int where outbuf is a bytearray used to store the result, and it returns the number of bytes copied into that buffer. Then this function would be fully in-place (no heap allocation needed). Would you consider implementing that instead? I don't think it would change much (_stm32wb55_transparent.c already has a big 1024 byte buffer which could be reused for the outbuf).

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Thanks @dpgeorge I've removed that second / unused form of the function, this does simplify things.

I've also added the second response_buf arg as optional - it certainly is a worthwhile option to allow no-alloc useage of the function. If the buffer arg isn't supplied I've kept it returing the fully-formed bytes object as this simpler usage could be handy for simple use cases.

I've used this buffer arg for the native module too as suggested, it's working well and cleaner overall.


# Source files (.c or .py)
SRC = \
_stm32wb55_transparent.c \
Copy link
Member

Choose a reason for hiding this comment

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

remove the leading underscore from the name?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Thought it might be useful to differentiate that this is intended to be "internal" as opposed to the .py file being the public interface, like the C accelerator modules on cpython. The implemented / exposed C module no longer matches the filename anyway so yeah the underscore isn't really useful here.

stm32wb55_transparent_vcp.py \


# $(MPY_DIR)/ports/stm32/rfcore.c
Copy link
Member

Choose a reason for hiding this comment

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

remove unused lines

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I've tried to properly clean up all the code / comments now in the latest push thanks.

#include "stm32wbxx_ll_utils.h"
#include "stm32wbxx_ll_system.h"
#include "ports/stm32/mpbthciport.h"
#include "ports/stm32/rfcore.h"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these 2 includes are needed, neither the py/stream.h one. Then a lot of the CFLAGS lines in the Makefile can also be removed.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Ah yes, in earlier versions of this I was building stm32/rfcore.c into the module as well to use some of its functions... until I realised I obviously couldn't access the shared memory buffers this way.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Yep latest push has all of these cleaned up / minimsed and as suggested, the Makefile was able to become much simpler as well.

#include "ports/stm32/mpbthciport.h"
#include "ports/stm32/rfcore.h"

// Based on STM32CubeWB\Projects\P-NUCLEO-WB55.USBDongle\Applications\BLE\BLE_TransparentModeVCP
Copy link
Member

Choose a reason for hiding this comment

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

Are there any licensing/copyright issues to worry about here? Was any code copied?

Copy link
Sponsor Contributor Author

@andrewleech andrewleech Sep 1, 2022

Choose a reason for hiding this comment

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

Thanks for the reminder:

  * Copyright (c) 2021 STMicroelectronics.
  * All rights reserved.
  *
  * This software is licensed under terms that can be found in the LICENSE file
  * in the root directory of this software component.
  * If no LICENSE file comes with this software, it is provided AS-IS.

A couple of folders up I find:
https://github.com/STMicroelectronics/STM32CubeWB/blob/83aacadecbd5136ad3194f39e002ff50a5439ad9/Projects/P-NUCLEO-WB55.USBDongle/Applications/LICENSE.md

SLA0044 Rev5/February 2018

It appears to be a generally permissive licence

Point 5 in particular appears to be a bit sticky:

No use, reproduction or redistribution of this software partially or totally may be done in any manner that would subject this software to any Open Source Terms.

Perhaps I could snip the structs / functions from ST out into a separate file that includes this licence, then the rest of the C code here can be under the normal micropython licence.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

As I suggested in previous comment I've split out all the re-used ST code into a separate C file which references a copy of the matching ST licence file.

While it would certainly be possibly to re-implement the functionality from this file (it's just accessing a bunch of CPU registers really) I think this module is most useful as an example of being able to directly re-use vendor code when suitable functions are provided this way.

Copy link
Member

Choose a reason for hiding this comment

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

Point 5 in particular appears to be a bit sticky

Yes it does. It says the code cannot be used in a way that it would be subject to Open Source Terms, and in particular it mentions the MIT license. I take this to mean that the MIT license cannot be made to apply to this ST code. That makes sense, but I'm not sure if including the code in this repo (which has an MIT license at the top level) violates that point (5).

The code in this file is still under the ST license, we don't intend to change it to MIT.

I'm really not sure here.

Copy link
Member

Choose a reason for hiding this comment

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

If we do go ahead with keeping this code, IMO the license text should just be pasted at the top of the ST file (and not in a separate ST_LICENSE file).

Copy link
Member

Choose a reason for hiding this comment

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

After some discussion with more knowledgeable people on this topic, it's too dangerous to include this code with this license. I think the main intent of this part of the license is to prevent it being incorporated in Linux as a driver, and I think it's safest if we just don't include it in this repo at all.

(That said, don't close this PR just yet!)

Copy link
Sponsor Contributor Author

@andrewleech andrewleech Mar 16, 2023

Choose a reason for hiding this comment

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

Yeah it's a pretty fuzzy distinction going on in that licence. The writing there would definitely block usage within a gpl project as they enforce that everything is gpl compatible. Usage in something mit based might be different as mixed licencing is allowed there... but I definitely don't want to get you into hot water here!

This example would likely be safer split out into a separate repo, acting as a useful guide on how to have out-of-tree C module builds, perhaps then with a handy main.py file to set up the wb55 board/dongle as external radio for Unix micropython ble...

// Don't enable this if stdio is used for transp comms.
#define DEBUG_printf(...) // mp_printf(&mp_plat_print, "rfcore_transp: " __VA_ARGS__)

// TODO Check ble_init_params vs SHCI_C2_BLE_Init /
Copy link
Member

Choose a reason for hiding this comment

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

Can this TODO be removed?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

This was a reminder to me to compare the rfcore / cpu2 init options in

} ble_init_params = {
to the comparable settings in the transparent mode project from ST found (mostly) here: https://github.com/STMicroelectronics/STM32CubeWB/blob/83aacadecbd5136ad3194f39e002ff50a5439ad9/Projects/P-NUCLEO-WB55.USBDongle/Applications/BLE/BLE_TransparentModeVCP/Core/Inc/app_conf.h#L107

While there are differences, most of them are things that are set to 0 in micropython but higher numbers in the examples. These are settings that relate to the Host portion of the stack though, which we're not using from ST. The main discrepancy is MICROPY_HW_RFCORE_BLE_NUM_LINK (1) compared to the examples generally being set to 2. That's a question for a different discussion however, and the ``MICROPY` settings can be overridden for particular projects if needed anyway.

Either way, the TODO is removed now.

mp_obj_t stream_out_o;
const mp_stream_p_t *stream_out_p;

mp_uint_t mpstream_write(const void *buf, size_t len) {
Copy link
Member

Choose a reason for hiding this comment

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

Please call it mp_stream_write, also same for other 2 functions below.

Also I think it might be cleaner to pass in as the first argument the stream object, instead of having global variables.

See eg examples/natmod/btree/btree_c.c:mp_stream_posix_write

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Ah yes, definitely. This was originally used in the callback in parse_hci_info_t parse directly in rfcore.c when I was using that directly. I forgot to unwind all of this once I added the rfcore_ble_hci micropython interface, now that it's always called directly it'll be easy enough to pass the stream through.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Yep this is fixed now making it much cleaner overall.

} else {
// forward packet to rfcore
DEBUG_printf("rfcore_ble_hci_cmd\n");
mp_obj_t cmd = mp_obj_new_bytes(buf, rx);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of create a new bytes object, can use:

mp_obj_array_t cmd = {{&mp_type_bytearray}, BYTEARRAY_TYPECODE, 0, rx, buf};

@@ -762,4 +761,40 @@ STATIC mp_obj_t rfcore_sys_hci(size_t n_args, const mp_obj_t *args) {
}
MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(rfcore_sys_hci_obj, 3, 4, rfcore_sys_hci);

STATIC void rfcore_ble_hci_response_to_buffer(void *env, const uint8_t *buf, size_t len) {
// mp_buffer_info_t *respbufinfo = (mp_buffer_info_t *)env;
Copy link
Member

Choose a reason for hiding this comment

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

please remove this line

mp_int_t ogf = mp_obj_get_int(args[0]);
mp_int_t ocf = mp_obj_get_int(args[1]);
mp_get_buffer_raise(args[2], &bufinfo, MP_BUFFER_READ);
size_t len = tl_ble_hci_cmd_resp(HCI_OPCODE(ogf, ocf), bufinfo.buf, bufinfo.len);
Copy link
Member

Choose a reason for hiding this comment

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

this should be ssize_t

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

This whole section is now removed as it was just to support the other (unused) function branch.

@andrewleech andrewleech force-pushed the wb55-transparent-mode-nm branch 4 times, most recently from 6cc193a to ab7db68 Compare September 6, 2022 07:24
mp_raise_OSError(-len);
}
memcpy(bufinfo.buf, buf, len);
bufinfo.len = len;
Copy link
Member

Choose a reason for hiding this comment

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

this line does nothing


while (true) {
// Sleep briefly to allow micropython background processing.
mp_call_function_n_kw(sleep_ms_obj, 1, 0, (mp_obj_t[]) {MP_OBJ_NEW_SMALL_INT(1)});
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this function should just do one iteration and then return to Python. Then it could, for example, be used with asyncio.

But that's a pretty big change, so I don't expect it to be made. Could be done in the future if anyone wanted to do it.

@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
        rp2:    +0 +0.000% PICO

@dpgeorge
Copy link
Member

I rebased this.

@dpgeorge
Copy link
Member

Steps:

  • fix up comment above about bufinfo.len
  • merge changes to stm32 and docs
  • create a new repo just for the example
  • try to get as many files from Zephyr for the ST specific code
  • copy these needed 3rd-party files directly (ie no concatenating)
  • record the source/git hash of those files

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

RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this pull request Mar 14, 2024
…03-13

update frozen libraries for 9.0.0-rc.1
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

5 participants