Bluetooth: Mesh: Add support for Upload OOB Start #61389

ludvigsj · 2023-08-10T14:39:40Z

This adds support for the Upload OOB Start message to the DFD server, by providing callbacks that the application can use to hook any OOB scheme into the model behavior.

There are also extensive changes to the dfu_slot module, to accomodate the new needs that appeared with the support for OOB transfer (mainly, fwid, size and metadata are no longer available when the slot is allocated, they appear later in the handling).

subsys/bluetooth/mesh/shell/dfu.c

omkar3141 · 2023-08-15T07:21:09Z

include/zephyr/bluetooth/mesh/dfd_srv.h

+	 *  @c bt_mesh_dfd_srv_oob_store_complete.
+	 *
+	 *  @param srv          Firmware Distribution Server model instance.
+	 *  @param slot         Slot used for the upload.


How about this?

Suggested change

* @param slot Slot used for the upload.

* @param slot Slot to be used for the upload.

omkar3141 · 2023-08-15T07:22:38Z

include/zephyr/bluetooth/mesh/dfd_srv.h

@@ -75,6 +87,60 @@ struct bt_mesh_dfd_srv_cb {
 		    const struct bt_mesh_dfu_slot *slot,
 		    const struct bt_mesh_blob_io **io);

+	/** @brief Firware upload OOB start callback.


Not related to this PR, but should we start indicating globablly which params are inputs and which params are outputs or in-out?

omkar3141 · 2023-08-15T07:24:27Z

include/zephyr/bluetooth/mesh/dfd_srv.h

+	 *  @return BT_MESH_DFD_SUCCESS on success, or error code otherwise.
+	 */
+	int (*start_oob_upload)(struct bt_mesh_dfd_srv *srv,
+				const struct bt_mesh_dfu_slot *slot,


Didn't we discuss somewhere that image identification at application will be based on FWID rather than a slot? or was it in different context?

We decided to stop using slot index for image identification, and that further changes to this would require more discussion. I just kept the slot pointer since that was what already was used and it doesn't have the problems the idx method has with the index of a slot able to change when earlier slots are removed. If you'd like a different solution here, I'd like that to be its own issue.

omkar3141 · 2023-08-15T07:27:33Z

include/zephyr/bluetooth/mesh/dfd_srv.h

 	{                                                                      \
 		.cb = _cb,                                                     \
 		.dfu = BT_MESH_DFU_CLI_INIT(&_bt_mesh_dfd_srv_dfu_cb),         \
 		.upload = {                                                    \
 			.blob = { .cb = &_bt_mesh_dfd_srv_blob_cb },           \
 		},                                                             \
+		.oob_schemes = {                                               \


Since these will be populated by application, somedocstrings would help.

omkar3141 · 2023-08-15T07:29:41Z

include/zephyr/bluetooth/mesh/dfd_srv.h

 };

+/** @brief Call when an OOB check has completed or failed
+ *
+ *  This should be called by the application after an OOB check started by the @c start_oob


Suggested change

* This should be called by the application after an OOB check started by the @c start_oob

* This should be called by the application after an OOB check started by the @c start_oob_upload

omkar3141 · 2023-08-15T07:42:31Z

subsys/bluetooth/mesh/dfd_srv.c

@@ -461,7 +490,7 @@ static int handle_upload_start(struct bt_mesh_model *mod, struct bt_mesh_msg_ctx
 				    timeout_base);
 	if (err) {
 		LOG_ERR("BLOB Server rejected upload (err: %d)", err);


Not realated to this PR, but we don't have uniform debug strings. Sometimes its ... err: %d <something else>, and sometimes it is ... (err: %d). Could be done as a seperate task.

omkar3141 · 2023-08-16T07:54:51Z

subsys/bluetooth/mesh/dfu_slot.h

- *  @return 0 on success, or (negative) error code on failure.
+ *  @return 0 on success, (negative) error code otherwise.
+ */
+int bt_mesh_dfu_slot_fwid_set(struct bt_mesh_dfu_slot *dfu_slot,


I think you mentioned this, but I don't recollect. Could tell me again why there are seperate APIs for fwid_set and info_set?

Because during OOB, fwid must be set after the check is done, whereas size and metadata will be known only after the image is done transferring.

Ahh right. Then, I think, we need to clarify this a bit. Because then "fwid" is the "new fwid" ... and application should not mix that with the "old fwid". Because this part is not explained in this API documentation. Slots are identified by the "fwid" contained in them, and not the "fwid" of the firmware on the chip or the remote node.

Keep in mind that this file is an internal API, so no application will call these directly. But I'll clarify a bit.

omkar3141 · 2023-08-16T11:23:12Z

subsys/bluetooth/mesh/dfu_slot.c

 	}

-	sys_slist_init(&list);
+	committed_slot_count--;


Looks like, the logic in this API will fail if higher layer calls this API twice passing the same *dfu_slot. Potentially causing committed_slot_count to underflow.? Please check that committed_slot_count manipulations are sufficiently guarded since lot of logic in this module depends on correct value of this counter.

I did think about just counting the number of COMMITTED slots every time we need the count, but decided on this as a CPU cycle optimization. I'm fine with removing this counter to reduce risk.

Also, no, it won't fail on passing the same dfu_slot twice. It will return on line 224 in that case, because the first call will set the state to FREE and then if will fail the check.

omkar3141 · 2023-08-16T11:26:19Z

subsys/bluetooth/mesh/dfu_slot.c

-	SYS_SLIST_FOR_EACH_CONTAINER(&list, s, n) {
-		if (!idx--) {
-			return &s->slot;
+struct bt_mesh_dfu_slot *bt_mesh_dfu_slot_at(uint16_t idx)


To avoid confusion for API users, can we rename slot index (idx) to slot identifier (id)? Index kinds of hints that it is a sequencial thing. Where as identifier need not be sequential.

Suggested change

struct bt_mesh_dfu_slot *bt_mesh_dfu_slot_at(uint16_t idx)

struct bt_mesh_dfu_slot *bt_mesh_dfu_slot_get_by_id(uint16_t id

```suggestion

struct bt_mesh_dfu_slot *bt_mesh_dfu_slot_at(uint16_t idx)

```)

But it is sequential? slot_at(0) will get the first commited slot, slot_at(1) will get the second, etc.? In commit order.

Well, then why does it doubly checks
slots[i].idx == idx ? Wouldn't it be enough to simply return by using idx for array referencing? But that won't work. AFAICS, If the device has four slots, slot_data_load() can assign idx of 3 to slot[0] (depending on where it is positioned in the settings flash since entries can be stored in different order)? This is clear if someone read the relavent code, but it is very easy incorrectly infer that idx actually represents the physical ordering of the slots on the flash by just reading API decriptions and nomenclature. Hence the suggestion.

The fact that the slots exist in an array is not supposed to exist outside this file. The only place that a slot has "two indices" (i and idx in this function for example) are inside this file. All APIs use idx to mean slot::idx. The name "index" is also what the spec uses for this field, so it makes sense for outside viewers. I don't feel like changing this. Meaning: Where a slot is in the array is an implementation detail. No one should care and no one outside this file should know. They should not even care that it is an array.

And, yes. array location can be different from slot::idx. This is the entire point of having the slot::idx field.

My only comment is about the name choosen to represent the idx. In all of mesh code base, idx variable name is always used to represent some kind of index. Just like firmware has Firmware ID, slot has a Slot ID?

No, it's not an ID. An ID I would expect to be constant for the lifetime of the slot. This index is not. It's the "Index" from 4.3.5 in the spec. All places in the API that mention idx refer to the "Index" from that chapter. As in: If we order the commited slots in the order they were added, what index of that (abstract) list is the slot at. (This list being the "Firmware Images List" state from 4.3.5). Position in the list may change if an earlier slot is removed. I still can't see why it's confusing to call it index. It's only confusing if you think about the backing array used for storing the slots, but no one should do that, they should just look at the APIs where idx is always == "Index" from 4.3.5.

Hmm, now I get it with that. Ok, can this be then img_idx or imglist_idx?

I don't see much point, seeing as there are no other indexes in this API, but sure. I'll change it everywhere then, to be consistent.

omkar3141 · 2023-08-16T12:31:26Z

subsys/bluetooth/mesh/dfu_slot.c


 	return 0;
 }

-int bt_mesh_dfu_slot_del_all(void)
+int bt_mesh_dfu_slot_del(const struct bt_mesh_dfu_slot *dfu_slot)


Can you check ideompotencay of important slot APIs in the test. Like "del", "release", "add", and "del_all". Calling these APIs multiple times with same params, should not result in misbehavior.

tests/bsim/bluetooth/mesh/tests_scripts/dfu/dfu_slot.sh

tests/bsim/bluetooth/mesh/src/test_dfu.c

subsys/bluetooth/mesh/shell/dfu.c

include/zephyr/bluetooth/mesh/dfd_srv.h

subsys/bluetooth/mesh/dfd_srv.c

subsys/bluetooth/mesh/dfu_slot.c

omkar3141 · 2023-08-21T13:06:55Z

include/zephyr/bluetooth/mesh/dfd_srv.h

+ *  * @c BT_MESH_DFD_ERR_NEW_FW_NOT_AVAILABLE if the check completes successfully but no new
+ *    firmware is available.
+ *
+ *  If this function returns 0, the application should then download the firmware to the


You can leave this as is.

omkar3141 · 2023-08-21T13:10:58Z

include/zephyr/bluetooth/mesh/dfd_srv.h

+	 *
+	 *  Called at the start of an OOB firmware upload. The application must
+	 *  start a firmware check using an OOB mechanism, and then call
+	 *  @c bt_mesh_dfd_srv_oob_check_complete. Depending on the return value


How exactly depending on the return value of that function application makes a decision? It will be more clear, if that aspect is spelt out in doc string. It may be a slight repetition, but it would be useful.

"If return value is , the application must do , otherwise does ."

The documentation for bt_mesh_dfd_srv_oob_check_complete already explains this. I don't think it's needed twice.

omkar3141 · 2023-08-21T13:12:05Z

include/zephyr/bluetooth/mesh/dfd_srv.h

+
+/** @brief Call when an OOB store has completed or failed
+ *
+ *  This should be called by the application after an OOB store started by the @c start_oob


I didn't get this.

omkar3141 · 2023-08-21T13:17:50Z

subsys/bluetooth/mesh/dfu_slot.c

-	SYS_SLIST_FOR_EACH_CONTAINER(&list, s, n) {
-		if (!idx--) {
-			return &s->slot;
+struct bt_mesh_dfu_slot *bt_mesh_dfu_slot_at(uint16_t idx)


My only comment is about the name choosen to represent the idx. In all of mesh code base, idx variable name is always used to represent some kind of index. Just like firmware has Firmware ID, slot has a Slot ID?

omkar3141

Please check response to comments.

subsys/bluetooth/mesh/dfu_slot.h

subsys/bluetooth/mesh/dfu_slot.c

@PavelVPV

I want't have ability to control my comments next 3 weeks since vacation time. I remove my review to not block PR. @PavelVPV could you control that comments were considered?

PavelVPV · 2023-08-24T11:44:25Z

I want't have ability to control my comments next 3 weeks since vacation time. I remove my review to not block PR. @PavelVPV could you control that comments were considered?

Sure!

mia-ko · 2023-08-24T07:42:32Z

include/zephyr/bluetooth/mesh/dfd_srv.h

@@ -75,6 +87,60 @@ struct bt_mesh_dfd_srv_cb {
 		    const struct bt_mesh_dfu_slot *slot,
 		    const struct bt_mesh_blob_io **io);

+	/** @brief Firware upload OOB start callback.


Suggested change

/** @brief Firware upload OOB start callback.

/** @brief Firmware upload OOB start callback.

mia-ko · 2023-08-24T07:43:59Z

include/zephyr/bluetooth/mesh/dfd_srv.h

+	 *  @param srv          Firmware Distribution Server model instance.
+	 *  @param slot         Slot used for the upload.
+	 *  @param uri          Pointer to buffer containing the URI used to
+	 *                      check for new Firmware.


Suggested change

* check for new Firmware.

* check for new firmware.

mia-ko · 2023-08-24T07:46:39Z

include/zephyr/bluetooth/mesh/dfd_srv.h

+ *  callback has completed or failed. The @p status param should be set to one of the following
+ *  values:
+ *
+ *  * @c BT_MESH_DFD_SUCCESS if the check was succesfull and a new firmware ID was found.


firmware ID or Firmware ID like used in callbacks further up?

Actually, it looks like firmware ID is used more than Firmware ID. So maybe those other instances should be fixed instead.

tests/bsim/bluetooth/mesh/tests_scripts/dfu/dfu_slot_idempotency.sh

akredalen

I will approve once Alex's last comment is resolved.

ludvigsj · 2023-09-15T09:07:58Z

Sorry, need your reviews again, was notified of a small error I'd missed.

PavelVPV · 2023-09-18T08:18:49Z

@ludvigsj , the issue with the CI has been fixed on main, you need to rebase you PR to pass CI.

This adds support for the Upload OOB Start message to the DFD server, by providing callbacks that the application can use to hook any OOB scheme into the model behavior. There are also extensive changes to the dfu_slot module, to accomodate the new needs that appeared with the support for OOB transfer (mainly, fwid, size and metadata are no longer available when the slot is allocated, they appear later in the handling). Signed-off-by: Ludvig Samuelsen Jordet <[email protected]>

Aligne dfu sample with: zephyrproject-rtos/zephyr#61389 bt_mesh_dfu_slot no longer has 'uri' param. Signed-off-by: Alperen Şener <[email protected]>

ludvigsj requested review from jhedberg, Vudentz, sjanc, PavelVPV, wopu-ot, LingaoM and alwa-nordic as code owners August 10, 2023 14:39

zephyrbot added area: Bluetooth Mesh area: Bluetooth labels Aug 10, 2023

zephyrbot requested review from alxelax and Andrewpini August 10, 2023 14:40

zephyrbot assigned PavelVPV Aug 10, 2023

ludvigsj force-pushed the develop/NCSDK-14801_upload_oob_start_support branch 4 times, most recently from 6c9e06e to f716b95 Compare August 11, 2023 08:34

ludvigsj commented Aug 11, 2023

View reviewed changes

subsys/bluetooth/mesh/shell/dfu.c Outdated Show resolved Hide resolved

ludvigsj force-pushed the develop/NCSDK-14801_upload_oob_start_support branch from f716b95 to 702ec88 Compare August 11, 2023 10:59

omkar3141 requested changes Aug 16, 2023

View reviewed changes

alxelax previously requested changes Aug 17, 2023

View reviewed changes

omkar3141 requested changes Aug 21, 2023

View reviewed changes

PavelVPV reviewed Aug 21, 2023

View reviewed changes

subsys/bluetooth/mesh/dfu_slot.h Outdated Show resolved Hide resolved

subsys/bluetooth/mesh/dfu_slot.c Outdated Show resolved Hide resolved

subsys/bluetooth/mesh/dfu_slot.c Outdated Show resolved Hide resolved

subsys/bluetooth/mesh/dfu_slot.c Outdated Show resolved Hide resolved

alxelax mentioned this pull request Aug 22, 2023

tests: Bluetooth: Mesh: Use bsim settings backend #61074

Merged

mia-ko suggested changes Aug 24, 2023

View reviewed changes

ludvigsj force-pushed the develop/NCSDK-14801_upload_oob_start_support branch from 702ec88 to 16c090a Compare September 4, 2023 11:21

ludvigsj requested a review from nashif as a code owner September 4, 2023 11:21

zephyrbot added the area: Settings Settings subsystem label Sep 4, 2023

ludvigsj requested a review from akredalen September 14, 2023 13:48

alxelax reviewed Sep 14, 2023

View reviewed changes

tests/bsim/bluetooth/mesh/tests_scripts/dfu/dfu_slot_idempotency.sh Show resolved Hide resolved

akredalen reviewed Sep 14, 2023

View reviewed changes

ludvigsj dismissed PavelVPV’s stale review via ee61a15 September 15, 2023 06:36

ludvigsj force-pushed the develop/NCSDK-14801_upload_oob_start_support branch from 005f532 to ee61a15 Compare September 15, 2023 06:36

ludvigsj requested review from akredalen, alxelax and PavelVPV September 15, 2023 06:37

PavelVPV previously approved these changes Sep 15, 2023

View reviewed changes

akredalen previously approved these changes Sep 15, 2023

View reviewed changes

ludvigsj added the DNM This PR should not be merged (Do Not Merge) label Sep 15, 2023

ludvigsj dismissed stale reviews from akredalen and PavelVPV via 52e00d3 September 15, 2023 09:06

ludvigsj force-pushed the develop/NCSDK-14801_upload_oob_start_support branch from ee61a15 to 52e00d3 Compare September 15, 2023 09:06

ludvigsj requested review from PavelVPV and akredalen September 15, 2023 09:08

ludvigsj removed the DNM This PR should not be merged (Do Not Merge) label Sep 15, 2023

PavelVPV approved these changes Sep 15, 2023

View reviewed changes

akredalen approved these changes Sep 15, 2023

View reviewed changes

ludvigsj force-pushed the develop/NCSDK-14801_upload_oob_start_support branch from 52e00d3 to 6de0a0f Compare September 18, 2023 08:27

fabiobaltieri merged commit b990a74 into zephyrproject-rtos:main Sep 18, 2023
29 of 35 checks passed

m-alperen-sener mentioned this pull request Oct 3, 2023

bluetooth: samples: mesh: remove uri info prints in dfu sample nrfconnect/sdk-nrf#12511

Merged

	* @param slot Slot used for the upload.
	* @param slot Slot to be used for the upload.

	* This should be called by the application after an OOB check started by the @c start_oob
	* This should be called by the application after an OOB check started by the @c start_oob_upload

	/** @brief Firware upload OOB start callback.
	/** @brief Firmware upload OOB start callback.

Bluetooth: Mesh: Add support for Upload OOB Start #61389

Bluetooth: Mesh: Add support for Upload OOB Start #61389

Conversation

ludvigsj commented Aug 10, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludvigsj Aug 16, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omkar3141 left a comment

Choose a reason for hiding this comment

PavelVPV commented Aug 24, 2023

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

akredalen left a comment

Choose a reason for hiding this comment

ludvigsj commented Sep 15, 2023

PavelVPV commented Sep 18, 2023

ludvigsj Aug 16, 2023 •

edited

Loading