Bluetooth: Mesh: Add Directed Forwarding Support #66883

LingaoM · 2023-12-22T11:15:32Z

Add Directed Forwarding Support

#66480

PTS Case(not passed yet, other passed test case not list):

Directed Forwarding

Directed Forwarding Discovery

MESH/NODE/DF/DISC/BV-03-C
MESH/NODE/DF/DISC/BV-10-C

Directed Forwarding Establishment

~~MESH/NODE/DF/EST/BV-04-C~~

Directed Forwarding Echo

MESH/NODE/DF/ECHO/BI-04-C(LOOK LIKE PTS BUG)

Directed Forwarding Model

Directed Control State

~~MESH/SR/DFM/DFS/BV-04-C~~

Forwarding Table State

~~MESH/SR/DFM/FTS/BV-10-C~~
~~MESH/SR/DFM/FTS/BV-11-C~~

Directed Control Network Transmit State

~~MESH/SR/DFM/DCNTS/BV-01-C~~

Test Report:
MDF.zip

LingaoM · 2024-01-03T06:34:14Z

@PavelVPV @alxelax Except for listed above for several PTS tests not passed, everything else passed, so look forward to your review now.

omkar3141 · 2024-01-03T09:39:48Z

@LingaoM Can you please provide complete list of passing or not passing PTS tests? PR description is not clear with respect to what has been passed and what did not.

LingaoM · 2024-01-03T10:13:23Z

@LingaoM Can you please provide complete list of passing or not passing PTS tests? PR description is not clear with respect to what has been passed and what did not.

MDF.zip

omkar3141 · 2024-01-03T10:58:44Z

@LingaoM Can you please provide complete list of passing or not passing PTS tests? PR description is not clear with respect to what has been passed and what did not.

MDF.zip

Ok. So 126 out of 466 are PASS.

LingaoM · 2024-01-03T11:20:02Z

@LingaoM Can you please provide complete list of passing or not passing PTS tests? PR description is not clear with respect to what has been passed and what did not.

MDF.zip

Ok. So 126 out of 466 are PASS.

BTW: Only Mesh Directed Forwarding tested.

Add Directed Forwarding Support Signed-off-by: Lingao Meng <[email protected]>

alxelax · 2024-01-11T14:28:05Z

Hi @LingaoM, I've just started reviewing this PR.
Thanks for your great work. DFW is very complex feature. This is quite huge and might take a couple of days.
While I will be reviewing in details I'd like to emphasize the main architectural disadvantages that are seen already. Mesh-1.1 assumes that every subnetwork is operating with direct-forwarding algorithms independently. How do I understand that? It should be possible to have a scalable number of state machines as well as discovery and forwarding tables:

Figure 4.9: Path Origin State Machine states, events, and transitions

Could be all states encapsulated within the subnetwork structure?
Could we formalize the state machine that the specification suggests in a more readable manner? I meant Zephyr's State Machine Framework: https://docs.zephyrproject.org/latest/services/smf/index.html
Could we split dfw.c functionality into:
3.1) server commands receiving and state updating over commands and state bindings;
3.2) direct forwarding procedures like subclause 3.6.8.2 Directed forwarding procedures describes

Currently dfw.c is enormously huge and this is very difficult to maintain in the future.

If you have any questions or topics for architecture discussion we could continue in Zephyr's Mesh Discord. I expect this PR will have a lot of comments to not pollute it with architecture discussion. :)

LingaoM · 2024-01-16T01:39:13Z

subsys/bluetooth/mesh/dfw.c

+			__ASSERT_NO_MSG(false);
+		}
+
+		sub->dfw = dfw;


Could be all states encapsulated within the subnetwork structure?

@alxelax I need to explain to you why DFW states is not placed in struct bt_mesh_subnet, this is because during the process of calling settings_load(), we cannot guarantee the order in which the information of netkey and dfw will be restored.

Mesh Protocol 1.1 requires that the directed forwarding states of fixed paths and subnets needs to be stored.

If you look at my implementation code, you can see that its corresponding storage path is bt/mesh/DFW/Sub/Cfg/xxx, bt/mesh/DFW/Sub/Fw/xxx/xxx/xxx and bt/mesh/DFW/Sub/Dep/xxx/xxx/xxx/xxx.

According to my implementation, when settings_load() is executed, if the dfw information is restored first, only net_idx will be temporarily stored in the struct bt_mesh_dfw_subnet. After waiting for the subnet to be restored, a one-to-one correspondence will be established between the subnet and dfw in BT_MESH_KEY_ADDED .

