Bluetooth: Mesh: Composition data page 0 traversal #33614

trond-snekvik · 2021-03-23T14:01:11Z

Adds a parsing mechanism for Composition data page 0 in the Config
Client API, and uses it in the shell module to parse the incoming
composition data.

Demonstrates this API in the mesh_provisioner sample, allowing it to provision the basic mesh sample, as suggested by @pabigot in #33476.

cc @tsvehagen

The first byte of the composition data status message is the returned page index, not the status of the request. This is now reflected in the API. Signed-off-by: Trond Einar Snekvik <[email protected]>

jhedberg · 2021-03-23T14:30:03Z

samples/bluetooth/mesh_provisioner/src/main.c

 		return;
 	}

+	elem_addr = node->addr;
+	while (!bt_mesh_comp_elem_pull(&comp, &elem)) {


== 0 would be clearer to not make the reader think the function returns bool.

Alternatively, rethink the return value type (bool, enum, give back the elem pointer, etc)

Now returning a pointer, to avoid returning bool to indicate errors.

jhedberg · 2021-03-23T14:34:31Z

subsys/bluetooth/mesh/shell.c


-		shell_print(shell, "\tElement @ 0x%04x:", loc);
+	while (!bt_mesh_comp_elem_pull(&comp, &elem)) {


jhedberg · 2021-03-23T14:39:45Z

subsys/bluetooth/mesh/cfg_cli.c

+	}
+
+	elem->_buf =
+		(uint8_t *)net_buf_simple_pull_mem(page->_buf, modlist_size);


Looks like an unnecessary type cast

joerchan · 2021-03-23T14:33:48Z

include/bluetooth/mesh/cfg_cli.h

+ *  @return The model ID of the vendor model at the given index.
+ */
+struct bt_mesh_mod_id_vnd
+bt_mesh_comp_elem_mod_vnd(struct bt_mesh_comp_elem *elem, int idx);


Dont' have to break this line perhaps?

joerchan · 2021-03-23T14:38:16Z

include/bluetooth/mesh/cfg_cli.h

+int bt_mesh_comp_page_0_get(struct bt_mesh_comp_page_0 *comp,
+			    struct net_buf_simple *buf);
+
+/** @brief Pull an element from a composition data page.


from a composition data page 0?

More instances in this function API description.

If I'm following this right.

joerchan · 2021-03-23T14:39:31Z

include/bluetooth/mesh/cfg_cli.h

+};
+
+/** Composition data page 0 element representation */
+struct bt_mesh_comp_elem {


Would bt_mesh_comp_page_0_elem be more clear?

Probably, yes. I renamed both the composition data page and the elem to use a bt_mesh_comp_p0 prefix.

joerchan · 2021-03-23T14:40:42Z

subsys/bluetooth/mesh/cfg_cli.c

+	return 0;
+}
+
+int bt_mesh_comp_elem_pull(const struct bt_mesh_comp_page_0 *page,


similarly, would bt_mesh_comp_page_0_elem_pull be more clear?

jhedberg · 2021-03-23T14:41:36Z

subsys/bluetooth/mesh/cfg_cli.c

+
+uint16_t bt_mesh_comp_elem_mod(struct bt_mesh_comp_elem *elem, int idx)
+{
+	return sys_get_le16(&elem->_buf[idx * 2]);


Should you check elem->nsig first?

Added a bounds check, but there's no room for an error code in the return types of these functions, so I have made them return 0xffff. This can also be a valid return value though, so I think it's the lesser of two evils. I'd prefer not to change the return type, as this would mess up the vnd function.

jhedberg · 2021-03-23T14:43:07Z

subsys/bluetooth/mesh/cfg_cli.c

+struct bt_mesh_mod_id_vnd
+bt_mesh_comp_elem_mod_vnd(struct bt_mesh_comp_elem *elem, int idx)
+{
+	size_t offset = elem->nsig * 2 + idx * 4;


Same here, since this is a public API we might want to do a bounds check first?

trond-snekvik · 2021-03-24T10:14:43Z

Marking with DNM until @pabigot's comments in #33476 are resolved

jhedberg · 2021-03-25T09:46:33Z

subsys/bluetooth/mesh/cfg_cli.c

+	if (idx >= elem->nsig) {
+		return 0xffff;
+	}


Don't we want to take into use CHECKIF() from include/sys/check.h? I've seen that used in many Bluetooth audio related patches. It seems the general trend is to move toward that for all input parameter checking.

I wasn't aware of this. Makes sense, I'll change it.

Adds a parsing mechanism for Composition data page 0 in the Config Client API, and uses it in the shell module to parse the incoming composition data. Signed-off-by: Trond Einar Snekvik <[email protected]>

Instead of only configuring the Health Server model of the provisioned device, the Provisioner will instead walk the target's composition data, and bind each model to the same AppKey. This allows the provisioner to be used with the base mesh sample, and demonstrates the new composition data page 0 API. Signed-off-by: Trond Einar Snekvik <[email protected]>

Runs the Provisioner sample's main thread in a cooperative priority to avoid calling the Bluetooth APIs from a preemptive thread. Signed-off-by: Trond Einar Snekvik <[email protected]>

Adds blocking operation to the self configuration step by waiting for the status response. This ensures that the provisioner is fully configured, and reduces the number of loopback buffers required for the self-configuring. Signed-off-by: Trond Einar Snekvik <[email protected]>

Ups the CDB_NODE_COUNT config to fit 16 nodes, to make the sample more usable for general mesh testing. Signed-off-by: Trond Einar Snekvik <[email protected]>

pabigot · 2021-03-28T22:12:06Z

This needs to be merged to complete the resolution of #31031.

jhedberg · 2021-04-03T12:06:27Z

subsys/bluetooth/mesh/cfg_cli.c

+	if (buf->len < 10) {
+		return -EINVAL;
+	}


CHECKIF() here as well?

I left it out on purpose, as it's doing a comparison on on-air data. With CHECKIF, it can turn into an assert or be skipped, which would force the application to do their own check. This would sort of negate the intended effect of this macro

Right. Makes sense.

Bluetooth: Mesh: Rename bt_mesh_cfg_comp_data_get's status param to rsp

4a3d82b

The first byte of the composition data status message is the returned page index, not the status of the request. This is now reflected in the API. Signed-off-by: Trond Einar Snekvik <[email protected]>

trond-snekvik added area: Bluetooth area: Samples Samples area: API Changes to public APIs area: Bluetooth Mesh labels Mar 23, 2021

trond-snekvik requested a review from LingaoM March 23, 2021 14:01

trond-snekvik requested review from jhedberg, joerchan, nashif and Vudentz as code owners March 23, 2021 14:01

github-actions bot added the area: Documentation label Mar 23, 2021

trond-snekvik mentioned this pull request Mar 23, 2021

Bluetooth: Mesh: Add some actual behavior in sample #33476

Merged

jhedberg reviewed Mar 23, 2021

View reviewed changes

joerchan reviewed Mar 23, 2021

View reviewed changes

jhedberg reviewed Mar 23, 2021

View reviewed changes

zephyrbot requested review from asbjornsabo and Thalley March 23, 2021 20:28

zephyrbot assigned jhedberg Mar 23, 2021

trond-snekvik force-pushed the traverse_comp_data branch from 7204124 to 593b668 Compare March 24, 2021 09:26

trond-snekvik added the DNM This PR should not be merged (Do Not Merge) label Mar 24, 2021

jhedberg reviewed Mar 25, 2021

View reviewed changes

trond-snekvik added 4 commits March 26, 2021 14:12

Bluetooth: Mesh: Composition data page 0 traversal

693fa9c

Adds a parsing mechanism for Composition data page 0 in the Config Client API, and uses it in the shell module to parse the incoming composition data. Signed-off-by: Trond Einar Snekvik <[email protected]>

Bluetooth: Mesh: Provisioner sample: Cooperative thread

2b06b0a

Runs the Provisioner sample's main thread in a cooperative priority to avoid calling the Bluetooth APIs from a preemptive thread. Signed-off-by: Trond Einar Snekvik <[email protected]>

trond-snekvik force-pushed the traverse_comp_data branch from fc66ea5 to d2a70bd Compare March 26, 2021 13:12

Bluetooth: Mesh: Provisioner sample: Permit 16 nodes

dba5da1

Ups the CDB_NODE_COUNT config to fit 16 nodes, to make the sample more usable for general mesh testing. Signed-off-by: Trond Einar Snekvik <[email protected]>

trond-snekvik removed the DNM This PR should not be merged (Do Not Merge) label Mar 29, 2021

pabigot mentioned this pull request Mar 29, 2021

Changes to bluetooth mesh for k_work API #33782

Merged

LingaoM approved these changes Apr 2, 2021

View reviewed changes

jhedberg reviewed Apr 3, 2021

View reviewed changes

jhedberg approved these changes Apr 6, 2021

View reviewed changes

jhedberg merged commit d24810c into zephyrproject-rtos:master Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Mesh: Composition data page 0 traversal #33614

Bluetooth: Mesh: Composition data page 0 traversal #33614

trond-snekvik commented Mar 23, 2021

jhedberg Mar 23, 2021

jhedberg Mar 23, 2021

trond-snekvik Mar 24, 2021

jhedberg Mar 23, 2021

jhedberg Mar 23, 2021

joerchan Mar 23, 2021

joerchan Mar 23, 2021

joerchan Mar 23, 2021

trond-snekvik Mar 24, 2021

joerchan Mar 23, 2021

jhedberg Mar 23, 2021

trond-snekvik Mar 24, 2021

jhedberg Mar 23, 2021

trond-snekvik commented Mar 24, 2021

jhedberg Mar 25, 2021

trond-snekvik Mar 26, 2021

pabigot commented Mar 28, 2021

jhedberg Apr 3, 2021

trond-snekvik Apr 6, 2021

jhedberg Apr 6, 2021


		shell_print(shell, "\tElement @ 0x%04x:", loc);
		while (!bt_mesh_comp_elem_pull(&comp, &elem)) {

Bluetooth: Mesh: Composition data page 0 traversal #33614

Bluetooth: Mesh: Composition data page 0 traversal #33614

Conversation

trond-snekvik commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trond-snekvik commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabigot commented Mar 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment