Bluetooth: HCI: Use H:4 encoding for buffers

[jhedberg]

This PR changes the encoding of HCI packet types from net_buf metadata into the net_buf payload as a H:4 prefix byte. The bt_buf_get/set_type() APIs are deprecated but kept compatible with the change (i.e. they do net_buf_pull/push_u8() internally).

The PR also removes the experimental H:4 encoding support from the USB transport for several reasons:

• It's non-standard
• It doesn't have any real host-side counter part implementation (even BlueZ upstream doesn't support it)
• It comes with substantial maintenance overhead
• It complicates the HCI raw API needlessly
• It had no CI coverage

Testing status for the drivers. Please check the box once you've tested your respective driver with this PR:

• h4.c @jhedberg /upstream CI
• h5.c @jhedberg /upstream CI
• hci_ambiq.c @aaronyegx
• hci_da1469x.c @andrzej-kaczmarek
• hci_esp32.c @sylvioalves
• hci_ifx_cyw208xx.c @npal-cy
• hci_ifx_psoc6_bless.c @npal-cy
• hci_nxp.c @yeaissa
• hci_silabs_siwx917.c @jhedberg
• hci_silabs_efr32.c @jhedberg
• hci_spi_st.c @HoZHel
• hci_stm32wb0.c @HoZHel
• hci_stm32wba.c @asm5878
• ipc.c @dchat-nordic (probably tested by upstream CI?)
• ipm_stm32wb.c @asm5878
• spi.c @HoZHel
• userchan.c @jhedberg /@aescolar
• USB @jfischer-no
• Native controller: @jhedberg @cvinayak /upstream CI

[jhedberg]

This implements the first item in #88048, which was originally discussed at the recent Bluetooth WG F2F meeting.

[jhedberg]

One positive realization I had when I worked on this change, is that actually most of our HCI drivers already internally used the H:4 encoding. This is the primary reason why the PR ends up removing about 300 lines of code.

[alwa-nordic]

The existing bt_buf_set/get_type functions are kept compatible with the change, except that the get() function can only be called once, since it now pulls the byte from the buffer.

I get that this allows us to be partially backwards compatible, but the name of these functions is misleading now. I think the value of keeping them backwards compatible is worth more than a better name, but there may be a way so the function can be invoked multiple times: Doesn't the H4 type always land in the space net_buf_reserve-ed by buf.c? If so, we can just overwrite that byte if the reserved space is already used. Thoughts?

[alwa-nordic]

I did not know we had packed enums. That's cool. We could potentially use these in hci_types.h instead of plain uint8_t.

[jhedberg]

I did not know we had packed enums. That's cool. We could potentially use these in hci_types.h instead of plain uint8_t.

Sounds like a good idea.

[alwa-nordic]

We need to maintain some level of type information here. Either we should rename the argument to h4_type, or seeing as the enum is packed, we keep using the enum here. I suggest adding BUILD_ASSERT(sizeof(enum bt_buf_type) == sizeof(uint8_t)) in the latter case.

[jhedberg]

We need to maintain some level of type information here. Either we should rename the argument to h4_type, or seeing as the enum is packed, we keep using the enum here. I suggest adding BUILD_ASSERT(sizeof(enum bt_buf_type) == sizeof(uint8_t)) in the latter case.

I think it'd be nice to try to use the enum everywhere, combined with the build assert.

[jhedberg]

I moved from uint8_t to enum bt_buf_type for these APIs.

[alwa-nordic]

Thanks! Looks good!

I have a minor fix that I nack for:

• Bluetooth: HCI: Use H:4 encoding for buffers #88710 (comment)

The other discussions are not hard nacking.

[aaronyegx]

Verified sample/bluetooth/peripheral_hr worked normally on apollo4p_blue_kxr_evb and apollo3_evb with the change.

[alwa-nordic]

Both of the functions have the same caveat, not just the latter.

Can we be more precise here about the caveat? I think we have to mention that these function will now silently corrupt data unless the following assumptions hold.

• bt_buf_get_type(buf) has to be called exactly once before buf is accessed.
• bt_buf_set_type(buf) has to be called exactly once before it's handed to bt_hci_recv_t, with no accesses to buf in between.

Also, this is the migration guide, not the change log, so we need to say how to migrate away from the deprecated behavior. I.e. Verifying the above assumptions and substitute calls to bt_buf_get_type and bt_buf_set_type with net_buf_pull_u8 and net_buf_push_u8.

[jhedberg]

bt_buf_set_type(buf) has to be called exactly once before it's handed to bt_hci_recv_t, with no accesses to buf in between.

HCI drivers don't generally use this API at all, rather they use the various buffer allocation APIs (like bt_buf_get_rx()) which implicitly add the type prefix to the buffer. If you grep under drivers/bluetooth you wont find a single user of it.

[jhedberg]

Both of the functions have the same caveat, not just the latter.

Fixed.

[jhedberg]