LingaoM · 2024-01-16T01:43:02Z

Mesh-1.1 assumes that every subnetwork is operating with direct-forwarding algorithms independently. How do I understand that? It should be possible to have a scalable number of state machines as well as discovery and forwarding tables:

My understanding of your mean is to design the discovery table and forwarding table into separate like pools, rather than using the same fixed number of configurations for each subnet, right?

If you have any questions or topics for architecture discussion we could continue in Zephyr's Mesh Discord.

Due to GFW restrictions in mainland China, it may be more convenient for me to communicate using github, thanks.

alxelax

Hi, I agree with your explanation about the first point of my initial list. However, the second and third should be done.

Using Zephyr's state machine framework will reduce complexity of implementation. We will use some already done and tested pattern.
Also, splitting on server and common parts are mandatory. At the moment, we do not know which part of the current implementation will be required for dfw's client implementation. I'd prefer to keep dfw logic in common place rather than client will reuse server functionality.

Also, I think dfw integration into existing mesh code should be redesigned. At the current implementation, I see, some layers take responsibility to trigger one or another dfw algorithm. I do not think this is correct. They should send events to dfw and dfw should decide what to do according to state machine states.

Dfw implementation requires simplification. There are a couple of functions that are enormously long and complex. Also, there are some functions parts of them look similar (look at my comments).

My concentration to the about 3rd thousands line of dfw implementation went down. Obviously I missed a lot of places those should be improved or have bugs.
To be sure the quality of dfw is good enough, it requires significant babblesim testing.
Right now, I recommend keeping it as experimental feature.

alxelax · 2024-01-12T08:22:34Z

include/zephyr/bluetooth/mesh/cfg.h

@@ -587,6 +587,14 @@ uint8_t bt_mesh_app_key_del(uint16_t app_idx, uint16_t net_idx);
 */
 bool bt_mesh_app_key_exists(uint16_t app_idx);

+/** @brief Get the binding network key index of the application key.


Suggested change

/** @brief Get the binding network key index of the application key.

/** @brief Get the bound subnetwork index by the application index.

Zephyr's Mesh implementation operates subnetwork term instead of network key index.

Why did you add this function as the public API instead of adding it into local cfg.h?

alxelax · 2024-01-12T08:23:27Z

include/zephyr/bluetooth/mesh/cfg.h

+ *
+ *  @param app_idx Application index.
+ *
+ *  @return The binding network key index if Application is known, BT_MESH_KEY_UNUSED otherwise.


Suggested change

* @return The binding network key index if Application is known, BT_MESH_KEY_UNUSED otherwise.

* @return The subnetwork index if it is bound to provided application index, BT_MESH_KEY_UNUSED otherwise.

alxelax · 2024-01-12T08:29:36Z

include/zephyr/bluetooth/mesh/cfg.h

+ *
+ *  @return The binding network key index if Application is known, BT_MESH_KEY_UNUSED otherwise.
+ */
+uint16_t bt_mesh_app_binding_net_key_get(uint16_t app_idx);


Suggested change

uint16_t bt_mesh_app_binding_net_key_get(uint16_t app_idx);

uint16_t bt_mesh_subnet_by_app_idx_get(uint16_t app_idx);

alxelax · 2024-01-12T08:40:42Z

include/zephyr/bluetooth/mesh/msg.h

-
-	/** Send message with a random delay according to the Access layer transmitting rules. */
-	bool  rnd_delay;
+	uint8_t send_rel:1,	/*


I guess you need to add comment above each bitfield.
This syntax /** is the marker for doxygen to pick up comment into generated documentation. The comment style that you used here will be ignored by doxygen.

alxelax · 2024-01-12T08:47:13Z

include/zephyr/bluetooth/mesh/msg.h

@@ -24,6 +24,14 @@
 extern "C" {
 #endif

+/** Security material */


/** Possible message security credentials */ enum bt_mesh_cred { BT_MESH_CRED_FLOODING, /**< Flooding security credentials */ BT_MESH_CRED_FRIEND, /**< Friendship security credentials */ BT_MESH_CRED_DIRECTED, /**< Directed security credentials */ };

Immutable-credentials is an additional tag for security credential, but not a separate type of credentials.
It might be flooding immutable, but might be just flooding.
I'd prefer to not mix these two entities in the single variable.

