Skip to content

share 2 patch for ack extension #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/cat.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cat_user_data_init();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#endif
}

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

#if CONFIG_LIB_CAT_USRDATA
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

char *strbuf = (char *)malloc(512);
assert(NULL == strbuf);
memset(strbuf,0,ACKBUF_LEN);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if(get_cat_user_databuf_errorcode())
{
sprintf(strbuf,"ERROR%s",get_cat_user_databuf(CAT_USER_DATABUF_OPS_WRITE, CAT_USER_DATABUF_ACK_ERR));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else
{
sprintf(strbuf,"ERROR");
}
strncpy(get_atcmd_buf(self), strbuf, get_atcmd_buf_size(self));
free(strbuf);
#else
strncpy(get_atcmd_buf(self), "ERROR", get_atcmd_buf_size(self));
#endif
start_flush_io_buffer(self, CAT_STATE_AFTER_FLUSH_RESET);
return;
}
Expand All @@ -332,7 +351,16 @@ static void ack_ok(struct cat_object *self)
{
assert(self != NULL);

#if CONFIG_LIB_CAT_USRDATA
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));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

strncpy(get_atcmd_buf(self), strbuf, get_atcmd_buf_size(self));
free(strbuf);
#else
strncpy(get_atcmd_buf(self), "OK", get_atcmd_buf_size(self));
#endif
start_flush_io_buffer(self, CAT_STATE_AFTER_FLUSH_RESET);
return;
}
Expand Down
3 changes: 3 additions & 0 deletions src/cat.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ extern "C" {
#include <stdint.h>
#include <stdio.h>
#include <stdbool.h>
#if CONFIG_LIB_CAT_USRDATA
#include "cat_usrdata.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

#endif

/* only forward declarations (looks for definition below) */
struct cat_command;
Expand Down
96 changes: 96 additions & 0 deletions src/cat_usrdata.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/****************************************************************************
* src/cat_usrdata.c
*
* Copyright (C) 2021 Xiaomi Corperation
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
****************************************************************************/

#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <stdbool.h>
#include <assert.h>
#include "cat.h"
#include "cat_usrdata.h"

static cat_user_data_t cat_user_data;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* @description:clear databuf
* @param {cat_user_data_ops_e} ops:read/write
* @param {cat_user_data_ack_e} ack:ok/error
* @return:success/fail
*/
bool cat_user_databuf_clear(cat_user_data_ops_e ops,cat_user_data_ack_e ack)
{
assert(IS_DATA_OPS(ops));
assert(IS_DATA_ACK(ack));
memset((cat_user_data.data_buf[ops][ack].buf),0,WRITEBUF_LEN);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces between comas and args...

return true;
}

/**
* @description:get databuf
* @param {cat_user_data_ops_e} ops:read/write
* @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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all cat-related functions should be prefixed with cat_ prefix

{
assert(IS_DATA_OPS(ops));
assert(IS_DATA_ACK(ack));
return (uint8_t *)(cat_user_data.data_buf[ops][ack].buf);
}

/**
* @description:get databuf size
* @param {cat_user_data_ops_e} ops:read/write
* @param {cat_user_data_ack_e} ack:ok/error
* @return:databuf size
*/
uint16_t get_cat_user_databuf_size(cat_user_data_ops_e ops,cat_user_data_ack_e ack)
{
assert(IS_DATA_OPS(ops));
assert(IS_DATA_ACK(ack));
return sizeof(cat_user_data.data_buf[ops][ack].buf);
}

/**
* @description:get errorcode
* @param {*}
* @return:errorcode
*/
uint16_t get_cat_user_databuf_errorcode(void)
{
return cat_user_data.error_code;
}

/**
* @description:set errorcode
* @param {uint16_t} val
* @return {*}
*/
void set_cat_user_databuf_errorcode(uint16_t val)
{
cat_user_data.error_code = val;
}

/**
* @description:userdata initialization
* @param {*}
* @return {*}
*/
void cat_user_data_init(void)
{
memset(&cat_user_data,0,sizeof(cat_user_data));
}
67 changes: 67 additions & 0 deletions src/cat_usrdata.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/****************************************************************************
* src/cat_usrdata.h
*
* Copyright (C) 2021 Xiaomi Corperation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
****************************************************************************/
#ifndef __CAT_USRDATA_H
#define __CAT_USRDATA_H

/*user define*/
#define WRITEBUF_LEN 256
#define READBUF_LEN WRITEBUF_LEN
#define ACKBUF_LEN 512
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/*cat user date buffer for write/read*/
typedef enum {
CAT_USER_DATABUF_OPS_WRITE = 0,
CAT_USER_DATABUF_OPS_READ,
CAT_USER_DATABUF_OPS_MAX
} cat_user_data_ops_e;

/*cat user date buffer for ack_ok/ack_error */
typedef enum {
CAT_USER_DATABUF_ACK_OK = 0,
CAT_USER_DATABUF_ACK_ERR,
CAT_USER_DATABUF_ACK_MAX
} cat_user_data_ack_e;

/*usr data buf*/
typedef struct cat_user_databuf_s
{
uint8_t buf[WRITEBUF_LEN];
}cat_user_databuf_t;

/*usr data struct*/
typedef struct cat_user_data_s
{
cat_user_databuf_t data_buf[CAT_USER_DATABUF_OPS_MAX][CAT_USER_DATABUF_ACK_MAX];
uint16_t error_code;
}cat_user_data_t;


#define IS_DATA_OPS(n) ((CAT_USER_DATABUF_OPS_WRITE == n) || \
(CAT_USER_DATABUF_OPS_READ == n))

#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);
Copy link
Owner

@marcinbor85 marcinbor85 Aug 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comments (source code functions documentation)

uint8_t *get_cat_user_databuf(cat_user_data_ops_e ops,cat_user_data_ack_e ack);
uint16_t get_cat_user_databuf_size(cat_user_data_ops_e ops,cat_user_data_ack_e ack);
uint16_t get_cat_user_databuf_errorcode(void);
void set_cat_user_databuf_errorcode(uint16_t val);
void cat_user_data_init(void);

#endif /*__CAT_USRDATA_H*/