@alwa-nordic I did some more thinking about this, and I think hci_types.h is a justifiable place for the packet type prefix byte. I initially felt against this, since it's different from the rest of the HCI payload, which the drivers will just proxy unmodified back-and-forth, whereas the type is something that needs to be separately handled if the underlying transport doesn't use this format. However, while I made the claim that drivers/bluetooth.h should be the only interface that drivers should look at, I failed to consider the fact that most drivers either way have to be aware of the packet type-specific header, in order to determine the packet length, and the type-specific header (event, ACL data, etc) is part of hci_types.h. Drivers currently also need to peek at the event definitions to make the "discardable" vs "not discardable" distinction.

So, I'll take another stab at keeping buf.h (i.e. enum bt_buf_type) backwards compatible. I'm still not quite comfortable with keeping "H4" in the other enumeration name, since "H:4" really is about an entire transport and not just about packet types. I'd rather just strictly document that the values defined based on those values from the transport spec. We don't even have to document it just in reference to H:4, but can say that "the possible values are are commands (1), ACL (2), SCO (3), events (4) and ISO (5) as specified in the UART and 3-Wire UART transport specs" (and extend the reference list to other transports if/when they show up in future specs with the same mapping from value to packet type). So while the documentation makes these references, I think I'd still prefer to keep the enum name generic, i.e. enum bt_hci_pkt_type, however that could be done as a follow-up step and just use our existing H4 defines from hci_types.h for now.

[jhedberg]

I've pushed these changes now. Doesn't look that bad, however there's some inefficiency introduced with the conversions. In some case we even have to do the conversion twice, such as for some bt_hci_raw users like the hci_uart app: For data coming from the host the app first has to convert to enum bt_buf_type to be able to call bt_buf_get_tx(), which in turn internally has convert back to H:4 to be able to prepend the right byte.

[jhedberg]

And I've now pushed a rebased & squashed patch set. I had to simplify some parts in the commit split, since it wasn't (trivially) possible to include all changes cleanly to the original commit set (I basically recreated all commits as far as it made sense).

[jhedberg]

@andrzej-kaczmarek @sylvioalves @npal-cy @yeaissa @HoZHel @asm5878 @dchat-nordic: Could you please take a look at the HCI driver testing checklist in this PR description, and mark each driver as tested once you (or someone else with the respective HW) have tested it with this PR? Thanks!

[yeaissa]

@andrzej-kaczmarek @sylvioalves @npal-cy @yeaissa @HoZHel @asm5878 @dchat-nordic: Could you please take a look at the HCI driver testing checklist in this PR description, and mark each driver as tested once you (or someone else with the respective HW) have tested it with this PR? Thanks!

I checked on NXP platform using hci_nxp.c, no regression noticed. Thanks for the change @jhedberg.
I'm not sure I can make the change on your description. Please go ahead and mark it. Thanks.

[Thalley]

@jhedberg have you seen any significant changes in memory usage with this PR?

[Thalley]

From the point of view of the host?

We could consider making the directions absolute rather than relative like we have for ISO data paths:

#define BT_HCI_DATAPATH_DIR_HOST_TO_CTLR 0x00
#define BT_HCI_DATAPATH_DIR_CTLR_TO_HOST 0x01

The same argument can be applied for the ACL and ISO IN/OUT, but if we modify one, we should probably modify all, which seems outside the scope of this PR

[jhedberg]

From the point of view of the host?

From the point of view of BT_BUF_*_IN/OUT values when converting from a H:4 type (which don't convey any direction info by themselves).

[jhedberg]

I added a clarifying sentence to the docstring above the enum definition.

[jhedberg]

Also worth noting that this entire public header will hopefully disappear soon, so I wouldn't put too much effort into trying to micro-optimize it :)

[Copilot]

Pull Request Overview

This PR revises the HCI packet type encoding by moving the H:4 type byte from net_buf metadata into the net_buf payload. It deprecates the bt_buf_get/set_type() APIs and removes the experimental H:4 USB transport support.

• Remove H:4 mode enums, APIs, and raw-mode functions from hci_raw.h
• Update buffer API functions in buf.h to use H:4 prefix encoding
• Modify multiple HCI transport drivers to directly use the payload’s first byte for packet type

Reviewed Changes

Copilot reviewed 52 out of 53 changed files in this pull request and generated no comments.

File	Description
include/zephyr/bluetooth/hci_raw.h	Removed legacy H:4 mode definitions and raw-channel APIs
include/zephyr/bluetooth/buf.h	Updated buffer type conversion functions and deprecation of old APIs
drivers/bluetooth/hci/*	Replaced bt_buf_get/set_type() invocations with direct payload access
drivers/bluetooth/hci/h4.c	Removed redundant type handling before UART transmission

Files not reviewed (1)

• doc/releases/migration-guide-4.2.rst: Language not supported

[jhedberg]

@jhedberg have you seen any significant changes in memory usage with this PR?

I haven't done any profiling, but I'd expect to see at least a decrease in code size (if you just look at the diffstat). On aggregate it's a big reduction in code, however it'll be spread out over many platforms (a little for each HCI driver) so any single build wont show the same magnitude as the PR as a whole.

[alwa-nordic]

I am quite happy with how this looks now. I found a small mistake: