Bluetooth: Mesh: Fix OOB upload in Fw Distribution server #63739

PavelVPV · 2023-10-10T09:39:04Z

The PR fixes bugs related to OOB upload.

This PR also adds support for the following PTS test:

DFU/SR/FD/BV-06-C
DFU/SR/FD/BV-13-C
DFU/SR/FD/BV-14-C
DFU/SR/FD/BV-15-C
DFU/SR/FD/BV-16-C
DFU/SR/FD/BV-17-C
DFU/SR/FD/BV-18-C
DFU/SR/FD/BV-22-C

In OOB upload, when Check Firmware OOB procedure completes successfully and the firmware is already received, we send Firmware Distribution Upload Status message with update Phase set to Transfer Success. In this case, we must set Upload Progress to 100%. This can't be done through the callback as the application layer doesn't yet know that the firmware is already received. This will happen by the exist from bt_mesh_dfd_srv_oob_check_complete function, which will return error code -EEXIST. Signed-off-by: Pavel Vasilyev <[email protected]>

Balaklaka · 2023-10-10T11:38:38Z

tests/bluetooth/tester/src/btp_mesh.c

@@ -21,7 +21,7 @@

 #include <zephyr/logging/log.h>
 #define LOG_MODULE_NAME bttester_mesh
-LOG_MODULE_REGISTER(LOG_MODULE_NAME, CONFIG_BTTESTER_LOG_LEVEL);
+LOG_MODULE_REGISTER(LOG_MODULE_NAME, 4);//CONFIG_BTTESTER_LOG_LEVEL);


Should this be reverted back.

Balaklaka · 2023-10-10T11:50:33Z

tests/bluetooth/tester/src/btp_mesh.c

