share 2 patch for ack extension #17

LeonCT · 2021-08-13T15:13:13Z

hi：
Thank you very much for the convenience of your excellent code framework.and I want to share 2 patch for ack extension.
please review If code requirements are met.Thank you very much！

N/A add usrdata for cAT ack Signed-off-by: chentao9 <[email protected]>

N/A add usrdata buf to cAT's ack and initialization Signed-off-by: chentao9 <[email protected]>

marcinbor85 · 2021-08-15T07:48:14Z

src/cat.c

@@ -72,6 +72,9 @@ static void reset_state(struct cat_object *self)
        }
        self->cmd = NULL;
        self->cmd_type = CAT_CMD_TYPE_NONE;
+        #if CONFIG_LIB_CAT_USRDATA


at this moment in cAT library there is no macro-based configuration feature. adding this kind of feature should be separated task.

marcinbor85 · 2021-08-15T07:49:22Z

src/cat.c

@@ -72,6 +72,9 @@ static void reset_state(struct cat_object *self)
        }
        self->cmd = NULL;
        self->cmd_type = CAT_CMD_TYPE_NONE;
+        #if CONFIG_LIB_CAT_USRDATA
+        cat_user_data_init();


calling functions from external cAT-related modules needs to pass current context (self pointer). it allows to continue object-oriented architecture.

marcinbor85 · 2021-08-15T07:49:34Z

src/cat.c

@@ -323,7 +326,23 @@ static void ack_error(struct cat_object *self)
 {
        assert(self != NULL);

+        #if CONFIG_LIB_CAT_USRDATA


marcinbor85 · 2021-08-15T07:52:05Z

src/cat.c

+        #if CONFIG_LIB_CAT_USRDATA
+        char *strbuf = (char *)malloc(512);
+        assert(NULL == strbuf);
+        memset(strbuf,0,ACKBUF_LEN);


using dynamic allocation for this purpose is completely unnecessary. first of all, one of the major requirements is to completely avoid dynamic allocations in this library. at second, You could use variable array length, for this purpose.

marcinbor85 · 2021-08-15T07:52:47Z

src/cat_usrdata.c

+ * @param {cat_user_data_ack_e} ack:ok/error
+ * @return:databuf pointer
+ */
+uint8_t *get_cat_user_databuf(cat_user_data_ops_e ops,cat_user_data_ack_e ack)


all cat-related functions should be prefixed with cat_ prefix

marcinbor85 · 2021-08-15T07:54:50Z

src/cat.c

+        memset(strbuf,0,ACKBUF_LEN);
+        if(get_cat_user_databuf_errorcode())
+        {
+                sprintf(strbuf,"ERROR%s",get_cat_user_databuf(CAT_USER_DATABUF_OPS_WRITE, CAT_USER_DATABUF_ACK_ERR));


differentation (and customization) of the automatic ack/error response could be implemented more generic way, for e.g. with optional callbacks with return codes.

marcinbor85 · 2021-08-15T07:55:04Z

src/cat.c

+        char *strbuf = (char *)malloc(ACKBUF_LEN);
+        assert(NULL == strbuf);
+        memset(strbuf,0,ACKBUF_LEN);
+        sprintf(strbuf,"OK%s",get_cat_user_databuf(CAT_USER_DATABUF_OPS_WRITE, CAT_USER_DATABUF_ACK_OK));


marcinbor85 · 2021-08-15T07:55:20Z

src/cat_usrdata.c

+/****************************************************************************
+ * src/cat_usrdata.c
+ *
+ * Copyright (C) 2021 Xiaomi Corperation


marcinbor85 · 2021-08-15T07:55:30Z

src/cat.h

@@ -32,6 +32,9 @@ extern "C" {
 #include <stdint.h>
 #include <stdio.h>
 #include <stdbool.h>
+#if CONFIG_LIB_CAT_USRDATA
+#include "cat_usrdata.h"


marcinbor85 · 2021-08-15T07:56:11Z

src/cat_usrdata.c

+{
+        assert(IS_DATA_OPS(ops));
+        assert(IS_DATA_ACK(ack));
+        memset((cat_user_data.data_buf[ops][ack].buf),0,WRITEBUF_LEN);


spaces between comas and args...

marcinbor85 · 2021-08-15T07:57:49Z

src/cat_usrdata.c

+#include "cat.h"
+#include "cat_usrdata.h"
+
+static cat_user_data_t cat_user_data;


this static allocation makes it impossible to use more than one cAT instance in single system. pointer to this data should be passed in function args to continue object-oriented approach

marcinbor85 · 2021-08-15T07:59:21Z

src/cat_usrdata.h

+/*user define*/
+#define WRITEBUF_LEN        256
+#define READBUF_LEN         WRITEBUF_LEN
+#define ACKBUF_LEN          512


hardcoded buffers sizes should be strongly avoded (please use static descriptors instead)

marcinbor85 · 2021-08-15T07:59:41Z

src/cat_usrdata.h

+#define IS_DATA_ACK(n)  ((CAT_USER_DATABUF_ACK_OK == n) || \
+                                (CAT_USER_DATABUF_ACK_ERR == n))
+
+bool cat_user_databuf_clear(cat_user_data_ops_e ops,cat_user_data_ack_e ack);


add comments (source code functions documentation)

marcinbor85 · 2021-08-15T08:01:17Z

besides of all above, all new features require add some basic unit tests or at least examples to show how to use it.

LeonCT · 2021-08-15T08:42:06Z

besides of all above, all new features require add some basic unit tests or at least examples to show how to use it.

Thank you for your improvement suggestions after the code review. I will revise it according to the suggestions and resubmit it. Please review it later.I really appreciate it！

chentao9 added 2 commits August 13, 2021 22:25

feature:cAT: add usrdata for cAT ack

9437ee2

N/A add usrdata for cAT ack Signed-off-by: chentao9 <[email protected]>

feature:cAT: add usrdata to cAT process

82daa32

N/A add usrdata buf to cAT's ack and initialization Signed-off-by: chentao9 <[email protected]>

marcinbor85 self-requested a review August 15, 2021 07:45

marcinbor85 reviewed Aug 15, 2021

View reviewed changes

xiaoxiang781216 mentioned this pull request Aug 27, 2022

Remove the tail space from all source code #27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

share 2 patch for ack extension #17

share 2 patch for ack extension #17

LeonCT commented Aug 13, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021

marcinbor85 Aug 15, 2021 •

edited

Loading

marcinbor85 commented Aug 15, 2021 •

edited

Loading

LeonCT commented Aug 15, 2021

share 2 patch for ack extension #17

Are you sure you want to change the base?

share 2 patch for ack extension #17

Conversation

LeonCT commented Aug 13, 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

marcinbor85 Aug 15, 2021 • edited Loading

Choose a reason for hiding this comment

marcinbor85 commented Aug 15, 2021 • edited Loading

LeonCT commented Aug 15, 2021

marcinbor85 Aug 15, 2021 •

edited

Loading

marcinbor85 commented Aug 15, 2021 •

edited

Loading