Repository navigation
mgmt: mcumgr: transport: smp_bt: Fix transport mgmt info functions - #4
Open
JPHutchins wants to merge 3 commits into
Open
JPHutchins wants to merge 3 commits into
JPHutchins wants to merge 3 commits into
Conversation
The Bluetooth transport emitted the ID of its transport mode under the CBOR key "type" in the details on transport modes response. The transport management group documentation, every other transport that implements the information functions and the transport_mgmt test use "id", so a client following the documentation could not read the mode ID of the Bluetooth transport. Introduced in commit e095e47 ("mgmt: mcumgr: transport: smp_bt: Add support for transport mgmt"). Assisted-by: Claude:claude-opus-5-5 Signed-off-by: JP Hutchins <jp@intercreate.io>
smp_bt_bridge_config_details() returned MGMT_RETURN_CHECK(ok) as its bool result, which is false on success, and never set the return code. transport_mgmt_config_details() therefore treated every successful encode as a failure and returned its uninitialized rc as the SMP result, leaving the configs list unterminated. The function also reported the mode 0 configuration for any requested mode. Set the return code and return the encode status, and reject modes other than 0 with TRANSPORT_MGMT_ERR_INVALID_MODE, as the other transports do. Introduced in commit e095e47 ("mgmt: mcumgr: transport: smp_bt: Add support for transport mgmt"). Assisted-by: Claude:claude-opus-5-5 Signed-off-by: JP Hutchins <jp@intercreate.io>
The transport_bluetooth test only checked that the Bluetooth transport builds with transport management enabled, so nothing checked the responses of its information functions. Run the test on native_sim and query the Bluetooth transport through the dummy transport: check the details on transport modes response, the details on transport configuration response for mode 0 and the rejection of other modes. The test does not call bt_enable(), so no Bluetooth controller is needed. Drop build_only so that, as with the transport_mgmt test, the test can also run on nrf52840dk/nrf52840 hardware. native_sim/native/64 is not enabled because the Bluetooth transport user data does not fit the default MCUmgr buffer user data size with 64-bit pointers. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: JP Hutchins <jp@intercreate.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
LLM Disclosure
This post was authored by claude-opus-5-5 on behalf of @JPHutchins. JP asked me to turn a research note on SMP group 11 into a Zephyr fix for the Bluetooth transport, which emits
typewhere the group specifiesid. While fixing it I found a second defect in the neighboring configuration details callback, fixed it in a separate commit, and wrote a test that covers both.Important
Review copy on the intercreate fork, not the upstream PR. Everything above Review notes is the proposed upstream description, under the title of this PR. Upstream,
@thedjnKbecomes a live mention.Fixes both transport management information callbacks of the Bluetooth SMP transport and turns
tests/subsys/mgmt/mcumgr/transport_bluetoothfrom a build-only test into a runtime test that covers them. The callbacks were added in zephyrproject-rtos#119051, which brought Bluetooth support to the transport management group from zephyrproject-rtos#113952.Details on transport modes (command 7).
smp_bt_bridge_modes()writes the mode ID under the keytype. The group 11 documentation specifiesid, as do the UART, raw UART, shell, dummy and raw dummy transports and thetransport_mgmttest, so a client that follows the documentation cannot read the mode ID of a Bluetooth device.Details on transport configuration (command 8).
smp_bt_bridge_config_details()returnsMGMT_RETURN_CHECK(ok)from a function that returnsbool, which isfalseon success, and never writes*rc.transport_mgmt_config_details()therefore returns its uninitializedrcand leaves theconfigslist unterminated. The callback also ignored the requested mode. It now sets*rc, returns the encode status, and rejects modes other than 0 withTRANSPORT_MGMT_ERR_INVALID_MODE, as the other transports do.Transport management is experimental and 4.5.0 is not tagged yet (
VERSIONis4.4.99), so no release has emitted thetypekey and there is nothing to stay compatible with.Found while implementing a client for this group: JPHutchins/smp#73, tracked in JPHutchins/smp#72.
/cc
@thedjnKTesting
west twister -p native_sim -T tests/subsys/mgmt/mcumgr/transport_bluetooth, with the test from this PR andsmp_bt.cfrom each point in the series:smp_bt.ctest_modestest_config_detailstest_config_details_invalid_modemainExpected mode 'id' to be presentwest twister -p native_sim -T tests/subsys/mgmt/mcumgr: 20 of 20 configurations pass before the series, 21 of 21 after; the new one istransport_bluetooth.nrf52840dk/nrf52840: built with twister, not run on hardware.native_sim/native/64is not inplatform_allow:struct smp_bt_user_dataholds a pointer and does not fit the Bluetooth default of 8 bytes forCONFIG_MCUMGR_TRANSPORT_NETBUF_USER_DATA_SIZEon 64-bit hosts.Review notes
Not part of the upstream description.
typekey (commit 1). Commit 2 fixessmp_bt_bridge_config_details(), which I found in the neighboring code while fixing commit 1: same file, same origin PR, same maintainer, so it is in this PR as its own commit. It can be split into a separate PR; the two config details tests in commit 3 would move with it.Assisted-by: Claude:claude-opus-5-5and yourSigned-off-by, added withgit commit -sas you authorized. NoCo-authored-by.AGENTS.mdasks for no mention of AI tools in PR bodies beyondAssisted-by. I followed your disclosure rule, as on lib: notify: fix C23 incompatible function pointer types zephyrproject-rtos/zephyr#115691.main. It is 149 commits ahead ofic/main, none of them under mcumgr.JPHutchins/zephyr, and open the PR with the description above.READpermission upstream, so--add-reviewer thedjnKwill probably be refused. The/ccis the fallback, and the assignment bot addsnordicjm(the MCU Manager maintainer, the same person) anyway.Also noticed, not changed
Candidates for issues or follow-up PRs:
CONFIG_MCUMGR_TRANSPORT_BT=yandCONFIG_MCUMGR_GRP_TRANSPORT=ybut noCONFIG_BT_CENTRAL/CONFIG_BT_GATT_CLIENT, the link fails with undefinedbt_gatt_discover,bt_gatt_subscribe,bt_conn_le_createandbt_gatt_write_without_response_cb. The outgoing bridge code insmp_bt.cis compiled underCONFIG_MCUMGR_GRP_TRANSPORTalone, and no Kconfig dependency prevents that configuration. Reproduced on native_sim with-DCONFIG_BT=y -DCONFIG_BT_PERIPHERAL=y -DCONFIG_MCUMGR_TRANSPORT_BT=yon thetransport_mgmttest.MCUMGR_TRANSPORT_NETBUF_MIN_USER_DATA_SIZEdefaults to 8 for Bluetooth, butstruct smp_bt_user_datais 16 bytes with 64-bit pointers. The Bluetooth transport therefore fails itsBUILD_ASSERTonnative_sim/native/64with default settings.smp_bt_bridge_modes()opens its map with a bound of 2 for 4 pairs. The other transports use the pair count; Copilot flagged the same mismatch in the mgmt: mcumgr: grp: Add initial transport mgmt implementation zephyrproject-rtos/zephyr#113952 review. It is harmless with the current zcbor, because the bound only reserves header width and 2 and 4 both encode in one byte, so it is left alone.doc/services/device_mgmt/smp_protocol.rstlists groups 0–9, 63 and 64. Groups 10 and 11 appear only in the toctree.