mcumgr: fs_mgmt: Add a hook on download/upload complete. #87526

nicogou · 2025-03-23T16:52:31Z

Added a hook on the FS group that notify applications when a file download/upload has completed.
#87380

nordicjm · 2025-03-24T08:53:40Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+		status = mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_UPLOAD_DOWNLOAD_DONE,
+			 &upload_or_download, sizeof(upload_or_download), &err_rc, &err_group);


it should be a struct which also has the filename

nordicjm · 2025-03-24T08:53:50Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+		enum mgmt_cb_return status;
+		int32_t err_rc;
+		uint16_t err_group;
+		zcbor_state_t *zse = ctxt->writer->zs;


variables declared at top of scope

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

nordicjm · 2025-03-24T08:57:02Z

include/zephyr/mgmt/mcumgr/grp/fs_mgmt/fs_mgmt.h

+enum {
+	STATE_NO_UPLOAD_OR_DOWNLOAD = 0,
+	STATE_UPLOAD,
+	STATE_DOWNLOAD,
+};


this needs a name for the enum starting with fs_mgmt_ and FS_MGMT_ prefix on values. Also as other comment states, should be encapsulated within a struct which includes the filename

nordicjm · 2025-03-24T08:58:14Z

include/zephyr/mgmt/mcumgr/mgmt/callbacks.h

 	/** Callback when a file has been accessed, data is fs_mgmt_file_access(). */
 	MGMT_EVT_OP_FS_MGMT_FILE_ACCESS		= MGMT_DEF_EVT_OP_ID(MGMT_EVT_GRP_FS, 0),

+	MGMT_EVT_OP_FS_MGMT_UPLOAD_DOWNLOAD_DONE	= MGMT_DEF_EVT_OP_ID(MGMT_EVT_GRP_FS, 1),


MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_COMPLETE or
MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_FINISHED or MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE perhaps so it fits in the alignment?

nicogou · 2025-03-24T11:22:43Z

I did the requested changes by using the already existing fs_mgmt_file_access struct to store the filename and operation (read = download, write = upload). I also fixed an error in the comments around the fs_mgmt_file_access_types enum where upload and download where switched.

nordicjm

Not tested but changes look OK, some changes to tidy up and remove status since this is a notification hook, doesn't make sense to be able to return an error

nordicjm · 2025-04-02T09:22:13Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+
+		strcpy(path, fs_mgmt_ctxt.path);
+		file_access_data.filename = path;
+		LOG_ERR("fs_mgmt.c path: %s", file_access_data.filename);


Suggested change

LOG_ERR("fs_mgmt.c path: %s", file_access_data.filename);

nordicjm · 2025-04-02T09:22:45Z

include/zephyr/mgmt/mcumgr/mgmt/callbacks.h

 enum fs_mgmt_group_events {
 	/** Callback when a file has been accessed, data is fs_mgmt_file_access(). */
-	MGMT_EVT_OP_FS_MGMT_FILE_ACCESS		= MGMT_DEF_EVT_OP_ID(MGMT_EVT_GRP_FS, 0),
+	MGMT_EVT_OP_FS_MGMT_FILE_ACCESS			= MGMT_DEF_EVT_OP_ID(MGMT_EVT_GRP_FS, 0),


go back to old alignment

nordicjm · 2025-04-02T09:23:17Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+#if defined(CONFIG_MCUMGR_GRP_FS_FILE_ACCESS_HOOK)
+		/* Warn application that file download/upload is done. */
+		status = mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE,
+			 &file_access_data, sizeof(file_access_data), &err_rc, &err_group);


align to the variable on the previous line

nordicjm · 2025-04-02T09:24:08Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+		enum mgmt_cb_return status;
+		int32_t err_rc;
+		uint16_t err_group;
+		zcbor_state_t *zse = ctxt->writer->zs;


Suggested change

zcbor_state_t *zse = ctxt->writer->zs;

nordicjm · 2025-04-02T09:24:46Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+		struct fs_mgmt_file_access file_access_data;
+		char path[CONFIG_MCUMGR_GRP_FS_PATH_LEN + 1];


Suggested change

struct fs_mgmt_file_access file_access_data;

char path[CONFIG_MCUMGR_GRP_FS_PATH_LEN + 1];

char path[CONFIG_MCUMGR_GRP_FS_PATH_LEN + 1];

struct fs_mgmt_file_access file_access_data = {

.filename = path,

};

nordicjm · 2025-04-02T09:24:53Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+		zcbor_state_t *zse = ctxt->writer->zs;
+
+		strcpy(path, fs_mgmt_ctxt.path);
+		file_access_data.filename = path;


Suggested change

file_access_data.filename = path;