Btw. BT_MESH_CRED_IMMUTABLE is not analyzed anywhere in the code. Do you need it at all?

alxelax · 2024-01-16T11:43:19Z

subsys/bluetooth/mesh/dfw.c

+
+}
+
+static void dfw_state_machine_state_set(struct dfw_state_machine *machine,


I'm pretty sure it is better to use Zephyr's state machine framework here.
Since you've already implemented it on state machine manner, it will not be a complex task to convert it to Zephyr's fsm.

alxelax · 2024-01-16T12:00:31Z

subsys/bluetooth/mesh/dfw.c

+	dfw_forwarding_clear(fw);
+}
+
+static void dfw_path_reply_delay_expired(struct k_work *work)


Seems very complex function with multirange nested if. Could it be split on simple functions?

alxelax · 2024-01-16T12:02:48Z

subsys/bluetooth/mesh/dfw.c

+	return false;
+}
+
+int bt_mesh_dfw_path_request(struct bt_mesh_net_rx *rx, struct net_buf_simple *buf)


One more long and complex function that requires refactoring.

alxelax · 2024-01-16T12:04:15Z

subsys/bluetooth/mesh/dfw.c

+	}
+}
+
+int bt_mesh_dfw_path_reply(struct bt_mesh_net_rx *rx, struct net_buf_simple *buf)


One more function for splitting and refactoring.

alxelax · 2024-01-16T12:07:40Z

subsys/bluetooth/mesh/dfw.c

+		(void)bt_mesh_dfw_get(sub->net_idx, &state);
+		net_buf_simple_add_u8(&msg, state);
+
+		(void)bt_mesh_dfw_relay_get(sub->net_idx, &state);


Why do all such kind of functions in dfw have returned values? It is not analyzed and sent in the air as is.

PavelVPV

This is a huge work, @LingaoM! We really appreciate it, thanks!

I'm posting my first comments. I've just briefly gone through the interaction between the MDF and the rest of the stack. I didn't check the compliance with the spec and dfw.c code.

I agree with all @alxelax points. We need to remove the dependency on the MDF from the layers. From what I see using callbacks similar to bt_mesh_subnet_cb/bt_mesh_friend_cb/bt_mesh_lpn_cb/etc. could be an option. I wish the stack used a single events callback like we had in nRF5 SDK for Mesh, then it would be less memory consuming and would give more flexibility with regards to public API (bt_mesh_friend_cb and bt_mesh_lpn_cb are public callbacks). I think it would make sense to add such callback for cfg_srv.c to post events like subscription list change, publication state change, etc.