+
+		dfd_srv_oob_ctx.progress = MIN(dfd_srv_oob_ctx.progress + 25, 99);
+
+		if (dfd_srv_oob_ctx.progress == 99) {


Should this be >= or <=, or is it correct with exact 99. dfd_srv_oob_ctx.progress can be less than 99.

ASFAIK The image is ready for storing if progress is exactly 99. After storing it will be 100.

No, MIN will not let it to be set greater than 99.

alxelax

Seems Ok but requires a bit more changes.

subsys/bluetooth/mesh/dfd_srv.c

tests/bluetooth/tester/src/btp_mesh.c

alxelax · 2023-10-10T12:38:53Z

tests/bluetooth/tester/src/btp_mesh.c

+
+		dfd_srv_oob_ctx.progress = MIN(dfd_srv_oob_ctx.progress + 25, 99);
+
+		if (dfd_srv_oob_ctx.progress == 99) {


ASFAIK The image is ready for storing if progress is exactly 99. After storing it will be 100.

tests/bluetooth/tester/src/btp_mesh.c

This message _at least_ 2 bytes long, but can be longer, thus BT_MESH_LEN_MIN should be used. Signed-off-by: Pavel Vasilyev <[email protected]>

If a Fw Distribution Client sends the Upload OOB Start message, but the application layer didn't call bt_mesh_dfd_srv_oob_check_complete yet, we have no other option other than ignore the message. The next phase in this case could be Transfer Active, Transfer Success or Failed and it will be set only after Check Firmware OOB procedure completes. Signed-off-by: Pavel Vasilyev <[email protected]>

If the previous upload was in-band and it didn't complete, the slot will stay reserved. By design we release slot not at the end of the upload phase, but at the start of a new upload phase. This fixes DFU/SR/FD/BV-13-C. Signed-off-by: Pavel Vasilyev <[email protected]>

Enable all OOB upload tests except DFU/SR/FD/BV-58-C as we don't support Firmware Retrieval Over HTTPS procedure. Zephyr PR: zephyrproject-rtos/zephyr#63739 Signed-off-by: Pavel Vasilyev <[email protected]>

jhedberg · 2023-10-10T14:46:43Z

@PavelVPV is this something you think should be merged for 3.5? If so, please set the target milestone appropriately.

Enable all OOB upload tests except DFU/SR/FD/BV-58-C as we don't support Firmware Retrieval Over HTTPS procedure. Zephyr PR: zephyrproject-rtos/zephyr#63739 Signed-off-by: Pavel Vasilyev <[email protected]>

ludvigsj · 2023-10-11T07:09:36Z

tests/bluetooth/tester/src/btp_mesh.c

+		status = BT_MESH_DFD_ERR_URI_NOT_SUPPORTED;
+	} else if (dfd_srv_oob_ctx.uri_len < 7 || memcmp(dfd_srv_oob_ctx.uri,
+							 SUPPORTED_SCHEME "://",
+							 strlen(SUPPORTED_SCHEME) + 3)) {


If this passes PTS then I guess it's fine, but I think returning SUCCESS for e.g. "http://?:?" (which this does as far as I can tell) is wrong, and can fail if PTS changes. At the very least, make a comment here about this so that if this fails in the future when PTS becomes more diligent then we easily can find where to fix it?

~~This was wrong, http://?:? is actually valid I think. My second comment still stands~~

Forget this entire comment, I'm not understanding the RFC I think

This also returns URI_MALFORMED for "https://example.com/" which is definitely wrong, can easilly be fixed by checking the entire SUPPORTED_SCHEME ":" also when checking for supported URI. It also returns MALFORMED for "http:something" which is correct according to the RFC (even though I think it's invalid in the HTTP spec but that's not what we should check here)

This allow to run the following PTS test: - DFU/SR/FD/BV-06-C - DFU/SR/FD/BV-13-C - DFU/SR/FD/BV-14-C - DFU/SR/FD/BV-15-C - DFU/SR/FD/BV-16-C - DFU/SR/FD/BV-17-C - DFU/SR/FD/BV-18-C - DFU/SR/FD/BV-22-C Signed-off-by: Pavel Vasilyev <[email protected]>

this is useful for debugging. Signed-off-by: Pavel Vasilyev <[email protected]>

PavelVPV · 2023-10-11T08:52:06Z

@PavelVPV is this something you think should be merged for 3.5? If so, please set the target milestone appropriately.

Added milestone (hope it is not too late).

jhedberg · 2023-10-11T09:26:56Z

@PavelVPV is this something you think should be merged for 3.5? If so, please set the target milestone appropriately.

Added milestone (hope it is not too late).

Is this fixing any bugs? I don't see any "Fixes: ..." entry.

PavelVPV · 2023-10-11T10:45:40Z

@PavelVPV is this something you think should be merged for 3.5? If so, please set the target milestone appropriately.

Added milestone (hope it is not too late).

Is this fixing any bugs? I don't see any "Fixes: ..." entry.

Sorry, added now.

PavelVPV requested review from alwa-nordic, jhedberg, Vudentz, sjanc and LingaoM as code owners October 10, 2023 09:39

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

zephyrbot requested review from akredalen, alxelax, Andrewpini, asbjornsabo, hermabe and Thalley October 10, 2023 09:39

zephyrbot assigned PavelVPV Oct 10, 2023

Balaklaka self-requested a review October 10, 2023 11:40

Balaklaka reviewed Oct 10, 2023

View reviewed changes

PavelVPV force-pushed the dfd_oob_fixes branch from 2dc37c3 to 1d958f9 Compare October 10, 2023 12:50

alxelax requested changes Oct 10, 2023

View reviewed changes

Bluetooth: Mesh: Fix Fw Dist Upload OOB Start msg length check

c0ffe4b

This message _at least_ 2 bytes long, but can be longer, thus BT_MESH_LEN_MIN should be used. Signed-off-by: Pavel Vasilyev <[email protected]>

PavelVPV force-pushed the dfd_oob_fixes branch from 1d958f9 to cb019af Compare October 10, 2023 12:56

PavelVPV requested review from alxelax and Balaklaka October 10, 2023 12:56

PavelVPV force-pushed the dfd_oob_fixes branch 3 times, most recently from e1808bc to 2d33737 Compare October 10, 2023 13:07

alxelax previously approved these changes Oct 10, 2023

View reviewed changes

PavelVPV added 2 commits October 10, 2023 15:35

PavelVPV dismissed alxelax’s stale review via 610a41b October 10, 2023 13:35

PavelVPV force-pushed the dfd_oob_fixes branch from 2d33737 to 610a41b Compare October 10, 2023 13:35

PavelVPV mentioned this pull request Oct 10, 2023

mesh: Enable Fw Distribution server OOB upload tests auto-pts/auto-pts#1031

Merged

PavelVPV requested a review from alxelax October 10, 2023 14:23

alxelax previously approved these changes Oct 10, 2023

View reviewed changes

jhedberg added the area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests label Oct 10, 2023

Balaklaka approved these changes Oct 10, 2023

View reviewed changes

ludvigsj reviewed Oct 11, 2023

View reviewed changes

PavelVPV added 2 commits October 11, 2023 10:50

Bluetooth: Mesh: Print URI and FWID in Upload OOB Start msg

8ec9219

this is useful for debugging. Signed-off-by: Pavel Vasilyev <[email protected]>

PavelVPV dismissed alxelax’s stale review via 8ec9219 October 11, 2023 08:51

PavelVPV force-pushed the dfd_oob_fixes branch from 610a41b to 8ec9219 Compare October 11, 2023 08:51

PavelVPV requested review from alxelax and ludvigsj October 11, 2023 08:51

PavelVPV added this to the v3.5.0 milestone Oct 11, 2023

ludvigsj approved these changes Oct 11, 2023

View reviewed changes

PavelVPV changed the title ~~tests: bluetooth: tester Add support for OOB upload tests in Fw Distribution server~~ Bluetooth: Mesh: Fix OOB upload in Fw Distribution server Oct 11, 2023

jhedberg approved these changes Oct 11, 2023

View reviewed changes

fabiobaltieri approved these changes Oct 11, 2023

View reviewed changes

jhedberg merged commit 2c9fc04 into zephyrproject-rtos:main Oct 11, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Mesh: Fix OOB upload in Fw Distribution server #63739

Bluetooth: Mesh: Fix OOB upload in Fw Distribution server #63739

PavelVPV commented Oct 10, 2023 •

edited

Loading

Balaklaka Oct 10, 2023

Balaklaka Oct 10, 2023

alxelax Oct 10, 2023

PavelVPV Oct 10, 2023

alxelax left a comment

alxelax Oct 10, 2023

jhedberg commented Oct 10, 2023

ludvigsj Oct 11, 2023 •

edited

Loading

ludvigsj Oct 11, 2023 •

edited

Loading

PavelVPV Oct 11, 2023

PavelVPV commented Oct 11, 2023

jhedberg commented Oct 11, 2023

PavelVPV commented Oct 11, 2023


		dfd_srv_oob_ctx.progress = MIN(dfd_srv_oob_ctx.progress + 25, 99);

		if (dfd_srv_oob_ctx.progress == 99) {

Bluetooth: Mesh: Fix OOB upload in Fw Distribution server #63739

Bluetooth: Mesh: Fix OOB upload in Fw Distribution server #63739

Conversation

PavelVPV commented Oct 10, 2023 • edited Loading

Balaklaka Oct 10, 2023

Choose a reason for hiding this comment

Balaklaka Oct 10, 2023

Choose a reason for hiding this comment

alxelax Oct 10, 2023

Choose a reason for hiding this comment

PavelVPV Oct 10, 2023

Choose a reason for hiding this comment

alxelax left a comment

Choose a reason for hiding this comment

alxelax Oct 10, 2023

Choose a reason for hiding this comment

jhedberg commented Oct 10, 2023

ludvigsj Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

ludvigsj Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

PavelVPV Oct 11, 2023

Choose a reason for hiding this comment

PavelVPV commented Oct 11, 2023

jhedberg commented Oct 11, 2023

PavelVPV commented Oct 11, 2023

PavelVPV commented Oct 10, 2023 •

edited

Loading

ludvigsj Oct 11, 2023 •

edited

Loading

ludvigsj Oct 11, 2023 •

edited

Loading