share 2 patch for ack extension #17
Conversation
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]>
| } | ||
| self->cmd = NULL; | ||
| self->cmd_type = CAT_CMD_TYPE_NONE; | ||
| #if CONFIG_LIB_CAT_USRDATA |
There was a problem hiding this comment.
at this moment in cAT library there is no macro-based configuration feature. adding this kind of feature should be separated task.
| self->cmd = NULL; | ||
| self->cmd_type = CAT_CMD_TYPE_NONE; | ||
| #if CONFIG_LIB_CAT_USRDATA | ||
| cat_user_data_init(); |
There was a problem hiding this comment.
calling functions from external cAT-related modules needs to pass current context (self pointer). it allows to continue object-oriented architecture.
| { | ||
| assert(self != NULL); | ||
|
|
||
| #if CONFIG_LIB_CAT_USRDATA |
| #if CONFIG_LIB_CAT_USRDATA | ||
| char *strbuf = (char *)malloc(512); | ||
| assert(NULL == strbuf); | ||
| memset(strbuf,0,ACKBUF_LEN); |
There was a problem hiding this comment.
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.
| * @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) |
There was a problem hiding this comment.
all cat-related functions should be prefixed with cat_ prefix
| 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)); |
There was a problem hiding this comment.
differentation (and customization) of the automatic ack/error response could be implemented more generic way, for e.g. with optional callbacks with return codes.
| 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)); |
| /**************************************************************************** | ||
| * src/cat_usrdata.c | ||
| * | ||
| * Copyright (C) 2021 Xiaomi Corperation |
| #include <stdio.h> | ||
| #include <stdbool.h> | ||
| #if CONFIG_LIB_CAT_USRDATA | ||
| #include "cat_usrdata.h" |
| { | ||
| assert(IS_DATA_OPS(ops)); | ||
| assert(IS_DATA_ACK(ack)); | ||
| memset((cat_user_data.data_buf[ops][ack].buf),0,WRITEBUF_LEN); |
There was a problem hiding this comment.
spaces between comas and args...
| #include "cat.h" | ||
| #include "cat_usrdata.h" | ||
|
|
||
| static cat_user_data_t cat_user_data; |
There was a problem hiding this comment.
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
| /*user define*/ | ||
| #define WRITEBUF_LEN 256 | ||
| #define READBUF_LEN WRITEBUF_LEN | ||
| #define ACKBUF_LEN 512 |
There was a problem hiding this comment.
hardcoded buffers sizes should be strongly avoded (please use static descriptors instead)
| #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); |
There was a problem hiding this comment.
add comments (source code functions documentation)
|
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！ |
2 participants
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！