I also agree with @alxelax that we need to use the SMF which is well (I believe) tested and maintained in Zephyr rather than inventing something own. As well as I agree with @alxelax that we should hide all MDF state machine triggers under some API that generate events for MDF (in places where we can't use callbacks to move the code to dfw.c).

PavelVPV · 2024-01-23T15:46:53Z

subsys/bluetooth/mesh/Kconfig

+
+config BT_MESH_DFW_STATE_MACHINE_COUNT
+	int "Max number of Path Origin state Machine"
+	default 20


Here and below. use range as 20 is minimum allowed value by spec.

PavelVPV · 2024-01-23T15:47:22Z

subsys/bluetooth/mesh/Kconfig

+menu "Directed Control"
+
+config BT_MESH_DFW_ENABLED
+	bool "Directed forwarding enable"


Here and in other such options: enable -> enabled (as in other such options), or put Enable in the first place, or enabled by default (we have added by default to few options some time ago to differentiate these options from options that enables support for a feature).

PavelVPV · 2024-01-23T20:14:00Z

subsys/bluetooth/mesh/access.c

@@ -47,7 +48,7 @@ struct mod_pub_val {
 		uint8_t  retransmit;
 		uint8_t  period;
 		uint8_t  period_div:4,
-			 cred:1;
+			 cred:2;


Is it backward compatible? If cred was stored with value 1 in the old firmware, will it be restored correctly as BT_MESH_CRED_FRIEND when the firmware is updated even without MDF support?

PavelVPV · 2024-01-24T06:46:56Z

subsys/bluetooth/mesh/dfw.c

+	if (!ADDR_RANGE_IN(src, &fw->path_origin)) {
+		for (i = 0; i < ARRAY_SIZE(fw->dependent_origin); i++) {
+			if (ADDR_RANGE_IN(src, &fw->dependent_origin[i])) {
+				break;
+			}
+		}
+
+		if (i == ARRAY_SIZE(fw->dependent_origin)) {
+			return false;
+		}
+	}


Is return true; missed somewhere here?

PavelVPV · 2024-01-24T07:30:15Z

subsys/bluetooth/mesh/net.c

+} relay_tables[] = {
+#if defined(CONFIG_BT_MESH_RELAY)
+	{
+		.inbound_bearer = BT_MESH_NET_IF_ADV,


But in this table it is only BT_MESH_NET_IF_ADV.

PavelVPV · 2024-01-25T13:40:54Z

subsys/bluetooth/mesh/net.c

+	cred = &rx->sub->keys[SUBNET_KEY_TX_IDX(rx->sub)].flooding;
+
+#if defined(CONFIG_BT_MESH_DFW)
+	if (outbound_security_material == BT_MESH_CRED_DIRECTED) {
+		cred = &rx->sub->keys[SUBNET_KEY_TX_IDX(rx->sub)].directed;
+	}
+#endif


Suggested change

cred = &rx->sub->keys[SUBNET_KEY_TX_IDX(rx->sub)].flooding;

#if defined(CONFIG_BT_MESH_DFW)

if (outbound_security_material == BT_MESH_CRED_DIRECTED) {

cred = &rx->sub->keys[SUBNET_KEY_TX_IDX(rx->sub)].directed;

}

#endif

#if defined(CONFIG_BT_MESH_DFW)

if (outbound_security_material == BT_MESH_CRED_DIRECTED) {

cred = &rx->sub->keys[SUBNET_KEY_TX_IDX(rx->sub)].directed;

}

else

#endif

{

cred = &rx->sub->keys[SUBNET_KEY_TX_IDX(rx->sub)].flooding;

}

PavelVPV · 2024-01-25T13:42:04Z

subsys/bluetooth/mesh/net.c

+		outbound_bearers |= relay_tables[i].conditions(adv, rx,
+							&outbound_security_material);
+	}
+


Exit earlier if no outbound_bearers selected?

PavelVPV · 2024-01-25T13:45:06Z

subsys/bluetooth/mesh/transport.c

+#if defined(CONFIG_BT_MESH_DFW)
+	if (ctl_op <= TRANS_CTL_OP_PATH_REQ_SOLICITATION &&
+	    ctl_op >= TRANS_CTL_OP_PATH_REQUEST) {
+		enum bt_mesh_feat_state state = BT_MESH_FEATURE_DISABLED;
+
+		if (rx->ctx.cred != BT_MESH_CRED_DIRECTED &&
+		    ctl_op != TRANS_CTL_OP_PATH_ECHO_REPLY) {
+			return -EINVAL;
+		}
+
+		(void)bt_mesh_dfw_get(rx->ctx.net_idx, &state);
+		if (state != BT_MESH_FEATURE_ENABLED) {
+			return -EACCES;
+		}
+	}
+
+	switch (ctl_op) {
+	case TRANS_CTL_OP_PATH_REQUEST:
+		return bt_mesh_dfw_path_request(rx, buf);
+	case TRANS_CTL_OP_PATH_REPLY:
+		return bt_mesh_dfw_path_reply(rx, buf);
+	case TRANS_CTL_OP_PATH_CONFIRM:
+		return bt_mesh_dfw_path_confirm(rx, buf);
+	case TRANS_CTL_OP_PATH_ECHO_REQ:
+		return bt_mesh_dfw_path_echo_request(rx, buf);
+	case TRANS_CTL_OP_PATH_ECHO_REPLY:
+		return bt_mesh_dfw_path_echo_reply(rx, buf);
+	case TRANS_CTL_OP_DEPENDENT_NODE_UPDATE:
+		return bt_mesh_dfw_dependent_node_update(rx, buf);
+	case TRANS_CTL_OP_PATH_REQ_SOLICITATION:
+		return bt_mesh_dfw_path_request_solicitation(rx, buf);
+	}
+#endif


Create a function in dfw.c and move this code there?

PavelVPV · 2024-01-25T13:46:46Z

subsys/bluetooth/mesh/transport.h

@@ -77,6 +84,43 @@ struct bt_mesh_ctl_friend_sub_confirm {
 	uint8_t xact;
 } __packed;

+struct bt_mesh_ctl_path_request {


Are all these structures needs to be accessible outside of dfw.c? Why not define them there?

PavelVPV · 2024-01-25T14:16:56Z

subsys/bluetooth/mesh/transport.c

 		switch (ctl_op) {
 		case TRANS_CTL_OP_FRIEND_UPDATE:
+			if (rx->ctx.cred != BT_MESH_CRED_FRIEND) {


I guess because the rx address can be the lpn's address, but the opcode not necessarily Friend Update or Subscription Confirmation. But what about this:

if ((ctl_op == TRANS_CTL_OP_FRIEND_UPDATE || ctl_op == TRANS_CTL_OP_FRIEND_SUB_CFM) && rx->ctx.cred != BT_MESH_CRED_FRIEND) { LOG_WRN("Message from friend with wrong credentials"); return -EINVAL; }

this will preserve the error message. Otherwise it could be just:

if (rx->friend_cred) { switch (ctl_op) { // TRANS_CTL_OP_FRIEND_UPDATE // TRANS_CTL_OP_FRIEND_SUB_CFM ...

PavelVPV

Posting more comments. Need to go through the state machine.

PavelVPV · 2024-01-29T11:35:39Z

subsys/bluetooth/mesh/dfw.c

+	if (IS_ENABLED(CONFIG_BT_SETTINGS)) {
+		dfw_cfg_store();
+	}


I though that this patter always creates the unused function warning when CONFIG_BT_SETTINGS is disabled. Perhaps IS_ENABLED(CONFIG_BT_SETTINGS) should be moved inside dfw_cfg_store()?

PavelVPV · 2024-01-29T11:53:50Z

subsys/bluetooth/mesh/dfw.c

+{
+	uint8_t xmit = net_buf_simple_pull_u8(buf);
+
+	dfw_cfg.directed_ctl_relay_retransmit = xmit;


We need to check if it actually changed to not trigger settings once again. Same for other states. I see that you already have such functions (e.g. bt_mesh_dfw_ctl_relay_retransmit_set) that avoid extra store. They can be used here.

PavelVPV · 2024-01-29T12:25:44Z

subsys/bluetooth/mesh/dfw.c

+		goto send_status;
+	}
+
+	mod = get_model(elem, &buf->data[3], buf->len - 3);


Check that buf->len either 2 or 4. Otherwise, return -EINVAL and abort message processing.

PavelVPV · 2024-01-29T12:30:29Z

subsys/bluetooth/mesh/dfw.c

+	net_buf_simple_add_u8(&msg, status);
+
+	/* Directed Publish Policy Get Message */
+	if (buf->len == 4 || buf->len == 6) {


If the length is neither 4 nor 6, we can't send message.

PavelVPV · 2024-01-29T12:37:15Z

subsys/bluetooth/mesh/dfw.c

+	uint16_t net_idx;
+
+	net_idx = sys_get_le16(buf->data);
+	if (net_idx == BT_MESH_KEY_UNUSED) {


BT_MESH_KEY_UNUSED is an internal value. For this, every index starting from 0x1000 should be prohibited (figure 4.5).

PavelVPV · 2024-01-30T13:07:33Z

subsys/bluetooth/mesh/dfw.c

+static const char *path_lifetime_to_str(enum bt_mesh_dfw_path_lifetime lifetime)
+{
+	static const char * const strs[] = {
+		"12 Minuters",
+		"2 Hours",
+		"24 Hours",
+		"10 Days",
+	};
+
+	return strs[lifetime];
+}


Doesn't it generate a unused function warning if the logs are disabled? Shouldn't it be wrapped into #if defined(CONFIG_BT_MESH_DFW_LOG_LEVEL_DBG)/#endif?

PavelVPV · 2024-01-30T13:11:11Z

subsys/bluetooth/mesh/dfw.c

+		return -EINVAL;
+	}
+
+	friend_enable = buf->data[6];


Prohibited values should be checked

PavelVPV · 2024-01-30T13:46:52Z

subsys/bluetooth/mesh/dfw.c

+	/*
+	 * Ignore for all-directed-forwarding-nodes, all-nodes, and all-relays
+	 * fixed group addresses.
+	 */


This comment probably belongs to the if statement at line 3141

PavelVPV · 2024-01-30T14:42:55Z

subsys/bluetooth/mesh/dfw.c

+	fw_new->echo_intv.ticks *= percent;
+	fw_new->echo_intv.ticks /= 100;


Are you sure we can access ticks field of k_timeout_t? I don't see any sign that this field can be treated as public. In fact, the description for k_timeout_t says:

Timeout arguments presented to kernel APIs are stored in this opaque type

Thought I may understand this incorrectly.

PavelVPV · 2024-01-30T15:04:44Z

subsys/bluetooth/mesh/dfw.c

+		if (!dfw) {
+			__ASSERT_NO_MSG(false);
+		}


Suggested change

if (!dfw) {

__ASSERT_NO_MSG(false);

}

__ASSERT_NO_MSG(!!dfw);

PavelVPV

I think I'm done with the review for the current moment.

We also need some documentation page that will describe this feature (how it works, what is supported, configuration and API (dfw srv model, credentials part)).

PavelVPV · 2024-01-30T20:54:40Z

subsys/bluetooth/mesh/dfw.c

+	dfw->unicast_echo_intv = cfg.unicast_echo_intv;
+	dfw->multicast_echo_intv = cfg.multicast_echo_intv;
+
+	LOG_DBG("Restored directed forwarding subnet configuration value");


Suggested change

LOG_DBG("Restored directed forwarding subnet configuration value");

LOG_DBG("Restored directed forwarding configuration value for subnet %#03x", net_idx);

PavelVPV · 2024-01-30T21:14:29Z

subsys/bluetooth/mesh/dfw.c

+	val.is_dependent_origin = is_dependent_origin;
+	val.secondary_count = dependent->secondary_count;
+
+	err = settings_delete(path);


Either I don't understand something or this should be settings_save_one instead of settings_delete.

PavelVPV · 2024-01-31T07:39:58Z

subsys/bluetooth/mesh/dfw.c

+	dv->forwarding_number = ++dfw->forwarding_number;
+	dv->metric = 0;
+
+	k_work_init_delayable(&dv->timer, dfw_initialization_discovery_expired);


Why you didn't use Z_WORK_DELAYABLE_INITIALIZER for this timer?

PavelVPV · 2024-01-31T07:51:41Z

subsys/bluetooth/mesh/dfw.c

+	if (fw->path_not_ready) {
+		fw->path_not_ready = false;
+	}


Seems like it could be just:

Suggested change

if (fw->path_not_ready) {

fw->path_not_ready = false;

}

fw->path_not_ready = false;

PavelVPV · 2024-01-31T08:21:13Z

subsys/bluetooth/mesh/dfw.c

+		(void)bt_rand(&random, sizeof(random));
+		random %= 2000;
+		random += ARRAY_INDEX(state_machines, machine) * 2000;
+		(void)k_work_reschedule(&machine->timer, K_MSEC(random));


In our implementation, when device boots up, a model starts publishing after the publication period:

zephyr/subsys/bluetooth/mesh/access.c

Lines 2710 to 2736 in 25cfe86

if (mod->pub && mod->pub->addr != BT_MESH_ADDR_UNASSIGNED) {

int32_t ms = bt_mesh_model_pub_period_get(mod);

if (IS_ENABLED(CONFIG_BT_MESH_DFW) && mod->pub->cred == BT_MESH_CRED_DIRECTED) {

(void)bt_mesh_dfw_path_origin_state_machine_start(

bt_mesh_app_binding_net_key_get(mod->pub->key),

NULL, mod->pub->addr, true);

}

if (mod->pub->update && ms > 0) {

/* Delay the first publication after power-up for longer time (section

* 3.7.3.1):

*

* When the publication of a message is the result of a power-up, a state

* transition progress update, or completion of a state transition, multiple

* nodes may be reporting the state change at the same time. To reduce the

* probability of a message collision, these messages should be sent with a

* random delay between 20 and 500 milliseconds.

*/

uint16_t random;

random = !!mod->pub->delayable ? pub_delay_get(RANDOM_DELAY_LONG) : 0;

LOG_DBG("Starting publish timer (period %u ms, delay %u ms)", ms, random);

k_work_schedule(&mod->pub->timer, K_MSEC(ms + random));

}

}

2 seconds even multiplied by the number of state machines may be less than publication period for some models. MDF will monitor the path, but will always fail. Should we change our implementation in the access layer to start publication without waiting for the publication period? We can make it always use 20 to 500ms delay to randomize publications from several models. @omkar3141 do you have any thoughts?

PavelVPV · 2024-01-31T09:59:09Z

subsys/bluetooth/mesh/dfw.c

+	if (atomic_test_and_set_bit(fw->flags, BT_MESH_DFW_FORWARDING_FLAG_ECHO_REPLY)) {
+		LOG_ERR("Not received Echo Reply");
+		dfw->update_id++;
+		dfw_forwarding_clear(fw);
+
+		(void)bt_mesh_dfw_path_origin_state_machine_start(dfw->net_idx, NULL,
+								  ctx.addr, false);
+		return;
+	}


I'm not sure this is correct. This depends on the Path Origin State Machine state. If the state machine is in the Path Validation state, then according to Table 4.347, when the path validation fails due to not received echo reply, we should start the new Directed Forwarding Initialization procedure:

Table 4.346:

End of the section 3.6.8.2.6:

But here a new machine instance is started which is only applicable for the Path In Use state:

I also see that DFW_STATE_MACHINE_EVENT_PATH_VALID_FAILED event is not used.

PavelVPV · 2024-01-31T11:01:18Z

subsys/bluetooth/mesh/dfw.h

+	BT_MESH_DFW_ENABLED,		/* Indicate the directed enabled or not. */
+	BT_MESH_DFW_RELAY_ENABLED,	/* Indicate the directed relay enabled or not. */
+	BT_MESH_DFW_FRIEND_ENABLED,	/* Indicate the directed friend enabled or not. */


These names are confusing when read for example bt_mesh_dfw_set function. Could the postfix be for example _STATE: BT_MESH_DFW_STATE, BT_MESH_DFW_RELAY_STATE, BT_MESH_DFW_FRIEND_STATE?

PavelVPV · 2024-01-31T16:31:43Z

subsys/bluetooth/mesh/dfw.c

+		dfw_table_clear(sub->dfw, true);
+
+		dfw_subnet_cfg_clear(sub->dfw);
+
+		/* Remove All pending store flags */
+		atomic_clear(sub->dfw->flags);


All settings API should be called from the settings work (thread).

PavelVPV · 2024-01-31T20:39:59Z

subsys/bluetooth/mesh/net.c

+} relay_tables[] = {
+#if defined(CONFIG_BT_MESH_RELAY)
+	{
+		.inbound_bearer = BT_MESH_NET_IF_ADV,


And if I don't miss anything, now the relay from proxy to adv is broken (which was previously implemented in relay_to_adv function).

github-actions · 2024-05-07T00:29:43Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

LingaoM added area: Bluetooth area: Bluetooth Mesh labels Dec 22, 2023

LingaoM added this to the future milestone Dec 22, 2023

LingaoM requested review from PavelVPV, Andrewpini and alxelax December 22, 2023 11:15

zephyrbot requested review from akredalen, asbjornsabo, hermabe, jhedberg, sjanc, Thalley and Vudentz December 22, 2023 11:16

zephyrbot assigned PavelVPV Dec 22, 2023

LingaoM added DNM This PR should not be merged (Do Not Merge) In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on labels Dec 22, 2023

Thalley removed their request for review December 22, 2023 13:08

LingaoM force-pushed the add_mdf_support branch from c24e2da to 4fa0c42 Compare December 23, 2023 11:03

zephyrbot requested a review from Thalley December 23, 2023 11:04

LingaoM removed the request for review from Thalley December 23, 2023 11:04

LingaoM force-pushed the add_mdf_support branch from 4fa0c42 to e746579 Compare December 23, 2023 11:34

zephyrbot requested a review from Thalley December 23, 2023 11:34

LingaoM force-pushed the add_mdf_support branch 3 times, most recently from 0a2feb4 to 931d83d Compare December 23, 2023 12:20

LingaoM marked this pull request as draft December 23, 2023 12:32

LingaoM removed the DNM This PR should not be merged (Do Not Merge) label Dec 23, 2023

LingaoM force-pushed the add_mdf_support branch 3 times, most recently from 57d2b18 to bcb4ed3 Compare December 25, 2023 04:50

LingaoM force-pushed the add_mdf_support branch from 3528d0f to 8cb610e Compare January 3, 2024 06:30

LingaoM removed the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Jan 3, 2024

Thalley removed their request for review January 3, 2024 07:01

LingaoM force-pushed the add_mdf_support branch from 8cb610e to cf47259 Compare January 3, 2024 11:16

zephyrbot requested a review from Thalley January 3, 2024 11:17

LingaoM force-pushed the add_mdf_support branch 5 times, most recently from bb735bb to a8709df Compare January 9, 2024 09:27

Bluetooth: Mesh: Add Directed Forwarding Support

25cfe86

Add Directed Forwarding Support Signed-off-by: Lingao Meng <[email protected]>

LingaoM force-pushed the add_mdf_support branch from a8709df to 25cfe86 Compare January 11, 2024 01:36

Thalley removed their request for review January 11, 2024 09:03

LingaoM commented Jan 16, 2024

View reviewed changes

alxelax requested changes Jan 16, 2024

View reviewed changes

PavelVPV requested changes Jan 25, 2024

View reviewed changes

PavelVPV mentioned this pull request Jan 25, 2024

Bluetooth: Mesh: Directed Forwarding #66480

Open

32 tasks

PavelVPV reviewed Jan 30, 2024

View reviewed changes

PavelVPV reviewed Jan 31, 2024

View reviewed changes

henrikbrixandersen removed this from the future milestone Mar 7, 2024

github-actions bot added the Stale label May 7, 2024

github-actions bot closed this May 21, 2024

	/** @brief Get the binding network key index of the application key.
	/** @brief Get the bound subnetwork index by the application index.

	* @return The binding network key index if Application is known, BT_MESH_KEY_UNUSED otherwise.
	* @return The subnetwork index if it is bound to provided application index, BT_MESH_KEY_UNUSED otherwise.

	uint16_t bt_mesh_app_binding_net_key_get(uint16_t app_idx);
	uint16_t bt_mesh_subnet_by_app_idx_get(uint16_t app_idx);


		}

		static void dfw_state_machine_state_set(struct dfw_state_machine *machine,

		fw_new->echo_intv.ticks *= percent;
		fw_new->echo_intv.ticks /= 100;

	LOG_DBG("Restored directed forwarding subnet configuration value");
	LOG_DBG("Restored directed forwarding configuration value for subnet %#03x", net_idx);

	if (mod->pub && mod->pub->addr != BT_MESH_ADDR_UNASSIGNED) {
	int32_t ms = bt_mesh_model_pub_period_get(mod);

	if (IS_ENABLED(CONFIG_BT_MESH_DFW) && mod->pub->cred == BT_MESH_CRED_DIRECTED) {
	(void)bt_mesh_dfw_path_origin_state_machine_start(
	bt_mesh_app_binding_net_key_get(mod->pub->key),
	NULL, mod->pub->addr, true);
	}

	if (mod->pub->update && ms > 0) {
	/* Delay the first publication after power-up for longer time (section
	* 3.7.3.1):
	*
	* When the publication of a message is the result of a power-up, a state
	* transition progress update, or completion of a state transition, multiple
	* nodes may be reporting the state change at the same time. To reduce the
	* probability of a message collision, these messages should be sent with a
	* random delay between 20 and 500 milliseconds.
	*/
	uint16_t random;

	random = !!mod->pub->delayable ? pub_delay_get(RANDOM_DELAY_LONG) : 0;

	LOG_DBG("Starting publish timer (period %u ms, delay %u ms)", ms, random);
	k_work_schedule(&mod->pub->timer, K_MSEC(ms + random));
	}
	}

Bluetooth: Mesh: Add Directed Forwarding Support #66883

Bluetooth: Mesh: Add Directed Forwarding Support #66883

Conversation

LingaoM commented Dec 22, 2023 • edited Loading

Add Directed Forwarding Support

PTS Case(not passed yet, other passed test case not list):

Directed Forwarding

Directed Forwarding Discovery

Directed Forwarding Establishment

Directed Forwarding Echo

Directed Forwarding Model

Directed Control State

Forwarding Table State

Directed Control Network Transmit State

LingaoM commented Jan 3, 2024

omkar3141 commented Jan 3, 2024 • edited Loading

LingaoM commented Jan 3, 2024

omkar3141 commented Jan 3, 2024

LingaoM commented Jan 3, 2024 • edited Loading

alxelax commented Jan 11, 2024 • edited Loading

LingaoM Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

LingaoM commented Jan 16, 2024 • edited Loading

alxelax left a comment • edited Loading

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

PavelVPV left a 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

PavelVPV left a 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

PavelVPV left a 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

github-actions bot commented May 7, 2024

LingaoM commented Dec 22, 2023 •

edited

Loading

omkar3141 commented Jan 3, 2024 •

edited

Loading

LingaoM commented Jan 3, 2024 •

edited

Loading

alxelax commented Jan 11, 2024 •

edited

Loading

LingaoM Jan 16, 2024 •

edited

Loading

LingaoM commented Jan 16, 2024 •

edited

Loading

alxelax left a comment •

edited

Loading