nordicjm · 2025-04-02T09:25:39Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+
+		if (status != MGMT_CB_OK) {
+			smp_add_cmd_err(zse, err_group, (uint16_t)err_rc);
+		}


Suggested change

if (status != MGMT_CB_OK) {

smp_add_cmd_err(zse, err_group, (uint16_t)err_rc);

}

The file has been uploaded/downloaded, doesn't really make sense to be able to return an error here

nordicjm · 2025-04-02T09:26:21Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+#if defined(CONFIG_MCUMGR_GRP_FS_FILE_ACCESS_HOOK)
+		struct fs_mgmt_file_access file_access_data;
+		char path[CONFIG_MCUMGR_GRP_FS_PATH_LEN + 1];
+		enum mgmt_cb_return status;


Suggested change

enum mgmt_cb_return status;

nordicjm · 2025-04-02T09:26:44Z

subsys/mgmt/mcumgr/grp/fs_mgmt/src/fs_mgmt.c

+
+#if defined(CONFIG_MCUMGR_GRP_FS_FILE_ACCESS_HOOK)
+		/* Warn application that file download/upload is done. */
+		status = mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE,


Suggested change

status = mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE,

(void)mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE, &file_access_data,

nicogou · 2025-04-13T22:05:29Z

@nordicjm took a little while to come back to this, but I made the requested changes

kartben · 2025-04-18T10:23:44Z

include/zephyr/mgmt/mcumgr/mgmt/callbacks.h

 	/** Callback when a file has been accessed, data is fs_mgmt_file_access(). */
 	MGMT_EVT_OP_FS_MGMT_FILE_ACCESS		= MGMT_DEF_EVT_OP_ID(MGMT_EVT_GRP_FS, 0),

+	MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE	= MGMT_DEF_EVT_OP_ID(MGMT_EVT_GRP_FS, 1),


missing a doxygen comment

Added a hook on the FS group that notify applications when a file download/upload has completed. Signed-off-by: Nicolas Goualard <nicolas.goualard@sfr.fr>

zephyrbot added the area: mcumgr label Mar 23, 2025

zephyrbot requested review from de-nordic and nordicjm March 23, 2025 16:53

zephyrbot assigned nordicjm Mar 23, 2025

nicogou force-pushed the fs-hook-87380 branch 2 times, most recently from 248e873 to bccc023 Compare March 23, 2025 19:52

nordicjm requested changes Mar 24, 2025

View reviewed changes

nicogou force-pushed the fs-hook-87380 branch from bccc023 to 23c151b Compare March 24, 2025 11:18

nicogou force-pushed the fs-hook-87380 branch 2 times, most recently from 94a592f to 0935965 Compare March 24, 2025 11:30

nicogou requested a review from nordicjm March 24, 2025 16:41

nordicjm reviewed Apr 2, 2025

View reviewed changes

nicogou force-pushed the fs-hook-87380 branch 2 times, most recently from f637e0a to d6e6425 Compare April 2, 2025 21:30

nicogou force-pushed the fs-hook-87380 branch 3 times, most recently from 4bf671a to c6ddf87 Compare April 12, 2025 10:13

nordicjm previously approved these changes Apr 14, 2025

View reviewed changes

kartben requested changes Apr 18, 2025

View reviewed changes

mcumgr: fs_mgmt: Add a hook on download/upload complete.

18de88a

Added a hook on the FS group that notify applications when a file download/upload has completed. Signed-off-by: Nicolas Goualard <nicolas.goualard@sfr.fr>

nicogou dismissed nordicjm’s stale review via 18de88a April 19, 2025 06:35

nicogou force-pushed the fs-hook-87380 branch from c6ddf87 to 18de88a Compare April 19, 2025 06:35

kartben approved these changes Apr 19, 2025

View reviewed changes

nordicjm approved these changes Apr 24, 2025

View reviewed changes

de-nordic approved these changes Apr 24, 2025

View reviewed changes

kartben merged commit 27bfeea into zephyrproject-rtos:main Apr 24, 2025
24 checks passed

		status = mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_UPLOAD_DOWNLOAD_DONE,
		&upload_or_download, sizeof(upload_or_download), &err_rc, &err_group);

		struct fs_mgmt_file_access file_access_data;
		char path[CONFIG_MCUMGR_GRP_FS_PATH_LEN + 1];

	status = mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE,
	(void)mgmt_callback_notify(MGMT_EVT_OP_FS_MGMT_FILE_ACCESS_DONE, &file_access_data,

mcumgr: fs_mgmt: Add a hook on download/upload complete. #87526

mcumgr: fs_mgmt: Add a hook on download/upload complete. #87526

Uh oh!

Conversation

nicogou commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicogou commented Mar 24, 2025

Uh oh!

nordicjm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicogou commented Apr 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicogou commented Mar 23, 2025 •

edited

Loading