sensortile_box_pro board: add sample to upgrade BlueNRG SoC firmware #74255
Conversation
avisconti
added
area: Boards
area: Samples
zephyrbot
requested review from
ajarmouni-st,
FRASTM,
gautierg-st,
GeorgeCGV,
kartben,
marwaiehm-st and
nashif
| BlueNRG-LP chip | ||
| =============== | ||
|
|
||
| The board is equipped with a ST Microelectronics `BlueNRG-LP`_ chip. Before using this |
There was a problem hiding this comment.
| The board is equipped with a ST Microelectronics `BlueNRG-LP`_ chip. Before using this | |
| The board is equipped with an ST Microelectronics `BlueNRG-LP`_ chip. Before using this |
There was a problem hiding this comment.
Sorry, I missed it. I'll review your inputs carefully tomorrow.
There was a problem hiding this comment.
"STMicroelectronics" is attached, no!?
There was a problem hiding this comment.
"STMicroelectronics" is attached, no!?
Oh yes. Thanks for noticing it. I'll correct it
| =============== | ||
|
|
||
| The board is equipped with a ST Microelectronics `BlueNRG-LP`_ chip. Before using this | ||
| functionality on zephyr a chip f/w upgrade may be necessary. Please follow the information |
There was a problem hiding this comment.
| functionality on zephyr a chip f/w upgrade may be necessary. Please follow the information | |
| functionality on Zephyr, a firmware upgrade of the chip may be necessary. Please follow the information |
| .. _sensortile_box_pro_sample_ble_fwupg: | ||
|
|
||
| ST SensorTile.box Pro BLE Firwmare Upgrade |
There was a problem hiding this comment.
I would recommend using .. zephyr:code-sample:: directive, see other samples using it for ... well... examples :D
|
|
||
| The board is equipped with a ST Microelectronics `BlueNRG-LP`_ chip. Before using this | ||
| functionality on zephyr a chip f/w upgrade may be necessary. Please follow the information | ||
| on :ref:`sensortile_box_pro_sample_ble_fwupg` sample. |
There was a problem hiding this comment.
use :zephyr:code-sample: to reference once you've update the sample's readme to use the code-sample directive
|
|
||
| Replace :code:`<tty_device>` with the correct device path automatically created on | ||
| the host after the SensorTile.box Pro board gets connected to it, | ||
| usually :code:`/dev/ttyUSBx` or :code:`/dev/ttyACMx` (with x being 0, 1, 2, ...). |
There was a problem hiding this comment.
random sphinx trivia, you may make it look nicer using the below syntax:
| usually :code:`/dev/ttyUSBx` or :code:`/dev/ttyACMx` (with x being 0, 1, 2, ...). | |
| usually :samp:`/dev/ttyUSB{x}` or :samp:`/dev/ttyACM{x}` (with x being 0, 1, 2, ...). |
https://www.sphinx-doc.org/en/master/usage/restructuredtext/roles.html#role-samp
There was a problem hiding this comment.
@kartben Can you possibly take a look again after all the changes? Thanks
I saw that infineon is doing something similar, but they are using the Bluetooth firmware blob in the driver itself (brcm_patchram_buf). I guess that using it in drivers would be ok, right? |
Errr, you what? Can you post a link? I had a look and it doesn't have the blob in the driver, you need to get the blob using Though note samples are expected to build and pass in CI and there can be no blobs there, so I would say this sample is not acceptable for inclusion even if the blob is moved out as it can never be built in CI |
Not sure I got your point, my bad. |
Why do you have a .bin file in this commit? |
Ops, I think it was left there by mistake. Thanks for catching it. :( |
avisconti
force-pushed
the
add-stile-ble-doc
branch
from
765b008 to
3ae0469
Compare
avisconti
force-pushed
the
add-stile-ble-doc
branch
from
3ae0469 to
5cc6d59
Compare
Should be alright, went through TSC and approved. |
| switch (status) { | ||
| case BLE_FW_UPG_START: | ||
| for (i = 0; i < 7; i++) { | ||
| gpio_pin_toggle(blue_gpio.port, blue_gpio.pin); |
| { | ||
| /* led 0 */ | ||
| if (!gpio_is_ready_dt(&green_gpio)) { | ||
| LOG_ERR("%s: device not ready.\n", green_gpio.port->name); |
There was a problem hiding this comment.
LOG_... calls don't need the explicit \n, also this file mixes printk and LOG, I think you should stick with one or the other, if you enable logging just use LOG everywhere
| static int ble_bl_cmd_mass_erase(const struct device *bl_uart) | ||
| { | ||
| if (ble_uart_send_cmd(bl_uart, BLE_BL_CMD_ERASE) < 0) { | ||
| return -1; |
There was a problem hiding this comment.
how about having a temporary return variable, logging the error message and returning the actual code? same in the other cases, not much point keeping it ultra compact, if something fails it'll be a pain to troubleshoot
| } | ||
|
|
||
| /* send address */ | ||
| if (ble_uart_send_data(bl_uart, (uint8_t *)&address_be, 4) < 0) { |
The blob was approved, this PR was not, would like @nashif to comment on acceptability of this, this sample requires a blob to build and cannot be run on zephyr's CI at all, unlike other samples where the blob is optional for some boards, here it is a requirement |
I understand your point despite the fact that there are already other samples (very few to be honest) which skip the CI. But I understand your point. The problem is that in order to increase the usability of the sensortile_box_pro board, in this case the ble usage, a method to update the ble chip f/w is necessary. Maybe a sample is not the answer, maybe there are other ways. So, instead of a "you cannot do that" approach, I would rather appreciate some suggestions on an alternative way to perform the upgrade. Please note that this issue is common to other ST boards as well, so the solution that will come up on this PR will be probably generally adopted. Maybe @nashif could really give his opinion on that. |
|
There was a conversation about this some time ago, was about a vendor specific radio protocol that required a blob to build. IIRC the sample got blocked on the basis on not being buildable in CI since CI does not fetch any blobs. I think it's a bit of a weak argument though because the same can be said for most bluetooth HCI drivers, most of the ones that live in https://github.com/zephyrproject-rtos/zephyr/tree/main/drivers/bluetooth/hci can't built without blobs and no CI target builds them. |
Yeah, exactly. For example at least the Infineon cyw208xx ble driver. This is the one I took a look at when I started to explore the way to do it. This driver should have the same CI issue I'm running into I guess... |
This is a sample, this is not a driver. Blobs are hardware enablement, not sample enablement. The corresponding sample is https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/bluetooth/hci_ipc which can and is built without blobs in CI
From my point of view it should be done downstream, out of the zephyr repo |
ok, I'll try to investigate in that direction then. |
What if the loader code was implemented as a |
But I guess it would have same blob/ci issue, isn't it? |
Sure but no one seems to be concerned with that for ble hci drivers... Or maybe you could have a sample Kconfig option enabled by the test run that replaces the blob with dummy data so that the sample can be compiled? |
Well Fabio, I'm actually exploring the possibility to prepare an external zephyr application to accomplish this task. It is basically the same code of this sample, but replacing the blobs part which is not needed. It will require a bit of extra explanations in order to be used, but seems to me the best solution. |
|
Sure I just find it incoherent that we are so worried about blobs in sample code and not worried at all about blobs in subsystem code. |
@fabiobaltieri I had an internal conversation with someone here in STM that was basically proposing similar approach. It should be very easy to do such modifications and it won't affect CI, but I'm not sure that it would be accepted. Something like And then user would build the sample with -DCONFIG_BLOB_ENABLED=y Would that be approved? |
Well it should be the opposite IMO, the default behavior uses the blob (so the user can use it as is) but the configuration used in CI (i.e. what's in sample.yaml) enables the dummy blob array thing. But yeah apart from that I think it would be fine, certainly worth trying anyway. Just to be sure: so this is really not something one may want to compile in as part of their application? Just a standalone sample and that's it? |
It makes sense, thx.
Yes, exactly. My feeling is that it won't be accepted anyway, as the real issue seems not to be the CI, but the fact that sample concept should not be used to convey a functionality, but to make examples. Let's see. |
This sample implement the ble firmware upgrade using the AN5471 protocol on uart2 channel. Signed-off-by: Armando Visconti <armando.visconti@st.com>
Extend sensortile_box_pro document adding BlueNRG-LP chip description. Signed-off-by: Armando Visconti <armando.visconti@st.com>
avisconti
force-pushed
the
add-stile-ble-doc
branch
from
b52b881 to
fab9d07
Compare
Since the mkbox board ble f/w upgrade method based on binary blobs that was proposed in zephyrproject-rtos#74255 has been turned down, all the work done in hal_stm32 module to host blobs is now reverted. Signed-off-by: Armando Visconti <armando.visconti@st.com>
|
This PR has not been accepted and is going to be closed down. |
Since the mkbox board ble f/w upgrade method based on binary blobs that was proposed in zephyrproject-rtos#74255 has been turned down, all the work done in hal_stm32 module to host blobs is now reverted. Signed-off-by: Armando Visconti <armando.visconti@st.com>
Since the mkbox board ble f/w upgrade method based on binary blobs that was proposed in #74255 has been turned down, all the work done in hal_stm32 module to host blobs is now reverted. Signed-off-by: Armando Visconti <armando.visconti@st.com>
Since the mkbox board ble f/w upgrade method based on binary blobs that was proposed in zephyrproject-rtos#74255 has been turned down, all the work done in hal_stm32 module to host blobs is now reverted. Signed-off-by: Armando Visconti <armando.visconti@st.com>
Add sample that implement the BlueNRG SoC firmware upgrade using the AN5471 protocol on uart2 channel.
It uses blobs that must be fetched with:
west blobs fetch stm32
Use of blobs approved by TSC (#74326)
(The CI for the sample is skipped because it requires the presence of a f/w blob that is not fetched by CI itself, as mentioned in https://docs.zephyrproject.org/latest/contribute/bin_blobs.html#support-and-maintenance)